From 93e477595d36cbffe7fa375586226a000b7ea30e Mon Sep 17 00:00:00 2001 From: Nikhil Manglore Date: Wed, 5 Aug 2026 21:51:12 +0000 Subject: [PATCH 1/3] Fix update-versions.py script to handle certain cases and add tests Signed-off-by: Nikhil Manglore --- scripts/update-versions.py | 69 ++++++++-------- tests/test_update_versions.py | 146 +++++++++++++++++++++------------- 2 files changed, 130 insertions(+), 85 deletions(-) diff --git a/scripts/update-versions.py b/scripts/update-versions.py index e7c214a..28011ba 100755 --- a/scripts/update-versions.py +++ b/scripts/update-versions.py @@ -5,7 +5,7 @@ import re import logging import subprocess -from typing import Dict, Any +from typing import Dict, Any, Optional logging.basicConfig(level=logging.INFO, format="%(message)s") @@ -71,15 +71,19 @@ def get_latest_module_release(repository: str, include_rc: bool = True) -> str: tags.sort(key=lambda v: (parse_version(v)[:3], parse_version(v)[3] or float('inf'))) return tags[-1] -def bump_bundle_if_needed(versions_data: Dict[str, Any], block: str) -> None: - """Bump the bundle version for a block if it hasn't already been incremented vs mainline.""" - current = versions_data[block]["version"] +def get_mainline_bundle_version(block: str) -> Optional[str]: + """Return the bundle version for a block on origin/mainline, or None if unavailable.""" try: result = subprocess.check_output(['git', 'show', 'origin/mainline:versions.json'], text=True) - mainline = json.loads(result)[block]["version"] + return json.loads(result).get(block, {}).get("version") except (subprocess.CalledProcessError, KeyError): - mainline = None - + return None + +def bump_bundle_if_needed(versions_data: Dict[str, Any], block: str) -> None: + """Bump the bundle version for a block if it hasn't already been incremented vs mainline.""" + current = versions_data[block]["version"] + mainline = get_mainline_bundle_version(block) + if mainline is not None and mainline != current: logging.info(f"Bundle version {block} already incremented from {mainline} to {current}") else: @@ -115,25 +119,25 @@ def update_versions(versions_data: Dict[str, Any], component_name: str, new_vers stable_version = get_latest_module_release(repository, include_rc=False) versions_data[new_major_minor_release]["modules"][name]["version"] = stable_version - # For backported valkey releases, always increment bundle version - if new_major_minor_release != latest: + # Bump the bundle version only if it hasn't already been incremented + mainline_bundle_version = get_mainline_bundle_version(new_major_minor_release) + if mainline_bundle_version is not None and mainline_bundle_version != existing_bundle_version: + logging.info(f"Valkey Bundle version {new_major_minor_release} already incremented from {mainline_bundle_version} to {existing_bundle_version}") + elif new_major_minor_release != latest: + # Backported valkey release versions_data[new_major_minor_release]["version"] = bump_version(existing_bundle_version) - logging.info(f"Updated backported bundle version from {existing_bundle_version} to {versions_data[new_major_minor_release]['version']}") + logging.info(f"Updated backported Valkey Bundle version from {existing_bundle_version} to {versions_data[new_major_minor_release]['version']}") else: - try: - subprocess.check_output(["git", "ls-remote", "--exit-code", "--heads", "origin", "valkey-bundle-update"], stderr=subprocess.DEVNULL) - logging.info("There is an open PR for the branch valkey-bundle-update - bundle patch version won't be bumped.") - except subprocess.CalledProcessError: - bundle_major, bundle_minor, bundle_patch, bundle_rc = parse_version(existing_bundle_version) - - if rc is not None or bundle_rc is not None: - if rc is not None: - versions_data[new_major_minor_release]["version"] = f"{bundle_major}.{bundle_minor}.{bundle_patch}-rc{bundle_rc + 1}" - else: - versions_data[new_major_minor_release]["version"] = f"{bundle_major}.{bundle_minor}.{bundle_patch}" + bundle_major, bundle_minor, bundle_patch, bundle_rc = parse_version(existing_bundle_version) + + if rc is not None or bundle_rc is not None: + if rc is not None: + versions_data[new_major_minor_release]["version"] = f"{bundle_major}.{bundle_minor}.{bundle_patch}-rc{(bundle_rc or 0) + 1}" else: - versions_data[new_major_minor_release]["version"] = bump_version(existing_bundle_version) - logging.info("There is no open PR for the branch valkey-bundle-update — bumping bundle patch version.") + versions_data[new_major_minor_release]["version"] = f"{bundle_major}.{bundle_minor}.{bundle_patch}" + else: + versions_data[new_major_minor_release]["version"] = bump_version(existing_bundle_version) + logging.info(f"Bumped Valkey Bundle version {new_major_minor_release} from {existing_bundle_version} to {versions_data[new_major_minor_release]['version']}") else: # New major/minor version known_modules = get_known_modules_from_versions(versions_data) @@ -179,6 +183,8 @@ def update_versions(versions_data: Dict[str, Any], component_name: str, new_vers ) sys.exit(0) + module_updated = False + if patch > 0: # For patch releases we will update all version entries with the same major.minor version as the module patch we just released for version_block in versions_data.keys(): @@ -187,23 +193,24 @@ def update_versions(versions_data: Dict[str, Any], component_name: str, new_vers current_module_version = versions_data[version_block]["modules"][module_key]["version"] current_major, current_minor, _, _ = parse_version(current_module_version) current_major_minor = f"{current_major}.{current_minor}" - + if current_major_minor == new_major_minor_release: versions_data[version_block]["modules"][module_key]["version"] = new_version logging.info(f"Patch release: Updated {module_key} to {new_version} in Bundle version {version_block}") - - if version_block != latest: + + if version_block == latest: + module_updated = True + else: bump_bundle_if_needed(versions_data, version_block) else: # For major or minor releases we will only update latest version entry versions_data[latest]["modules"][module_key] = {"version": new_version} + module_updated = True - try: - subprocess.check_output(["git", "ls-remote", "--exit-code", "--heads", "origin", "valkey-bundle-update"], stderr=subprocess.DEVNULL) - logging.info("There is an open PR for the branch valkey-bundle-update - bundle patch version won't be bumped.") - except subprocess.CalledProcessError: + # Only bump the latest bundle if the module for the latest block actually changed. + if module_updated: bump_bundle_if_needed(versions_data, latest) - + return versions_data if __name__ == "__main__": diff --git a/tests/test_update_versions.py b/tests/test_update_versions.py index 046441f..548cc56 100644 --- a/tests/test_update_versions.py +++ b/tests/test_update_versions.py @@ -81,32 +81,39 @@ def test_repo_names(self, versions_data): # update_versions — valkey component # --------------------------------------------------------------------------- class TestUpdateVersionsValkey: - def _no_open_pr(self, mocker): - """Simulate no open PR branch (git ls-remote fails).""" - mocker.patch("subprocess.check_output", side_effect=subprocess.CalledProcessError(1, "git")) + def _mainline_matches_current(self, mocker): + """Simulate origin/mainline having the same bundle versions as current (no prior bump).""" + mocker.patch.object(update_versions, "get_mainline_bundle_version", side_effect=lambda block: None) - def _open_pr_exists(self, mocker): - """Simulate open PR branch exists.""" - mocker.patch("subprocess.check_output", return_value=b"abc123\trefs/heads/valkey-bundle-update\n") + def _mainline_bundle_lower_than_current(self, mocker, block, mainline_version): + """Simulate a prior bump: origin/mainline has a lower bundle version than current for the block.""" + mocker.patch.object( + update_versions, + "get_mainline_bundle_version", + side_effect=lambda b: mainline_version if b == block else None, + ) def test_patch_update_bumps_server_version(self, versions_data, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") - self._no_open_pr(mocker) + self._mainline_matches_current(mocker) result = update_versions_fn(versions_data, "valkey", "9.0.5") assert result["9.0"]["valkey-server"]["version"] == "9.0.5" - def test_patch_update_bumps_bundle_when_no_pr(self, versions_data, mocker): + def test_patch_update_bumps_bundle_when_not_already_bumped(self, versions_data, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") - self._no_open_pr(mocker) + self._mainline_matches_current(mocker) result = update_versions_fn(versions_data, "valkey", "9.0.5") # Original bundle was 9.0.1, should become 9.0.2 assert result["9.0"]["version"] == "9.0.2" - def test_patch_update_no_bundle_bump_when_pr_exists(self, versions_data, mocker): + def test_patch_update_no_bundle_bump_when_already_ahead_of_mainline(self, versions_data, mocker): + """When the current branch already carries a bumped bundle version (differs from + mainline), a subsequent valkey update on the same branch should not double-bump.""" mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") - self._open_pr_exists(mocker) + # Simulate mainline has 9.0.0 while current is 9.0.1 (already bumped) + self._mainline_bundle_lower_than_current(mocker, "9.0", "9.0.0") original_bundle = versions_data["9.0"]["version"] result = update_versions_fn(versions_data, "valkey", "9.0.5") @@ -114,7 +121,7 @@ def test_patch_update_no_bundle_bump_when_pr_exists(self, versions_data, mocker) def test_rc_update_bumps_rc_number(self, versions_data_rc, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") - self._no_open_pr(mocker) + self._mainline_matches_current(mocker) # Bundle is 9.0.1-rc2, new valkey is RC so should become 9.0.1-rc3 result = update_versions_fn(versions_data_rc, "valkey", "9.0.3-rc1") @@ -122,7 +129,7 @@ def test_rc_update_bumps_rc_number(self, versions_data_rc, mocker): def test_stable_after_rc_drops_rc(self, versions_data_rc, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") - self._no_open_pr(mocker) + self._mainline_matches_current(mocker) # Bundle is 9.0.1-rc2, new valkey is stable so bundle should drop RC result = update_versions_fn(versions_data_rc, "valkey", "9.0.3") @@ -133,6 +140,7 @@ def test_new_major_minor_creates_entry(self, versions_data, mocker): mocker.patch.object( update_versions, "get_latest_module_release", return_value="2.0.0" ) + self._mainline_matches_current(mocker) result = update_versions_fn(versions_data, "valkey", "10.0.0") assert "10.0" in result @@ -144,6 +152,7 @@ def test_new_major_minor_creates_entry(self, versions_data, mocker): def test_new_major_minor_rc_creates_entry(self, versions_data, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="trixie") mocker.patch.object(update_versions, "get_latest_module_release", return_value="2.0.0-rc1") + self._mainline_matches_current(mocker) result = update_versions_fn(versions_data, "valkey", "10.0.0-rc1") assert "10.0" in result @@ -152,14 +161,38 @@ def test_new_major_minor_rc_creates_entry(self, versions_data, mocker): for mod in ["valkey-json", "valkey-bloom", "valkey-search", "valkey-ldap"]: assert result["10.0"]["modules"][mod]["version"] == "2.0.0-rc1" - def test_backported_valkey_always_bumps_bundle(self, versions_data, mocker): + def test_backported_valkey_bumps_bundle_when_not_already_bumped(self, versions_data, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") + self._mainline_matches_current(mocker) result = update_versions_fn(versions_data, "valkey", "8.1.5") assert result["8.1"]["valkey-server"]["version"] == "8.1.5" assert result["8.1"]["version"] == "8.1.3" # 8.1.2 -> 8.1.3 + def test_backported_valkey_no_double_bump_when_already_ahead(self, versions_data, mocker): + """Running the same backported valkey update twice should not double-bump the bundle.""" + mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") + # Simulate mainline has 8.1.1 while current has 8.1.2 (already bumped in a prior run) + self._mainline_bundle_lower_than_current(mocker, "8.1", "8.1.1") + + result = update_versions_fn(versions_data, "valkey", "8.1.5") + assert result["8.1"]["valkey-server"]["version"] == "8.1.5" + assert result["8.1"]["version"] == "8.1.2" # unchanged + + def test_valkey_update_bumps_bundle_when_mainline_matches_current(self, versions_data, mocker): + """Regression for issue #121: when the current branch has the same bundle version as + mainline (i.e. no prior bump on this branch), a valkey-server update must bump the bundle.""" + mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") + # Mainline and current both at 9.0.1 — this is the state right after a fresh checkout + self._mainline_bundle_lower_than_current(mocker, "9.0", "9.0.1") + + result = update_versions_fn(versions_data, "valkey", "9.0.5") + assert result["9.0"]["valkey-server"]["version"] == "9.0.5" + # Bundle must bump because mainline == current (no prior bump on this branch) + assert result["9.0"]["version"] == "9.0.2" + def test_backported_valkey_does_not_touch_latest(self, versions_data, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") + self._mainline_matches_current(mocker) original_latest = copy.deepcopy(versions_data["9.0"]) update_versions_fn(versions_data, "valkey", "8.1.5") assert versions_data["9.0"] == original_latest @@ -175,8 +208,7 @@ def test_ga_downgrades_rc_modules_to_stable(self, versions_data_rc_latest, mocke "valkey-io/valkey-ldap": "1.0.0", }[repo], ) - # Simulate no open PR - mocker.patch("subprocess.check_output", side_effect=subprocess.CalledProcessError(1, "git")) + self._mainline_matches_current(mocker) # 9.0 has valkey-server 9.0.0-rc1, search 1.1.0-rc1, ldap 1.1.0-rc1 result = update_versions_fn(versions_data_rc_latest, "valkey", "9.0.0") @@ -190,7 +222,7 @@ def test_ga_downgrades_rc_modules_to_stable(self, versions_data_rc_latest, mocke def test_ga_does_not_downgrade_when_no_rc_modules(self, versions_data, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") - mocker.patch("subprocess.check_output", side_effect=subprocess.CalledProcessError(1, "git")) + self._mainline_matches_current(mocker) # Set valkey-server to RC so the GA path triggers, but all modules are stable versions_data["9.0"]["valkey-server"]["version"] = "9.0.0-rc1" versions_data["9.0"]["version"] = "9.0.0-rc1" @@ -201,6 +233,7 @@ def test_ga_does_not_downgrade_when_no_rc_modules(self, versions_data, mocker): def test_ga_downgrade_only_on_latest_block(self, versions_data_three_blocks, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") + self._mainline_matches_current(mocker) # 9.1 is latest, 8.1 has valkey-server set to RC for this test versions_data_three_blocks["8.1"]["valkey-server"]["version"] = "8.1.0-rc1" versions_data_three_blocks["8.1"]["modules"]["valkey-search"]["version"] = "1.1.0-rc1" @@ -214,14 +247,20 @@ def test_ga_downgrade_only_on_latest_block(self, versions_data_three_blocks, moc # update_versions — module component # --------------------------------------------------------------------------- class TestUpdateVersionsModule: - def _no_open_pr(self, mocker): - mocker.patch("subprocess.check_output", side_effect=subprocess.CalledProcessError(1, "git")) + def _mainline_matches_current(self, mocker): + """Simulate origin/mainline having the same bundle versions as current (no prior bump).""" + mocker.patch.object(update_versions, "get_mainline_bundle_version", side_effect=lambda block: None) - def _open_pr_exists(self, mocker): - mocker.patch("subprocess.check_output", return_value=b"abc123\trefs/heads/valkey-bundle-update\n") + def _mainline_bundle_lower_than_current(self, mocker, block, mainline_version): + """Simulate a prior bump: origin/mainline has a lower bundle version than current for the block.""" + mocker.patch.object( + update_versions, + "get_mainline_bundle_version", + side_effect=lambda b: mainline_version if b == block else None, + ) def test_module_patch_updates_matching_blocks(self, versions_data, mocker): - self._no_open_pr(mocker) + self._mainline_matches_current(mocker) # valkey-json 1.0.1 exists in 8.1 and 9.0 — patch 1.0.2 should update both result = update_versions_fn(versions_data, "json", "1.0.2") @@ -229,13 +268,13 @@ def test_module_patch_updates_matching_blocks(self, versions_data, mocker): assert result["9.0"]["modules"]["valkey-json"]["version"] == "1.0.2" def test_module_patch_does_not_touch_unstable(self, versions_data, mocker): - self._no_open_pr(mocker) + self._mainline_matches_current(mocker) original_unstable = copy.deepcopy(versions_data["unstable"]) update_versions_fn(versions_data, "json", "1.0.2") assert versions_data["unstable"] == original_unstable def test_module_patch_updates_three_blocks(self, versions_data_three_blocks, mocker): - self._no_open_pr(mocker) + self._mainline_matches_current(mocker) # json is 1.0.1 in 8.1, 9.0, and 9.1 — patch should update all three result = update_versions_fn(versions_data_three_blocks, "json", "1.0.2") assert result["8.1"]["modules"]["valkey-json"]["version"] == "1.0.2" @@ -243,7 +282,7 @@ def test_module_patch_updates_three_blocks(self, versions_data_three_blocks, moc assert result["9.1"]["modules"]["valkey-json"]["version"] == "1.0.2" def test_module_patch_does_not_update_different_major_minor(self, versions_data, mocker): - self._no_open_pr(mocker) + self._mainline_matches_current(mocker) # valkey-search is 1.0.1 in both blocks. Releasing 2.0.1 should not match 1.0.x # First, set up a scenario: 8.1 has search 1.0.1, 9.0 has search 2.0.0 @@ -253,7 +292,7 @@ def test_module_patch_does_not_update_different_major_minor(self, versions_data, assert result["8.1"]["modules"]["valkey-search"]["version"] == "1.0.1" # unchanged def test_module_major_release_only_updates_latest(self, versions_data, mocker): - self._no_open_pr(mocker) + self._mainline_matches_current(mocker) # For major module release, valkey must be X.0.0 versions_data["9.0"]["valkey-server"]["version"] = "9.0.0" @@ -272,7 +311,7 @@ def test_module_minor_release_rejected_if_valkey_minor_is_zero(self, versions_da update_versions_fn(versions_data, "json", "1.1.0") def test_module_minor_release_allowed_when_valkey_minor_gt_zero(self, versions_data_three_blocks, mocker): - self._no_open_pr(mocker) + self._mainline_matches_current(mocker) # 9.1 is latest, valkey-server is 9.1.0-rc1 (minor=1), so module minor release should be allowed result = update_versions_fn(versions_data_three_blocks, "json", "1.1.0") assert result["9.1"]["modules"]["valkey-json"]["version"] == "1.1.0" @@ -281,20 +320,20 @@ def test_module_minor_release_allowed_when_valkey_minor_gt_zero(self, versions_d assert result["9.0"]["modules"]["valkey-json"]["version"] == "1.0.1" def test_module_major_release_allowed_with_rc_valkey(self, versions_data, mocker): - self._no_open_pr(mocker) + self._mainline_matches_current(mocker) versions_data["9.0"]["valkey-server"]["version"] = "9.0.0-rc1" result = update_versions_fn(versions_data, "json", "2.0.0") assert result["9.0"]["modules"]["valkey-json"]["version"] == "2.0.0" def test_module_bumps_bundle_patch_when_no_pr(self, versions_data, mocker): - self._no_open_pr(mocker) + self._mainline_matches_current(mocker) original_bundle = versions_data["9.0"]["version"] # "9.0.1" result = update_versions_fn(versions_data, "json", "1.0.2") assert result["9.0"]["version"] == "9.0.2" def test_module_bumps_rc_when_bundle_is_rc(self, versions_data_rc, mocker): - self._no_open_pr(mocker) + self._mainline_matches_current(mocker) # Bundle is 9.0.1-rc2, valkey-server is 9.0.2-rc1 # Module minor release: valkey_minor != 0 check — valkey is 9.0.x so minor=0 @@ -302,36 +341,35 @@ def test_module_bumps_rc_when_bundle_is_rc(self, versions_data_rc, mocker): result = update_versions_fn(versions_data_rc, "json", "1.0.2") assert result["9.0"]["version"] == "9.0.1-rc3" - def test_module_no_bundle_bump_when_pr_exists(self, versions_data, mocker): - def mock_check_output(*args, **kwargs): - cmd = args[0] if args else kwargs.get('args', []) - if cmd[0] == 'git' and 'ls-remote' in cmd: - return b"abc123\trefs/heads/valkey-bundle-update\n" - if cmd[0] == 'git' and 'show' in cmd: - return json.dumps(versions_data) - raise subprocess.CalledProcessError(1, "unknown") - - mocker.patch("subprocess.check_output", side_effect=mock_check_output) + def test_module_no_bundle_bump_when_already_ahead_of_mainline(self, versions_data, mocker): + """When the current branch already carries a bumped bundle version (differs from + mainline), a subsequent module update on the same branch should not double-bump.""" + # Simulate mainline has 9.0.0 while current is 9.0.1 (already bumped) + self._mainline_bundle_lower_than_current(mocker, "9.0", "9.0.0") original_bundle = versions_data["9.0"]["version"] result = update_versions_fn(versions_data, "json", "1.0.2") assert result["9.0"]["version"] == original_bundle + def test_module_patch_no_bump_when_no_block_matches(self, versions_data, mocker): + """Regression: dispatching a patch release whose major.minor line isn't present in + any block (e.g. search 1.1.1 when blocks are on 1.0.x and 1.2.x) must not bump the + latest bundle version.""" + self._mainline_matches_current(mocker) + # Set up: 8.1 and 9.0 have search on 1.0.x, and no block has search on 1.1.x + original = copy.deepcopy(versions_data) + result = update_versions_fn(versions_data, "search", "1.1.1") + # Nothing should have changed: no module versions, no bundle versions + assert result == original + def test_module_patch_dedup_non_latest_already_bumped(self, versions_data_three_blocks, mocker): """When a non-latest block's bundle was already bumped (differs from mainline), skip the bump.""" - def mock_check_output(*args, **kwargs): - cmd = args[0] if args else kwargs.get('args', []) - if cmd[0] == 'git' and 'ls-remote' in cmd: - raise subprocess.CalledProcessError(1, "git") - if cmd[0] == 'git' and 'show' in cmd: - # Simulate mainline has 8.1 bundle at 8.1.2 (same as current) - # but 9.0 bundle at 9.0.0 (different from current 9.0.1 — already bumped) - import json - mainline = copy.deepcopy(versions_data_three_blocks) - mainline["9.0"]["version"] = "9.0.0" - return json.dumps(mainline) - raise subprocess.CalledProcessError(1, "unknown") - - mocker.patch("subprocess.check_output", side_effect=mock_check_output) + # Mainline: 8.1 at 8.1.2 (same as current, will bump); 9.0 at 9.0.0 (differs from + # current 9.0.1, already bumped, will NOT bump). + mocker.patch.object( + update_versions, + "get_mainline_bundle_version", + side_effect=lambda block: {"8.1": "8.1.2", "9.0": "9.0.0"}.get(block), + ) result = update_versions_fn(versions_data_three_blocks, "json", "1.0.2") # 8.1 should bump (mainline matches current) From b0afd41031ef618f02c892496f12ac698b7cfbcc Mon Sep 17 00:00:00 2001 From: Nikhil Manglore Date: Wed, 5 Aug 2026 22:08:14 +0000 Subject: [PATCH 2/3] Resolve comments from code rabbit Signed-off-by: Nikhil Manglore --- scripts/update-versions.py | 16 ++++--- tests/test_update_versions.py | 84 ++++++++++++++++++++++++----------- 2 files changed, 69 insertions(+), 31 deletions(-) diff --git a/scripts/update-versions.py b/scripts/update-versions.py index 28011ba..36ec62b 100755 --- a/scripts/update-versions.py +++ b/scripts/update-versions.py @@ -72,12 +72,16 @@ def get_latest_module_release(repository: str, include_rc: bool = True) -> str: return tags[-1] def get_mainline_bundle_version(block: str) -> Optional[str]: - """Return the bundle version for a block on origin/mainline, or None if unavailable.""" - try: - result = subprocess.check_output(['git', 'show', 'origin/mainline:versions.json'], text=True) - return json.loads(result).get(block, {}).get("version") - except (subprocess.CalledProcessError, KeyError): - return None + """Return the bundle version for a block on origin/mainline, or None if the block + doesn't exist on mainline yet (new major.minor line first introduced on this branch). + + Git failures (e.g. missing origin/mainline ref, network errors) propagate as + CalledProcessError. We deliberately don't swallow them, because if we can't read + mainline we can't reason about whether this block has already been bumped on the + current branch, and silently proceeding could produce a double-bump. + """ + result = subprocess.check_output(['git', 'show', 'origin/mainline:versions.json'], text=True) + return json.loads(result).get(block, {}).get("version") def bump_bundle_if_needed(versions_data: Dict[str, Any], block: str) -> None: """Bump the bundle version for a block if it hasn't already been incremented vs mainline.""" diff --git a/tests/test_update_versions.py b/tests/test_update_versions.py index 548cc56..777031d 100644 --- a/tests/test_update_versions.py +++ b/tests/test_update_versions.py @@ -15,6 +15,32 @@ get_latest_major_minor = update_versions.get_latest_major_minor get_known_modules_from_versions = update_versions.get_known_modules_from_versions update_versions_fn = update_versions.update_versions +get_mainline_bundle_version = update_versions.get_mainline_bundle_version + + +# --------------------------------------------------------------------------- +# get_mainline_bundle_version +# --------------------------------------------------------------------------- +class TestGetMainlineBundleVersion: + def test_returns_version_when_block_exists(self, mocker): + mainline_data = {"9.0": {"version": "9.0.4"}, "8.1": {"version": "8.1.8"}} + mocker.patch("subprocess.check_output", return_value=json.dumps(mainline_data)) + assert get_mainline_bundle_version("9.0") == "9.0.4" + + def test_returns_none_when_block_missing(self, mocker): + """A brand-new major.minor line first introduced on the branch won't exist on mainline yet.""" + mainline_data = {"9.0": {"version": "9.0.4"}, "8.1": {"version": "8.1.8"}} + mocker.patch("subprocess.check_output", return_value=json.dumps(mainline_data)) + assert get_mainline_bundle_version("10.0") is None + + def test_raises_when_git_fails(self, mocker): + """Git failures propagate — callers must not silently proceed when mainline is unreadable.""" + mocker.patch( + "subprocess.check_output", + side_effect=subprocess.CalledProcessError(128, "git", stderr=b"fatal: bad revision"), + ) + with pytest.raises(subprocess.CalledProcessError): + get_mainline_bundle_version("9.0") # --------------------------------------------------------------------------- @@ -81,9 +107,13 @@ def test_repo_names(self, versions_data): # update_versions — valkey component # --------------------------------------------------------------------------- class TestUpdateVersionsValkey: - def _mainline_matches_current(self, mocker): + def _mainline_matches_current(self, mocker, versions_data): """Simulate origin/mainline having the same bundle versions as current (no prior bump).""" - mocker.patch.object(update_versions, "get_mainline_bundle_version", side_effect=lambda block: None) + mocker.patch.object( + update_versions, + "get_mainline_bundle_version", + side_effect=lambda block: versions_data.get(block, {}).get("version"), + ) def _mainline_bundle_lower_than_current(self, mocker, block, mainline_version): """Simulate a prior bump: origin/mainline has a lower bundle version than current for the block.""" @@ -95,14 +125,14 @@ def _mainline_bundle_lower_than_current(self, mocker, block, mainline_version): def test_patch_update_bumps_server_version(self, versions_data, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data) result = update_versions_fn(versions_data, "valkey", "9.0.5") assert result["9.0"]["valkey-server"]["version"] == "9.0.5" def test_patch_update_bumps_bundle_when_not_already_bumped(self, versions_data, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data) result = update_versions_fn(versions_data, "valkey", "9.0.5") # Original bundle was 9.0.1, should become 9.0.2 @@ -121,7 +151,7 @@ def test_patch_update_no_bundle_bump_when_already_ahead_of_mainline(self, versio def test_rc_update_bumps_rc_number(self, versions_data_rc, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data_rc) # Bundle is 9.0.1-rc2, new valkey is RC so should become 9.0.1-rc3 result = update_versions_fn(versions_data_rc, "valkey", "9.0.3-rc1") @@ -129,7 +159,7 @@ def test_rc_update_bumps_rc_number(self, versions_data_rc, mocker): def test_stable_after_rc_drops_rc(self, versions_data_rc, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data_rc) # Bundle is 9.0.1-rc2, new valkey is stable so bundle should drop RC result = update_versions_fn(versions_data_rc, "valkey", "9.0.3") @@ -140,7 +170,7 @@ def test_new_major_minor_creates_entry(self, versions_data, mocker): mocker.patch.object( update_versions, "get_latest_module_release", return_value="2.0.0" ) - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data) result = update_versions_fn(versions_data, "valkey", "10.0.0") assert "10.0" in result @@ -152,7 +182,7 @@ def test_new_major_minor_creates_entry(self, versions_data, mocker): def test_new_major_minor_rc_creates_entry(self, versions_data, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="trixie") mocker.patch.object(update_versions, "get_latest_module_release", return_value="2.0.0-rc1") - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data) result = update_versions_fn(versions_data, "valkey", "10.0.0-rc1") assert "10.0" in result @@ -163,7 +193,7 @@ def test_new_major_minor_rc_creates_entry(self, versions_data, mocker): def test_backported_valkey_bumps_bundle_when_not_already_bumped(self, versions_data, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data) result = update_versions_fn(versions_data, "valkey", "8.1.5") assert result["8.1"]["valkey-server"]["version"] == "8.1.5" assert result["8.1"]["version"] == "8.1.3" # 8.1.2 -> 8.1.3 @@ -192,7 +222,7 @@ def test_valkey_update_bumps_bundle_when_mainline_matches_current(self, versions def test_backported_valkey_does_not_touch_latest(self, versions_data, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data) original_latest = copy.deepcopy(versions_data["9.0"]) update_versions_fn(versions_data, "valkey", "8.1.5") assert versions_data["9.0"] == original_latest @@ -208,7 +238,7 @@ def test_ga_downgrades_rc_modules_to_stable(self, versions_data_rc_latest, mocke "valkey-io/valkey-ldap": "1.0.0", }[repo], ) - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data_rc_latest) # 9.0 has valkey-server 9.0.0-rc1, search 1.1.0-rc1, ldap 1.1.0-rc1 result = update_versions_fn(versions_data_rc_latest, "valkey", "9.0.0") @@ -222,7 +252,7 @@ def test_ga_downgrades_rc_modules_to_stable(self, versions_data_rc_latest, mocke def test_ga_does_not_downgrade_when_no_rc_modules(self, versions_data, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data) # Set valkey-server to RC so the GA path triggers, but all modules are stable versions_data["9.0"]["valkey-server"]["version"] = "9.0.0-rc1" versions_data["9.0"]["version"] = "9.0.0-rc1" @@ -233,7 +263,7 @@ def test_ga_does_not_downgrade_when_no_rc_modules(self, versions_data, mocker): def test_ga_downgrade_only_on_latest_block(self, versions_data_three_blocks, mocker): mocker.patch.object(update_versions, "get_debian_version", return_value="bookworm") - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data_three_blocks) # 9.1 is latest, 8.1 has valkey-server set to RC for this test versions_data_three_blocks["8.1"]["valkey-server"]["version"] = "8.1.0-rc1" versions_data_three_blocks["8.1"]["modules"]["valkey-search"]["version"] = "1.1.0-rc1" @@ -247,9 +277,13 @@ def test_ga_downgrade_only_on_latest_block(self, versions_data_three_blocks, moc # update_versions — module component # --------------------------------------------------------------------------- class TestUpdateVersionsModule: - def _mainline_matches_current(self, mocker): + def _mainline_matches_current(self, mocker, versions_data): """Simulate origin/mainline having the same bundle versions as current (no prior bump).""" - mocker.patch.object(update_versions, "get_mainline_bundle_version", side_effect=lambda block: None) + mocker.patch.object( + update_versions, + "get_mainline_bundle_version", + side_effect=lambda block: versions_data.get(block, {}).get("version"), + ) def _mainline_bundle_lower_than_current(self, mocker, block, mainline_version): """Simulate a prior bump: origin/mainline has a lower bundle version than current for the block.""" @@ -260,7 +294,7 @@ def _mainline_bundle_lower_than_current(self, mocker, block, mainline_version): ) def test_module_patch_updates_matching_blocks(self, versions_data, mocker): - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data) # valkey-json 1.0.1 exists in 8.1 and 9.0 — patch 1.0.2 should update both result = update_versions_fn(versions_data, "json", "1.0.2") @@ -268,13 +302,13 @@ def test_module_patch_updates_matching_blocks(self, versions_data, mocker): assert result["9.0"]["modules"]["valkey-json"]["version"] == "1.0.2" def test_module_patch_does_not_touch_unstable(self, versions_data, mocker): - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data) original_unstable = copy.deepcopy(versions_data["unstable"]) update_versions_fn(versions_data, "json", "1.0.2") assert versions_data["unstable"] == original_unstable def test_module_patch_updates_three_blocks(self, versions_data_three_blocks, mocker): - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data_three_blocks) # json is 1.0.1 in 8.1, 9.0, and 9.1 — patch should update all three result = update_versions_fn(versions_data_three_blocks, "json", "1.0.2") assert result["8.1"]["modules"]["valkey-json"]["version"] == "1.0.2" @@ -282,7 +316,7 @@ def test_module_patch_updates_three_blocks(self, versions_data_three_blocks, moc assert result["9.1"]["modules"]["valkey-json"]["version"] == "1.0.2" def test_module_patch_does_not_update_different_major_minor(self, versions_data, mocker): - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data) # valkey-search is 1.0.1 in both blocks. Releasing 2.0.1 should not match 1.0.x # First, set up a scenario: 8.1 has search 1.0.1, 9.0 has search 2.0.0 @@ -292,7 +326,7 @@ def test_module_patch_does_not_update_different_major_minor(self, versions_data, assert result["8.1"]["modules"]["valkey-search"]["version"] == "1.0.1" # unchanged def test_module_major_release_only_updates_latest(self, versions_data, mocker): - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data) # For major module release, valkey must be X.0.0 versions_data["9.0"]["valkey-server"]["version"] = "9.0.0" @@ -311,7 +345,7 @@ def test_module_minor_release_rejected_if_valkey_minor_is_zero(self, versions_da update_versions_fn(versions_data, "json", "1.1.0") def test_module_minor_release_allowed_when_valkey_minor_gt_zero(self, versions_data_three_blocks, mocker): - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data_three_blocks) # 9.1 is latest, valkey-server is 9.1.0-rc1 (minor=1), so module minor release should be allowed result = update_versions_fn(versions_data_three_blocks, "json", "1.1.0") assert result["9.1"]["modules"]["valkey-json"]["version"] == "1.1.0" @@ -320,20 +354,20 @@ def test_module_minor_release_allowed_when_valkey_minor_gt_zero(self, versions_d assert result["9.0"]["modules"]["valkey-json"]["version"] == "1.0.1" def test_module_major_release_allowed_with_rc_valkey(self, versions_data, mocker): - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data) versions_data["9.0"]["valkey-server"]["version"] = "9.0.0-rc1" result = update_versions_fn(versions_data, "json", "2.0.0") assert result["9.0"]["modules"]["valkey-json"]["version"] == "2.0.0" def test_module_bumps_bundle_patch_when_no_pr(self, versions_data, mocker): - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data) original_bundle = versions_data["9.0"]["version"] # "9.0.1" result = update_versions_fn(versions_data, "json", "1.0.2") assert result["9.0"]["version"] == "9.0.2" def test_module_bumps_rc_when_bundle_is_rc(self, versions_data_rc, mocker): - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data_rc) # Bundle is 9.0.1-rc2, valkey-server is 9.0.2-rc1 # Module minor release: valkey_minor != 0 check — valkey is 9.0.x so minor=0 @@ -354,7 +388,7 @@ def test_module_patch_no_bump_when_no_block_matches(self, versions_data, mocker) """Regression: dispatching a patch release whose major.minor line isn't present in any block (e.g. search 1.1.1 when blocks are on 1.0.x and 1.2.x) must not bump the latest bundle version.""" - self._mainline_matches_current(mocker) + self._mainline_matches_current(mocker, versions_data) # Set up: 8.1 and 9.0 have search on 1.0.x, and no block has search on 1.1.x original = copy.deepcopy(versions_data) result = update_versions_fn(versions_data, "search", "1.1.1") From 92b4b00fc27369225069a532409bbf55a76d8a4a Mon Sep 17 00:00:00 2001 From: Nikhil Manglore Date: Wed, 5 Aug 2026 22:17:47 +0000 Subject: [PATCH 3/3] Update comment Signed-off-by: Nikhil Manglore --- scripts/update-versions.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/scripts/update-versions.py b/scripts/update-versions.py index 36ec62b..1f27ecc 100755 --- a/scripts/update-versions.py +++ b/scripts/update-versions.py @@ -72,14 +72,7 @@ def get_latest_module_release(repository: str, include_rc: bool = True) -> str: return tags[-1] def get_mainline_bundle_version(block: str) -> Optional[str]: - """Return the bundle version for a block on origin/mainline, or None if the block - doesn't exist on mainline yet (new major.minor line first introduced on this branch). - - Git failures (e.g. missing origin/mainline ref, network errors) propagate as - CalledProcessError. We deliberately don't swallow them, because if we can't read - mainline we can't reason about whether this block has already been bumped on the - current branch, and silently proceeding could produce a double-bump. - """ + """Return the bundle version for a block on origin/mainline.""" result = subprocess.check_output(['git', 'show', 'origin/mainline:versions.json'], text=True) return json.loads(result).get(block, {}).get("version")