-
Notifications
You must be signed in to change notification settings - Fork 38
Bug 2025695 - landoscript: extend version_bump to handle JSON manifests #1413
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
base: master
Are you sure you want to change the base?
Changes from all 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 | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,3 +1,4 @@ | ||||||||||||||||
| import json | ||||||||||||||||
| import logging | ||||||||||||||||
| import os.path | ||||||||||||||||
| import typing | ||||||||||||||||
|
|
@@ -17,10 +18,11 @@ | |||||||||||||||
|
|
||||||||||||||||
| log = logging.getLogger(__name__) | ||||||||||||||||
|
|
||||||||||||||||
| # A list of files that this action is allowed to operate on. | ||||||||||||||||
| ALLOWED_BUMP_FILES = ( | ||||||||||||||||
| "browser/config/version.txt", | ||||||||||||||||
| "browser/config/version_display.txt", | ||||||||||||||||
| "browser/extensions/newtab/manifest.json", | ||||||||||||||||
| "browser/extensions/webcompat/manifest.json", | ||||||||||||||||
| "config/milestone.txt", | ||||||||||||||||
| "mobile/android/version.txt", | ||||||||||||||||
| "mail/config/version.txt", | ||||||||||||||||
|
|
@@ -34,6 +36,12 @@ class VersionBumpInfo: | |||||||||||||||
| files: list[str] | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| def parse_manifest_version(orig_contents: str) -> BaseVersion: | ||||||||||||||||
|
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. If we're not using a parser to write there's no good reason to use it to read either IMO. If we care about the risk of updating the wrong substring in the file the previously reviewed version of this is preferable. If we're using simple string replacement, this is unnecessary.
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'm sorry, I'm not sure I follow. The extra layer here just extracts the content of
I'm not sure what I should change here. Could you expand on it @bhearsum?
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. You're right, my bad. This is slightly different and more involved than what we do currently, but not categorically different. |
||||||||||||||||
| """Extract and parse the version field from a JSON manifest.""" | ||||||||||||||||
| manifest = json.loads(orig_contents) | ||||||||||||||||
| return BaseVersion.parse(manifest["version"]) | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| async def run( | ||||||||||||||||
| github_client: GithubClient, | ||||||||||||||||
| public_artifact_dir: str, | ||||||||||||||||
|
|
@@ -87,7 +95,8 @@ async def run( | |||||||||||||||
|
|
||||||||||||||||
| modified = orig.replace(str(cur), str(next_)) | ||||||||||||||||
| if orig == modified: | ||||||||||||||||
| raise LandoscriptError("file not modified, this should be impossible") | ||||||||||||||||
| log.warning(f"{file}: version replacement produced no change, skipping") | ||||||||||||||||
| continue | ||||||||||||||||
|
|
||||||||||||||||
| log.info(f"{file}: successfully bumped! new contents are:") | ||||||||||||||||
| log_file_contents(modified) | ||||||||||||||||
|
|
@@ -119,6 +128,11 @@ def extract_path(diff_text): | |||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| def get_cur_and_next_version(filename, orig_contents, next_version, munge_next_version): | ||||||||||||||||
| if filename.endswith(".json"): | ||||||||||||||||
| cur = parse_manifest_version(orig_contents) | ||||||||||||||||
|
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. I might suggest a more detailed check on what we're parsing here rather than just looking for ".json", but this is also fine for now. |
||||||||||||||||
| next_ = BaseVersion.parse(next_version) | ||||||||||||||||
| return cur, next_ | ||||||||||||||||
|
|
||||||||||||||||
| VersionClass: BaseVersion = find_what_version_parser_to_use(filename) | ||||||||||||||||
| lines = [line for line in orig_contents.splitlines() if line and not line.startswith("#")] | ||||||||||||||||
| cur = VersionClass.parse(lines[-1]) | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import json | ||
|
|
||
| import pytest | ||
|
|
||
| from landoscript.actions.version_bump import parse_manifest_version | ||
|
|
||
|
|
||
| def _manifest(version, name="New Tab"): | ||
| return json.dumps({"manifest_version": 2, "name": name, "version": version}, indent=2) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "version", | ||
| ["151.0.0", "151.1.0", "151.2.0"], | ||
| ) | ||
| def test_parse_manifest_version(version): | ||
| result = parse_manifest_version(_manifest(version)) | ||
| assert str(result) == version | ||
|
|
||
|
|
||
| def test_parse_manifest_version_ignores_other_fields(): | ||
| contents = '{"manifest_version": 2, "version": "151.0.0", "strict_min_version": "140.0"}' | ||
| result = parse_manifest_version(contents) | ||
| assert str(result) == "151.0.0" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "manifest_name", | ||
| ["New Tab", "Web Compat"], | ||
| ) | ||
| def test_parse_manifest_version_works_for_newtab_and_webcompat(manifest_name): | ||
| result = parse_manifest_version(_manifest("151.0.0", name=manifest_name)) | ||
| assert str(result) == "151.0.0" |
Uh oh!
There was an error while loading. Please reload this page.