Bug 2025695 - landoscript: extend version_bump to handle JSON manifests#1413
Bug 2025695 - landoscript: extend version_bump to handle JSON manifests#1413JohanLorenzo wants to merge 2 commits intomozilla-releng:masterfrom
Conversation
b4333f5 to
42863d0
Compare
|
I don't love that this is an entirely new ad-hoc action. Couldn't we re-use the existing version_bump and tag actions? |
42863d0 to
0a9cd84
Compare
3108822 to
018c4d5
Compare
Discussed over Element. We agreed on reusing |
b48547a to
b09ba88
Compare
bhearsum
left a comment
There was a problem hiding this comment.
I don't necessarily disagree that it's good to have this in the same file, but I can't help but notice that at least the current implementation has two notable departures from the main code path (a special way to create "next" versions and a special way to do the replacement). This is likely to lead to maintenance and comprehensibility problems later, especially if/when we add more cases like this.
Bubbling these sorts of branches up as high as possible would be one way to improve the situation. Rearchitecturing/refactoring to generalize the idea of filename-specific bumping methods might be another, eg: perhaps associating filenames with bump version generation and bump methods.
jcristau
left a comment
There was a problem hiding this comment.
Agree with Ben on the extra code seeming unnecessary.
b09ba88 to
88d3ad4
Compare
88d3ad4 to
f63d8d7
Compare
|
Thanks for the reviews! How does this simpler approach look? |
… 2025695) Skip silently rather than raising if str.replace produces no change, so retried tasks don't fail spuriously if the bump already landed.
f63d8d7 to
a6a7bf1
Compare
| files: list[str] | ||
|
|
||
|
|
||
| def parse_manifest_version(orig_contents: str) -> BaseVersion: |
There was a problem hiding this comment.
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.
Bug 2025695
Summary
version_bumpto handle JSON manifests: addsbrowser/extensions/newtab/manifest.jsontoALLOWED_BUMP_FILES, makesnext_versionoptional for JSON files (auto-computed by bumping minor), and usesjson.loads/json.dumpsfor replacementJSON_MANIFEST_BUMP_FILESfrozenset to gate JSON-specific code paths to explicitly listed files onlyparse_manifest_version()andapply_manifest_version_bump()as independently unit-testable pure functions (intest_version_bump_json_manifest.py)Merge order
dev-landoscript(done), then merge to masterVerification
uv run pytestinlandoscript/)manifest.jsonfromstaging-firefox, bumped150.1.0 → 150.2.0, submitted to staging Lando, gotLANDEDstatusxpi-1-lando-devworker, landing this commit on staging-firefox: