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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
