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**\n\n 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 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.\n\nhttps://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**\n\nThis PATCH creates a
regular close event whose is null, while lines 658-660 recognize a previous
merge only from close events with a non-null . 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 landed on , but
its close event has . Please adapt merge detection to the pushed hash/state, or
record and consume another durable marker, before automatically closing.
--
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]
