-
Notifications
You must be signed in to change notification settings - Fork 9
feat: add config-driven release_level utility #220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,7 @@ def register(graph_config): | |
| "worker_types", | ||
| ] | ||
| ) | ||
|
|
||
| validate_graph_config(graph_config._config) | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| # This Source Code Form is subject to the terms of the Mozilla Public | ||
| # License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| # file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
|
|
||
| import re | ||
|
|
||
|
|
||
| def release_level(release_branches, params): | ||
| """Whether this is a production release or not. | ||
|
|
||
| ``release_branches`` is the graph config's ``release-branches`` mapping of | ||
| project to the branches considered "production" for it. A value of ``True`` | ||
| for a project means every branch of that project is a release branch (the | ||
| model used by Mercurial based projects), while a list restricts releases to | ||
| the named branches. | ||
|
|
||
| :return str: One of "production" or "staging". | ||
| """ | ||
|
|
||
| if params["level"] == "3": | ||
| branches = (release_branches or {}).get(params["project"]) | ||
| if branches is True or ( | ||
| branches | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally optional, but you could break out this part into an intermediate variable to use in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've restructured the control flow a bit. Now I have 3 return statements, however I think that makes it easier to read and makes the intent and checks clear. Let me know what you think |
||
| and (m := re.match(r"refs/heads/(\S+)$", params["head_ref"])) | ||
| and m.group(1) in branches | ||
| ): | ||
| return "production" | ||
|
|
||
| return "staging" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| # This Source Code Form is subject to the terms of the Mozilla Public | ||
| # License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| # file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
|
|
||
| import pytest | ||
|
|
||
| from mozilla_taskgraph.util.attributes import release_level | ||
|
|
||
| FIREFOX_BRANCHES = ["main", "beta", "release", "esr140"] | ||
| RELEASE_BRANCHES = { | ||
| "firefox": FIREFOX_BRANCHES, | ||
| "mozilla-central": True, | ||
| } | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "release_branches,params,expected", | ||
| ( | ||
| # Not level 3 -> always staging, regardless of branch. | ||
| (RELEASE_BRANCHES, {"level": "1", "project": "mozilla-central"}, "staging"), | ||
| ( | ||
| RELEASE_BRANCHES, | ||
| {"level": "1", "project": "firefox", "head_ref": "refs/heads/beta"}, | ||
| "staging", | ||
| ), | ||
| # No `release-branches` mapping at all -> staging. | ||
| ( | ||
| None, | ||
| {"level": "3", "project": "firefox", "head_ref": "refs/heads/beta"}, | ||
| "staging", | ||
| ), | ||
| # Project not listed in the mapping -> staging (e.g. autoland). | ||
| (RELEASE_BRANCHES, {"level": "3", "project": "autoland"}, "staging"), | ||
| # Whole project is a release project (Mercurial model). | ||
| (RELEASE_BRANCHES, {"level": "3", "project": "mozilla-central"}, "production"), | ||
| # Git monorepo model: only listed branches are production. | ||
| ( | ||
| RELEASE_BRANCHES, | ||
| {"level": "3", "project": "firefox", "head_ref": "refs/heads/beta"}, | ||
| "production", | ||
| ), | ||
| ( | ||
| RELEASE_BRANCHES, | ||
| {"level": "3", "project": "firefox", "head_ref": "refs/heads/test"}, | ||
| "staging", | ||
| ), | ||
| # Only refs/heads/* match, not tags. | ||
| ( | ||
| RELEASE_BRANCHES, | ||
| {"level": "3", "project": "firefox", "head_ref": "refs/tags/beta"}, | ||
| "staging", | ||
| ), | ||
| ), | ||
| ) | ||
| def test_release_level(release_branches, params, expected): | ||
| assert release_level(release_branches, params) == expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than allow
release_branchesto be something other than a dict, let's just assume it is, and require callers to make that happen. (And maybe add a type annotation to help any callers that run linters?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that sounds good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the feedback. The latest commit should address these points.