[CELEBORN-2440][INFRA] Post a merge summary comment on the PR in merge_pr.py - #3823
[CELEBORN-2440][INFRA] Post a merge summary comment on the PR in merge_pr.py#3823pan3793 wants to merge 5 commits into
Conversation
…e_pr.py Record every branch the change landed on and a link to the resulting commit, so the merge is traceable from the PR page. Assisted-by: Claude Opus 5
…uto-close it The "Closes #N" string in the commit message only auto-closes the PR when the commit lands on the default branch, so close it through the API otherwise. Assisted-by: Claude Opus 5
Capitalize N in the (y/n) prompts to show that the default is no: all of them treat any input other than "y" as no. Add the missing space after ":" in the JIRA assignee prompt. Assisted-by: Claude Opus 5
| 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: |
There was a problem hiding this comment.
GITHUB_OAUTH_KEY is still documented as optional, but when it is unset this sends an unauthenticated PATCH. GitHub requires write permission for updating PR state, so the call fails, the None result is ignored, and maintenance-branch PRs remain open. Please either require and validate a write-capable token before the push, or explicitly skip this feature and update the configuration documentation.
https://docs.github.com/en/rest/pulls/pulls#update-a-pull-request
There was a problem hiding this comment.
Fixed in 11671f4: 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.
| 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) |
There was a problem hiding this comment.
This PATCH creates a regular close event whose commit_id is null, while lines 658-660 recognize a previous merge only from close events with a non-null commit_id. On a later invocation for another backport, the script therefore misses the already-merged path and attempts a fresh merge. This is observable on #3697: commit 0bf9c5a58 landed on branch-0.6, but its close event has commit_id: null. Please adapt merge detection to the pushed hash/state, or record and consume another durable marker, before automatically closing.
There was a problem hiding this 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.
368bec8 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.
11671f4 additionally skips the API close for merges into main, so it can never race GitHub's own auto-close.
| # 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") |
There was a problem hiding this comment.
This calls get_json(), which exits on HTTP errors and lets connection errors escape. Because the target commit has already been pushed, a rate-limit or network failure here prevents post_merge_comment() and all later JIRA handling; rerunning the merge is not a safe recovery. Please catch and report this state check locally, and ensure the remaining bookkeeping still runs.
There was a problem hiding this comment.
Fixed in 11671f4: 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.
GitHub links the merge commit to the "closed" event only when the commit lands on the default branch, so a pull request merged into branch-x.y is not recognized as already merged and the backport path is never taken. Fall back to "referenced" events, which any commit mentioning the PR raises, and confirm each against the merge footer that merge_pr generates. Assisted-by: Claude Opus 5
The merge is already pushed by the time the summary comment and the close run, so neither may abort the remaining bookkeeping: report any failure instead of raising, and skip the close when no token is configured rather than sending an unauthenticated PATCH. Leave merges into main to GitHub's own auto-close, whose close event carries the commit that find_merge_commit prefers. Assisted-by: Claude Opus 5
2 participants
What changes were proposed in this pull request?
Port a few
merge_pr.pyimprovements from Spark'smerge_spark_pr.py:Post a "Merge Summary" comment on the PR after the merge, recording every branch the
change landed on and a link to the resulting commit.
cherry_picknow returns the(ref, hash)pair it pushed so backports are included, and the comment is posted froma
finallyblock, so a cancelled cherry-pick does not drop what already landed. Thebackport path (PR already merged) posts a summary too.
Close pull requests that GitHub does not auto-close.
Closes #Ncloses the PR onlywhen the commit lands on the default branch, so one merged into
branch-x.ystaysopen. Merges into
mainare left to GitHub, whose close event carries the commit thatthe backport path below relies on.
Recognize pull requests merged into
branch-x.y. For the same reason, GitHub links themerge commit to the
closedevent only on the default branch, so those pull requestswere not recognized as merged and re-running the script on one attempted a fresh merge
instead of offering a backport.
find_merge_commitnow falls back toreferencedevents, confirmed against the
Closes #N from <ref>merge footer.Polish the interactive prompts: capitalize
Nin the(y/n)prompts, since all ofthem treat any input other than
yas no, and add the missing space after":"inthe JIRA assignee prompt.
The merge has already been pushed by the time the comment and the close run, so neither
aborts the rest of the bookkeeping: failures are reported rather than raised, and the
close is skipped when
GITHUB_OAUTH_KEYis not configured.For example, #3788 landed on
mainandbranch-0.7, and would have got:Why are the changes needed?
A merged PR currently records nothing about where the change landed: the commit is
reachable only by searching for the
Closes #Nfooter, and backport targets are notvisible from the PR page at all. Pull requests merged into
branch-x.yalso stay openuntil someone closes them by hand, and the script does not recognize them as merged
afterwards.
Does this PR resolve a correctness bug?
Does this PR introduce any user-facing change?
How was this patch tested?
Manually:
python3 -m py_compile dev/merge_pr.pyand the module doctests, which now coverhas_merge_footer, pass.find_merge_commitwas checked against the real GitHub events of#3697 (merged into
branch-0.6, previously undetected, now resolves0bf9c5a58), #3788 and#3768 (both merged into
main, still resolved from theclosedevent). The GitHub writecalls will be exercised by the next real merge.