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: [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]
