pan3793 commented on code in PR #3823:
URL: https://github.com/apache/celeborn/pull/3823#discussion_r3916877654
##########
dev/merge_pr.py:
##########
@@ -90,6 +91,59 @@ def get_json(url):
sys.exit(-1)
+def close_pr(pr_num):
+ url = "%s/pulls/%s" % (GITHUB_API_BASE, pr_num)
+ data = json.dumps({"state": "closed"}).encode("utf-8")
+ request = Request(url, data=data, method="PATCH")
+ request.add_header("Content-Type", "application/json")
+ request.add_header("Accept", "application/vnd.github+json")
+ if GITHUB_OAUTH_KEY:
Review Comment:
Fixed in 11671f45a: `close_pr` now returns early with `GITHUB_OAUTH_KEY is
not set; skipping closing PR #N.`, the guard `post_merge_comment` already had,
and the `GITHUB_OAUTH_KEY` doc comment no longer describes the key as a
rate-limit convenience. Note the failure was reported rather than silently
ignored (`Failed to close PR #N: HTTP 401 Unauthorized`), but the pre-check is
clearer and consistent.
##########
dev/merge_pr.py:
##########
@@ -634,11 +691,27 @@ def main():
merge_hash = merge_pr(pr_num, target_ref, title, body, pr_repo_desc)
+ # Ordered (branch, commit hash) pairs for the merge comment: the merge
sink first,
+ # then each cherry-pick target as it is picked.
+ merged_commits = [(target_ref, merge_hash)]
+
pick_prompt = "Would you like to pick %s into another branch?" % merge_hash
- while input("\n%s (y/n): " % pick_prompt).lower() == "y":
- merged_refs = merged_refs + [
- cherry_pick(pr_num, merge_hash, next(branch_iter, branch_names[0]))
- ]
+ # Post the summary in a finally block: the merge into the target branch
has already
+ # been pushed, so aborting a later cherry-pick must not drop that line.
+ try:
+ while input("\n%s (y/N): " % pick_prompt).lower() == "y":
+ picked = cherry_pick(pr_num, merge_hash, next(branch_iter,
branch_names[0]))
+ merged_refs = merged_refs + [picked[0]]
+ merged_commits = merged_commits + [picked]
+ finally:
+ # The "Closes #N" string in the commit message only auto-closes the PR
when the
+ # commit lands on the default branch. For merges into other branches
(e.g.
+ # branch-X.Y backport PRs), GitHub leaves the PR open, so close it
through the API.
+ pr_state = get_json("%s/pulls/%s" % (GITHUB_API_BASE,
pr_num)).get("state")
+ if pr_state != "closed":
+ print("\nPR #%s is still open after push; closing it
explicitly.\n" % pr_num)
+ close_pr(pr_num)
Review Comment:
The detection gap predates this PR: GitHub links a commit to the `closed`
event only when it lands on the default branch, so a pull request merged into
branch-x.y never had one. On #3697 the `commit_id: null` close event was
created manually on 2026-05-21, and the old filter was already empty for it, so
the API close does not lose anything that used to work.
368bec8f0 fixes the gap: `find_merge_commit` prefers `closed` events and
falls back to `referenced` events confirmed against the `Closes #N from <ref>`
merge footer. Verified against the real events of #3697: previously undetected,
now resolves `0bf9c5a58`.
11671f45a additionally skips the API close for merges into main, so it can
never race GitHub's own auto-close.
##########
dev/merge_pr.py:
##########
@@ -634,11 +691,27 @@ def main():
merge_hash = merge_pr(pr_num, target_ref, title, body, pr_repo_desc)
+ # Ordered (branch, commit hash) pairs for the merge comment: the merge
sink first,
+ # then each cherry-pick target as it is picked.
+ merged_commits = [(target_ref, merge_hash)]
+
pick_prompt = "Would you like to pick %s into another branch?" % merge_hash
- while input("\n%s (y/n): " % pick_prompt).lower() == "y":
- merged_refs = merged_refs + [
- cherry_pick(pr_num, merge_hash, next(branch_iter, branch_names[0]))
- ]
+ # Post the summary in a finally block: the merge into the target branch
has already
+ # been pushed, so aborting a later cherry-pick must not drop that line.
+ try:
+ while input("\n%s (y/N): " % pick_prompt).lower() == "y":
+ picked = cherry_pick(pr_num, merge_hash, next(branch_iter,
branch_names[0]))
+ merged_refs = merged_refs + [picked[0]]
+ merged_commits = merged_commits + [picked]
+ finally:
+ # The "Closes #N" string in the commit message only auto-closes the PR
when the
+ # commit lands on the default branch. For merges into other branches
(e.g.
+ # branch-X.Y backport PRs), GitHub leaves the PR open, so close it
through the API.
+ pr_state = get_json("%s/pulls/%s" % (GITHUB_API_BASE,
pr_num)).get("state")
Review Comment:
Fixed in 11671f45a: the state lookup is gone, since the close is now
conditional on the target branch instead, so there is no API call between the
push and the bookkeeping. `post_merge_comment` runs first, and
`close_pr`/`comment_pr` catch `Exception` rather than only `HTTPError`, so a
connection error can no longer skip the merge summary or the JIRA update.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
