You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "Yicong-Huang (via GitHub)" <gi...@apache.org> on 2026/08/21 06:04:48 UTC
[PR] [SPARK-58929][INFRA] Add a dry-run mode to merge_spark_pr.py [spark]
Yicong-Huang opened a new pull request, #58198:
URL: https://github.com/apache/spark/pull/58198
### What changes were proposed in this pull request?
This adds a dry-run mode to `dev/merge_spark_pr.py` that performs every read-only step for real but suppresses every effect that leaves the local machine, logging a clear `DRY-RUN: would <action> ...` line instead of executing it. The suppressed effects are exactly the outbound ones:
- the `git push` to `PUSH_REMOTE_NAME` (the only git command that reaches the shared apache repo; local fetch/checkout/squash-merge/cherry-pick still run on the throwaway `PR_TOOL_*` branches that `clean_up` removes, so conflicts and the computed merge hash stay realistic);
- the GitHub PR close and the merge/summary comment (`close_pr`, `comment_pr`, `post_merge_comment`);
- all JIRA writes: component updates, `fixVersions` updates, the resolve transition, assignee changes, and the contributor-role grant.
Read-only steps still run so the flow is faithful: fetching the PR, the JIRA issue lookup and `project_versions`, and -- importantly -- JIRA and GitHub token validation (`initialize_jira()` and `get_json()` are not gated), so a dry run exercises the same credentials a real merge needs. Interactive prompts continue to work.
Dry-run is enabled with a `--dry-run` / `-n` flag or the `DRY_RUN` environment variable, consistent with the script's existing env-based configuration. A small `parse_args` helper handles the flag (the script previously read `sys.argv` directly). Two new pure helpers (`parse_args` and `is_remote_mutating_git_cmd`) are covered by doctests.
### Why are the changes needed?
Before this change there was no way to rehearse the full merge flow without producing real side effects. The existing affordances cover only part of it: doctests exercise the pure logic; `SKIP_VERSION_CHECK` lets a committer iterate against a real PR; and `PR_REMOTE_NAME` / `PUSH_REMOTE_NAME` can point the `git push` at a scratch fork. But `GITHUB_API_BASE` and `JIRA_API_BASE` are hardcoded to apache, so the PR close/comment and JIRA resolve paths cannot be redirected and always hit the real `apache/spark` repo and ASF JIRA. Dry-run is the first mechanism that makes the entire flow (git + GitHub + JIRA) side-effect-free while keeping it realistic, which is useful for previewing a merge and for iterating on the merge script itself.
### Does this PR introduce _any_ user-facing change?
No. This is a committer-facing dev tool; the new mode is opt-in and the default behavior is unchanged.
### How was this patch tested?
- Doctests: `python -m doctest dev/merge_spark_pr.py` passes, including the new cases for `parse_args` and `is_remote_mutating_git_cmd` (92 tests total).
- Manual smoke test with `DRY_RUN=1`: confirmed `run_cmd` suppresses `git push` (string and list forms) and returns `""` while a read-only `git rev-parse` still runs; `close_pr`, `comment_pr`, `post_merge_comment`, `grant_contributor_role`, and `assign_issue` all print their `DRY-RUN: would ...` line and make no network call; and with dry-run off the default remains unchanged.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58929][INFRA] Add a dry-run mode to merge_spark_pr.py [spark]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on PR #58198:
URL: https://github.com/apache/spark/pull/58198#issuecomment-5414562363
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/7782448cd2f8cfd74f8ae0677930304154506047
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58929][INFRA] Add a dry-run mode to merge_spark_pr.py [spark]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #58198:
URL: https://github.com/apache/spark/pull/58198#discussion_r3847555376
##########
dev/merge_spark_pr.py:
##########
@@ -548,7 +567,40 @@ def fail(msg):
sys.exit(-1)
+def is_remote_mutating_git_cmd(cmd):
+ """True only for a git command that mutates a remote: today just ``git push``.
+
+ ``cmd`` is either a string ("git push apache X:branch-4.x") or an argv list. In dry-run
+ mode only these are suppressed; every other git command -- fetch, checkout, merge, commit,
+ cherry-pick, rev-parse, config, branch -D -- still runs, so the local squash-merge and
+ cherry-picks happen on the throwaway PR_TOOL_* branches that clean_up always removes. That
+ keeps conflict detection and the computed merge hash realistic, while the push to
+ PUSH_REMOTE_NAME (the only command that reaches the shared apache repo) is the single git
+ effect held back.
+
+ >>> is_remote_mutating_git_cmd("git push apache X:branch-4.x")
+ True
+ >>> is_remote_mutating_git_cmd(["git", "push", "apache", "X:branch-4.x"])
+ True
+ >>> is_remote_mutating_git_cmd("git fetch apache master:PR_TOOL_tmp")
+ False
+ >>> is_remote_mutating_git_cmd(["git", "commit", '--author="a <b>"', "-m", "msg"])
+ False
+ >>> is_remote_mutating_git_cmd("git checkout PR_TOOL_MERGE_PR_1")
+ False
+ >>> is_remote_mutating_git_cmd("git rev-parse HEAD")
+ False
+ """
+ tokens = cmd.split(" ") if isinstance(cmd, str) else list(cmd)
+ tokens = [t for t in tokens if t]
+ return len(tokens) >= 2 and tokens[0] == "git" and tokens[1] == "push"
+
+
def run_cmd(cmd):
+ if DRY_RUN and is_remote_mutating_git_cmd(cmd):
+ rendered = cmd if isinstance(cmd, str) else " ".join(cmd)
+ print("DRY-RUN: would run: %s" % rendered)
+ return ""
Review Comment:
Good catch, that was a real weakness. It should be gone with the refactor: there's no generic run_cmd anymore. Git I/O is on the Git client and push (the only remote mutation) is Git.push(), overridden by DryRunGit, and GitHub and JIRA writes go through their own clients the same way. So a new mutating call now has to land as a method on one of them, which hopefully makes the dry-run handling much harder to miss.
##########
dev/merge_spark_pr.py:
##########
@@ -83,6 +83,16 @@
# exceeding your IP's unauthenticated request rate limit. You can create an OAuth key at
# https://github.com/settings/tokens. This script only requires the "public_repo" scope.
GITHUB_OAUTH_KEY = os.environ.get("GITHUB_OAUTH_KEY")
+# When set to any non-empty value (via this env var, or the --dry-run/-n flag parsed in main),
+# run every read-only step for real -- fetch the PR, look up JIRA, compute fix versions, and
+# perform the local squash-merge and cherry-picks on the throwaway PR_TOOL_* branches so conflicts
+# still surface and a real merge hash is computed -- but suppress every effect that leaves this
+# machine: the git push to PUSH_REMOTE_NAME, the GitHub PR close/comment, and all JIRA writes
+# (component and fixVersion updates, assignment, and the resolve transition). Each suppressed
+# effect is logged as a "DRY-RUN: would ..." line instead of running. Any non-empty string is
+# truthy, matching the SKIP_VERSION_CHECK convention above. This is a module-level default; main()
+# also turns it on for the --dry-run/-n flag.
+DRY_RUN = bool(os.environ.get("DRY_RUN"))
Review Comment:
That's a fair point, thanks. For now I've kept the env var but wired it as the default for --dry-run, mainly to stay consistent with the existing pattern in this script (SKIP_VERSION_CHECK works the same way). The broader standardization concern really resonates, though. I'd love to revisit it properly across the dev tooling as a follow-up rather than trying to solve it just for this one flag here.
##########
dev/merge_spark_pr.py:
##########
@@ -1747,10 +1820,53 @@ def check_script_up_to_date():
)
+def parse_args(argv):
Review Comment:
Good call, switched to argparse.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58929][INFRA] Add a dry-run mode to merge_spark_pr.py [spark]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on PR #58198:
URL: https://github.com/apache/spark/pull/58198#issuecomment-5414768364
> I would have expected a bit more time for people to review this since it's a significant refactoring. In any case, I will try out the dry run on #58214 (after I resolve the merge conflict there)!
Fair point, sorry for rushing this. I tested the dry-run mode locally and also merged this PR itself using the updated merge script without dry run. So functionality wise the refactor should work the same as before. Please do let me know if you found any issues! I can quickly address them in follow-up PRs.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58929][INFRA] Add a dry-run mode to merge_spark_pr.py [spark]
Posted by "nchammas (via GitHub)" <gi...@apache.org>.
nchammas commented on code in PR #58198:
URL: https://github.com/apache/spark/pull/58198#discussion_r3845223453
##########
dev/merge_spark_pr.py:
##########
@@ -548,7 +567,40 @@ def fail(msg):
sys.exit(-1)
+def is_remote_mutating_git_cmd(cmd):
+ """True only for a git command that mutates a remote: today just ``git push``.
+
+ ``cmd`` is either a string ("git push apache X:branch-4.x") or an argv list. In dry-run
+ mode only these are suppressed; every other git command -- fetch, checkout, merge, commit,
+ cherry-pick, rev-parse, config, branch -D -- still runs, so the local squash-merge and
+ cherry-picks happen on the throwaway PR_TOOL_* branches that clean_up always removes. That
+ keeps conflict detection and the computed merge hash realistic, while the push to
+ PUSH_REMOTE_NAME (the only command that reaches the shared apache repo) is the single git
+ effect held back.
+
+ >>> is_remote_mutating_git_cmd("git push apache X:branch-4.x")
+ True
+ >>> is_remote_mutating_git_cmd(["git", "push", "apache", "X:branch-4.x"])
+ True
+ >>> is_remote_mutating_git_cmd("git fetch apache master:PR_TOOL_tmp")
+ False
+ >>> is_remote_mutating_git_cmd(["git", "commit", '--author="a <b>"', "-m", "msg"])
+ False
+ >>> is_remote_mutating_git_cmd("git checkout PR_TOOL_MERGE_PR_1")
+ False
+ >>> is_remote_mutating_git_cmd("git rev-parse HEAD")
+ False
+ """
+ tokens = cmd.split(" ") if isinstance(cmd, str) else list(cmd)
+ tokens = [t for t in tokens if t]
+ return len(tokens) >= 2 and tokens[0] == "git" and tokens[1] == "push"
+
+
def run_cmd(cmd):
+ if DRY_RUN and is_remote_mutating_git_cmd(cmd):
+ rendered = cmd if isinstance(cmd, str) else " ".join(cmd)
+ print("DRY-RUN: would run: %s" % rendered)
+ return ""
Review Comment:
I feel like it's too easy for someone down the line to call `run_cmd` with a mutating non-git command -- maybe a call to `gh`, or to another Python script -- and be surprised that their dry run wasn't completely dry.
##########
dev/merge_spark_pr.py:
##########
@@ -1747,10 +1820,53 @@ def check_script_up_to_date():
)
+def parse_args(argv):
Review Comment:
If we're going to expand the command line interface, I really think we should use `argparse` and avoid getting into custom arg handling if it's not really necessary.
##########
dev/merge_spark_pr.py:
##########
@@ -83,6 +83,16 @@
# exceeding your IP's unauthenticated request rate limit. You can create an OAuth key at
# https://github.com/settings/tokens. This script only requires the "public_repo" scope.
GITHUB_OAUTH_KEY = os.environ.get("GITHUB_OAUTH_KEY")
+# When set to any non-empty value (via this env var, or the --dry-run/-n flag parsed in main),
+# run every read-only step for real -- fetch the PR, look up JIRA, compute fix versions, and
+# perform the local squash-merge and cherry-picks on the throwaway PR_TOOL_* branches so conflicts
+# still surface and a real merge hash is computed -- but suppress every effect that leaves this
+# machine: the git push to PUSH_REMOTE_NAME, the GitHub PR close/comment, and all JIRA writes
+# (component and fixVersion updates, assignment, and the resolve transition). Each suppressed
+# effect is logged as a "DRY-RUN: would ..." line instead of running. Any non-empty string is
+# truthy, matching the SKIP_VERSION_CHECK convention above. This is a module-level default; main()
+# also turns it on for the --dry-run/-n flag.
+DRY_RUN = bool(os.environ.get("DRY_RUN"))
Review Comment:
Do we need to capture this as an env var as well (vs. just the CLI parameter)? We have so many already across our dev tooling and [no standardization](https://github.com/apache/spark/pull/57010#discussion_r3535264733).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58929][INFRA] Add a dry-run mode to merge_spark_pr.py [spark]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #58198:
URL: https://github.com/apache/spark/pull/58198#discussion_r3851057773
##########
dev/merge_spark_pr.py:
##########
@@ -548,12 +583,39 @@ def fail(msg):
sys.exit(-1)
-def run_cmd(cmd):
- print(cmd)
- if isinstance(cmd, list):
- return subprocess.check_output(cmd).decode("utf-8")
- else:
- return subprocess.check_output(cmd.split(" ")).decode("utf-8")
+class Git:
+ """Runs git for the merge script -- the single seam for all git I/O.
Review Comment:
Good point, thanks. I narrowed the docstring to the merge/backport flow and noted that check_script_up_to_date() calls git merge-base directly, so it no longer implies this client covers every git invocation.
##########
dev/merge_spark_pr.py:
##########
@@ -1312,23 +1350,99 @@ def choose_jira_assignee(issue):
print("Error assigning JIRA, try again (or leave blank and fix manually)")
-def grant_contributor_role(user: str):
- role = asf_jira.project_role("SPARK", 10010)
- role.add_user(user)
- print("Successfully added user '%s' to contributors role" % user)
-
+class Jira:
+ """ASF JIRA writes used by the merge script -- the single seam for JIRA mutations.
-def assign_issue(issue: int, assignee: str) -> bool:
- """
- Assign an issue to a user, which is a shorthand for jira.client.JIRA.assign_issue.
- The original one has an issue that it will search users again and only choose the assignee
- from 20 candidates. If it's unmatched, it picks the head blindly. In our case, the assignee
- is already resolved.
+ Reads stay on the module-level asf_jira client; only the writes (components, fix versions,
Review Comment:
You're right, thanks. Reworded it to say the pre-write lookups stay on asf_jira, and to call out that add_fix_versions and resolve_issue read the issue back after writing (with DryRunJira skipping both the write and that read-back).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58929][INFRA] Add a dry-run mode to merge_spark_pr.py [spark]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang closed pull request #58198: [SPARK-58929][INFRA] Add a dry-run mode to merge_spark_pr.py
URL: https://github.com/apache/spark/pull/58198
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58929][INFRA] Add a dry-run mode to merge_spark_pr.py [spark]
Posted by "nchammas (via GitHub)" <gi...@apache.org>.
nchammas commented on PR #58198:
URL: https://github.com/apache/spark/pull/58198#issuecomment-5414709777
I would have expected a bit more time for people to review this since it's a significant refactoring. In any case, I will try out the dry run on #58214 (after I resolve the merge conflict there)!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58929][INFRA] Add a dry-run mode to merge_spark_pr.py [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #58198:
URL: https://github.com/apache/spark/pull/58198#discussion_r3848994683
##########
dev/merge_spark_pr.py:
##########
@@ -548,12 +583,39 @@ def fail(msg):
sys.exit(-1)
-def run_cmd(cmd):
- print(cmd)
- if isinstance(cmd, list):
- return subprocess.check_output(cmd).decode("utf-8")
- else:
- return subprocess.check_output(cmd.split(" ")).decode("utf-8")
+class Git:
+ """Runs git for the merge script -- the single seam for all git I/O.
Review Comment:
This `all git I/O` wording is broader than the current script: `check_script_up_to_date()` still shells out to `git merge-base` directly. I would narrow this to the merge-flow git commands so future changes do not assume this client covers every git invocation.
```suggestion
"""Runs merge-flow git commands for the merge script.
```
##########
dev/merge_spark_pr.py:
##########
@@ -1312,23 +1350,99 @@ def choose_jira_assignee(issue):
print("Error assigning JIRA, try again (or leave blank and fix manually)")
-def grant_contributor_role(user: str):
- role = asf_jira.project_role("SPARK", 10010)
- role.add_user(user)
- print("Successfully added user '%s' to contributors role" % user)
-
+class Jira:
+ """ASF JIRA writes used by the merge script -- the single seam for JIRA mutations.
-def assign_issue(issue: int, assignee: str) -> bool:
- """
- Assign an issue to a user, which is a shorthand for jira.client.JIRA.assign_issue.
- The original one has an issue that it will search users again and only choose the assignee
- from 20 candidates. If it's unmatched, it picks the head blindly. In our case, the assignee
- is already resolved.
+ Reads stay on the module-level asf_jira client; only the writes (components, fix versions,
Review Comment:
This reads broader than the implementation. `add_fix_versions()` and `resolve_issue()` both read the issue back through `self._client.issue(...)` after writing, so I would narrow this to the pre-write lookups or call out those readbacks.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58929][INFRA] Add a dry-run mode to merge_spark_pr.py [spark]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on PR #58198:
URL: https://github.com/apache/spark/pull/58198#issuecomment-5401504604
@nchammas thanks so much for the suggestion, this is really the shape I was hoping for. I'd held off at first because it's a fairly big change to the script and I wasn't sure it would be welcome, but since you're raising the same concern I was glad to go ahead with it. push, the GitHub close/comment, and the JIRA writes now each go through a client with a Production and a DryRun subclass, and main() constructs the DryRun ones when --dry-run is set, so all the dry-run behavior lives in those subclasses. Please let me know if the structure isn't quite what you had in mind, happy to adjust.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58929][INFRA] Add a dry-run mode to merge_spark_pr.py [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #58198:
URL: https://github.com/apache/spark/pull/58198#issuecomment-5366722235
+1, LGTM! Thank you @Yicong-Huang!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org