SteNicholas commented on code in PR #3823:
URL: https://github.com/apache/celeborn/pull/3823#discussion_r3844657345
##########
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:
**[P1] Require authentication before trying to close the PR**
`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
##########
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:
**[P1] Preserve a merge marker when closing the PR**
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.
##########
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:
**[P2] Keep post-push bookkeeping best-effort**
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.
--
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]
