From f63ce573dbdd9c0c003b41736c6f0a74c0676efe Mon Sep 17 00:00:00 2001 From: Richard Levasseur Date: Tue, 30 Jun 2026 02:14:11 +0000 Subject: [PATCH] Fix release prep tool to reuse existing PRs and avoid early exit. The tool now checks for open branch PRs or PRs associated with the tracking issue before creating a new one, and no longer exits early when there are no local changes to commit. --- tests/tools/private/release/release_test.py | 95 +++++++++++++++++++++ tools/private/release/gh.py | 16 ++++ tools/private/release/prepare.py | 50 ++++++++--- tools/private/release/release.py | 67 +-------------- tools/private/release/release_issue.py | 66 ++++++++++++++ 5 files changed, 215 insertions(+), 79 deletions(-) diff --git a/tests/tools/private/release/release_test.py b/tests/tools/private/release/release_test.py index 534aa2a5fd..a1467d8987 100644 --- a/tests/tools/private/release/release_test.py +++ b/tests/tools/private/release/release_test.py @@ -35,6 +35,7 @@ def _mock_git_and_gh(test_case): mock_git.branch_exists.return_value = False mock_git.tag_exists.return_value = False mock_gh.get_release_tracking_issue.side_effect = NoTrackingIssueError("Not found") + mock_gh.get_open_pr.return_value = None class TempDirTestCase(unittest.TestCase): @@ -799,6 +800,100 @@ def test_prepare_dry_run(self, mock_replace, mock_changelog): self.mock_gh.get_release_tracking_issue.assert_called_once_with("2.0.0") self.mock_git.add_modified_and_deleted.assert_not_called() + @patch("tools.private.release.prepare.changelog_news") + @patch("tools.private.release.prepare.replace_version_next") + def test_prepare_use_associated_pr_from_tracking_issue( + self, mock_replace, mock_changelog + ): + # Arrange + args = MagicMock(version="2.0.0", issue=None, dry_run=False) + self.mock_git.status.side_effect = ["", ""] + self.mock_git.branch_exists.return_value = True + self.mock_gh.get_release_tracking_issue.side_effect = None + self.mock_gh.get_release_tracking_issue.return_value = 123 + self.mock_gh.get_open_pr.return_value = None + # PR #456 is already associated in the tracking issue + self.mock_gh.get_issue_body.return_value = ( + "- [ ] Prepare Release | status=pending pr=#456" + ) + + # Act + result = releaser.cmd_prepare(args) + + # Assert + self.assertEqual(result, 0) + self.mock_git.checkout.assert_called_once_with("prepare-2.0.0") + self.mock_git.commit.assert_not_called() + self.mock_git.push.assert_called_once_with( + "origin", "prepare-2.0.0", set_upstream=True + ) + self.mock_gh.get_open_pr.assert_called_once_with("prepare-2.0.0") + self.mock_gh.create_pr.assert_not_called() # Should NOT create a new PR + self.mock_gh.update_issue_body.assert_called_once() + call_args = self.mock_gh.update_issue_body.call_args[0] + self.assertIn("pr=#456", call_args[1]) + + @patch("tools.private.release.prepare.changelog_news") + @patch("tools.private.release.prepare.replace_version_next") + def test_prepare_create_pr_when_none_associated(self, mock_replace, mock_changelog): + # Arrange + args = MagicMock(version="2.0.0", issue=None, dry_run=False) + self.mock_git.status.side_effect = ["", ""] + self.mock_git.branch_exists.return_value = True + self.mock_gh.get_release_tracking_issue.side_effect = None + self.mock_gh.get_release_tracking_issue.return_value = 123 + self.mock_gh.get_open_pr.return_value = None + # No PR associated in the tracking issue + self.mock_gh.get_issue_body.return_value = "- [ ] Prepare Release" + self.mock_gh.create_pr.return_value = "https://github.com/foo/bar/pull/789" + + # Act + result = releaser.cmd_prepare(args) + + # Assert + self.assertEqual(result, 0) + self.mock_git.checkout.assert_called_once_with("prepare-2.0.0") + self.mock_git.commit.assert_not_called() + self.mock_git.push.assert_called_once_with( + "origin", "prepare-2.0.0", set_upstream=True + ) + self.mock_gh.get_open_pr.assert_called_once_with("prepare-2.0.0") + self.mock_gh.create_pr.assert_called_once_with("2.0.0", 123) + self.mock_gh.update_issue_body.assert_called_once() + call_args = self.mock_gh.update_issue_body.call_args[0] + self.assertIn("pr=#789", call_args[1]) + + @patch("tools.private.release.prepare.changelog_news") + @patch("tools.private.release.prepare.replace_version_next") + def test_prepare_reuse_existing_pr(self, mock_replace, mock_changelog): + # Arrange + args = MagicMock(version="2.0.0", issue=None, dry_run=False) + self.mock_git.status.side_effect = ["", ""] + self.mock_git.branch_exists.return_value = True + self.mock_gh.get_release_tracking_issue.side_effect = None + self.mock_gh.get_release_tracking_issue.return_value = 123 + self.mock_gh.get_open_pr.return_value = { + "number": 456, + "url": "https://github.com/foo/bar/pull/456", + } + self.mock_gh.get_issue_body.return_value = "- [ ] Prepare Release" + + # Act + result = releaser.cmd_prepare(args) + + # Assert + self.assertEqual(result, 0) + self.mock_git.checkout.assert_called_once_with("prepare-2.0.0") + self.mock_git.commit.assert_not_called() + self.mock_git.push.assert_called_once_with( + "origin", "prepare-2.0.0", set_upstream=True + ) + self.mock_gh.get_open_pr.assert_called_once_with("prepare-2.0.0") + self.mock_gh.create_pr.assert_not_called() + self.mock_gh.update_issue_body.assert_called_once() + call_args = self.mock_gh.update_issue_body.call_args[0] + self.assertIn("pr=#456", call_args[1]) + @patch("tools.private.release.prepare.changelog_news") @patch("tools.private.release.prepare.replace_version_next") def test_prepare_dry_run_no_issue(self, mock_replace, mock_changelog): diff --git a/tools/private/release/gh.py b/tools/private/release/gh.py index f37d87f396..da86415650 100644 --- a/tools/private/release/gh.py +++ b/tools/private/release/gh.py @@ -162,6 +162,22 @@ def create_pr(version, issue_num): ) +def get_open_pr(branch_name): + """Returns PR info if an open PR exists for the given branch, else None.""" + cmd = [ + "gh", + "pr", + "list", + f"--repo={_REPO}", + f"--head={branch_name}", + "--state=open", + "--json=number,url", + ] + output = run_cmd(*cmd) + prs = json.loads(output) if output else [] + return prs[0] if prs else None + + def get_pr_info(pr_num): """Gets information about a PR, including state, merge commit, and body.""" output = run_cmd( diff --git a/tools/private/release/prepare.py b/tools/private/release/prepare.py index 1d946bda6c..c9012e0d04 100644 --- a/tools/private/release/prepare.py +++ b/tools/private/release/prepare.py @@ -2,7 +2,10 @@ import pathlib from tools.private.release import changelog_news, gh, git -from tools.private.release.release_issue import update_task_in_body +from tools.private.release.release_issue import ( + parse_checklist_state, + update_task_in_body, +) from tools.private.release.utils import ( determine_next_version, replace_version_next, @@ -102,26 +105,47 @@ def cmd_prepare(args): print(f"[DRY RUN] Would push branch {branch_name} to origin") else: modified_files = git.status() - if not modified_files: + if modified_files: + # Stage all modified and deleted tracked files + git.add_modified_and_deleted() + git.commit(f"Prepare release {version}") + else: print("No files modified by the release tool. Nothing to commit.") - return 0 - - # Stage all modified and deleted tracked files - git.add_modified_and_deleted() - git.commit(f"Prepare release {version}") + print(f"Pushing branch {branch_name} to origin...") git.push("origin", branch_name, set_upstream=True) # --- Create PR --- - if args.dry_run: - target_issue = f"#{issue_num}" if issue_num else "" + # Determine if we need to create a PR or reuse an existing one + open_pr = gh.get_open_pr(branch_name) + associated_pr = None + + if not open_pr and issue_num: + body = gh.get_issue_body(issue_num) + state = parse_checklist_state(body) + associated_pr = state["prepare_release"]["pr"] + + if open_pr: + pr_num = open_pr["number"] + pr_url = open_pr["url"] + print(f"Open Pull Request already exists: {pr_url} (PR #{pr_num})") + elif associated_pr: + pr_num = associated_pr.lstrip("#") + pr_url = f"https://github.com/bazel-contrib/rules_python/pull/{pr_num}" print( - f"[DRY RUN] Would create Pull Request for branch {branch_name} targeting issue {target_issue}" + f"PR #{pr_num} is already associated in tracking issue #{issue_num}. Using it." ) else: - pr_url = gh.create_pr(version, issue_num) - pr_num = pr_url.split("/")[-1] - print(f"Created Pull Request: {pr_url} (PR #{pr_num})") + if args.dry_run: + target_issue = f"#{issue_num}" if issue_num else "" + print( + f"[DRY RUN] Would create Pull Request for branch {branch_name} targeting issue {target_issue}" + ) + pr_num = "" + else: + pr_url = gh.create_pr(version, issue_num) + pr_num = pr_url.split("/")[-1] + print(f"Created Pull Request: {pr_url} (PR #{pr_num})") # --- Update checklist --- if args.dry_run: diff --git a/tools/private/release/release.py b/tools/private/release/release.py index d9a225d5cf..dab99e6d05 100644 --- a/tools/private/release/release.py +++ b/tools/private/release/release.py @@ -10,6 +10,7 @@ from tools.private.release import changelog_news, gh, git from tools.private.release.prepare import cmd_prepare from tools.private.release.release_issue import ( + parse_checklist_state, parse_metadata_line, update_task_in_body, ) @@ -35,72 +36,6 @@ def _semver_type(value): # ============================================================================== -def parse_checklist_state(body): - """Parses the main checklist tasks and their metadata.""" - state = { - "prepare_release": { - "checked": False, - "status": None, - "pr": None, - "commit": None, - }, - "create_branch": { - "checked": False, - "status": None, - "branch": None, - "commit": None, - }, - "tag_final": {"checked": False, "status": None, "tag": None, "commit": None}, - "rc_tags": {}, # Dynamically mapped: int -> metadata dict - } - - lines = body.splitlines() - for line in lines: - parsed = parse_metadata_line(line) - if not parsed: - continue - - name = parsed["name"].strip() - meta = parsed["metadata"] - checked = parsed["checked"] - name_lower = name.lower() - - if "prepare release" in name_lower: - state["prepare_release"] = { - "checked": checked, - "status": meta.get("status"), - "pr": meta.get("pr"), - "commit": meta.get("commit"), - } - elif "create release branch" in name_lower: - state["create_branch"] = { - "checked": checked, - "status": meta.get("status"), - "branch": meta.get("branch"), - "commit": meta.get("commit"), - } - elif "tag final" in name_lower: - state["tag_final"] = { - "checked": checked, - "status": meta.get("status"), - "tag": meta.get("tag"), - "commit": meta.get("commit"), - } - else: - # Match Tag RC - rc_match = re.match(r"Tag RC(\d+)", name, re.IGNORECASE) - if rc_match: - rc_num = int(rc_match.group(1)) - state["rc_tags"][rc_num] = { - "checked": checked, - "status": meta.get("status"), - "tag": meta.get("tag"), - "commit": meta.get("commit"), - } - - return state - - def parse_backports(body): """Parses the ## Backports checklist section.""" body = body.replace("\r\n", "\n") diff --git a/tools/private/release/release_issue.py b/tools/private/release/release_issue.py index 27bcf3d774..f96b917ab9 100644 --- a/tools/private/release/release_issue.py +++ b/tools/private/release/release_issue.py @@ -60,3 +60,69 @@ def update_task_in_body(body, task_name, checked, metadata): ) return "\n".join(updated_lines) + + +def parse_checklist_state(body): + """Parses the main checklist tasks and their metadata.""" + state = { + "prepare_release": { + "checked": False, + "status": None, + "pr": None, + "commit": None, + }, + "create_branch": { + "checked": False, + "status": None, + "branch": None, + "commit": None, + }, + "tag_final": {"checked": False, "status": None, "tag": None, "commit": None}, + "rc_tags": {}, # Dynamically mapped: int -> metadata dict + } + + lines = body.splitlines() + for line in lines: + parsed = parse_metadata_line(line) + if not parsed: + continue + + name = parsed["name"].strip() + meta = parsed["metadata"] + checked = parsed["checked"] + name_lower = name.lower() + + if "prepare release" in name_lower: + state["prepare_release"] = { + "checked": checked, + "status": meta.get("status"), + "pr": meta.get("pr"), + "commit": meta.get("commit"), + } + elif "create release branch" in name_lower: + state["create_branch"] = { + "checked": checked, + "status": meta.get("status"), + "branch": meta.get("branch"), + "commit": meta.get("commit"), + } + elif "tag final" in name_lower: + state["tag_final"] = { + "checked": checked, + "status": meta.get("status"), + "tag": meta.get("tag"), + "commit": meta.get("commit"), + } + else: + # Match Tag RC + rc_match = re.match(r"Tag RC(\d+)", name, re.IGNORECASE) + if rc_match: + rc_num = int(rc_match.group(1)) + state["rc_tags"][rc_num] = { + "checked": checked, + "status": meta.get("status"), + "tag": meta.get("tag"), + "commit": meta.get("commit"), + } + + return state