spark-test commented on code in PR #60031:
URL: https://github.com/apache/spark/pull/60031#discussion_r4479767730
##########
dev/merge_spark_pr.py:
##########
@@ -2658,61 +3091,25 @@ def main():
if pr.get("draft", False) or "[WIP]" in title or "[DO-NOT-MERGE]" in title:
fail("Cannot merge a draft PR #%s: %s" % (pr_num, title))
- # e.g. 'Revert "[SPARK-56357][BUILD] Upgrade sbt to 1.12.8"'
- is_revert_pr = title.startswith('Revert "') and title.endswith('"')
- # e.g. 'Reapply "[SPARK-56357][BUILD] Upgrade sbt to 1.12.8"'
- is_reapply_pr = title.startswith('Reapply "') and title.endswith('"')
-
- # Normalized PR-title component tags, used later to reconcile JIRA
components. Empty for
- # Revert/Reapply PRs, whose titles are kept verbatim and not parsed for
components.
- title_components: List[str] = []
-
- # Revert and Reapply PRs keep their title verbatim.
- if not (is_revert_pr or is_reapply_pr):
- # Parse; fail on a malformed title.
- try:
- parsed = Title.parse(title)
- except ValueError as e:
- fail("Malformed PR title: %s" % e)
-
- # Normalize component tags via the registry and track primary.
- components = []
- has_primary = False
- for tag in parsed.components:
- c = Component.find(tag)
- if c is not None and c.primary:
- has_primary = True
- components.append(Component.normalize(tag))
- if not has_primary:
- new_tags = prompt_for_components()
- components = new_tags + components
-
- # Deduplicate tags in insertion order.
- components = list(dict.fromkeys(components))
-
- # Move version tags (e.g. [4.X], [4.2]) to the head of components.
- versions = [t for t in components if _VERSION_TAG_RE.match(t)]
- if versions:
- others = [t for t in components if not _VERSION_TAG_RE.match(t)]
- components = versions + others
-
- # Move FOLLOWUP to the last tag.
- non_followup = [t for t in components if t != "FOLLOWUP"]
- if len(non_followup) < len(components):
- components = non_followup + ["FOLLOWUP"]
-
- # Warn about tags that are neither known components nor version tags.
- unknown = [
- t for t in components if Component.find(t) is None and not
_VERSION_TAG_RE.match(t)
- ]
- if unknown:
- print_error("Title has unknown tag(s): %s" % ", ".join("[%s]" % t
for t in unknown))
-
- parsed.components = components
- title_components = list(parsed.components)
- title = str(parsed)
- if title != pr["title"]:
- print("Normalized title: %s" % title)
+ # Merged pull requests don't appear as merged in the GitHub API;
+ # Instead, they're closed by committers.
+ # A PR might have multiple merge commits, if it's reopened and merged
again. We shall
+ # cherry-pick PRs in closed state with the latest merge hash.
+ # If the PR is still open (reopened), we shall not cherry-pick it but
perform the normal
+ # merge as it could have been reverted earlier.
+ merge_hash, message = (None, None)
+ if pr["state"] == "closed":
+ merge_hash, message = find_merge_commit(pr_num, pr_events)
+ if merge_hash is not None:
+ print("Pull request %s has already been merged, assuming you want to
backport" % pr_num)
+ commit_is_downloaded = (
+ git.run(["git", "rev-parse", "--quiet", "--verify", "%s^{commit}"
% merge_hash]).strip()
+ != ""
+ )
+ if not commit_is_downloaded:
+ fail("Couldn't find any merge commit for #%s, you may need to
update HEAD." % pr_num)
+ print("Found commit %s:\n%s" % (merge_hash, message))
+ title = title_for_run(pr_num, title, message)
Review Comment:
**Nit (P3):** No doctest reaches this line or the
`jira_title_for_backport()` call before the backport's JIRA update, and
together they decide whether a backport reads the committed subject. Passing
`None` here instead of `message`, dropping that call, or swapping its two title
arguments would still pass every doctest, and the PR description itself lists
four `main()` mutants that survive. With `None`, a backport goes back to
preparing the current PR title, prompting for its components and updating JIRA
from it. `main()` has never been doctested in this file, and moving the title
handling into `prepare_merge_title()`, `title_for_run()` and
`jira_title_for_backport()` already pinned most of it. Committing the mocked
`main()` run that the description mentions, as a doctest, would pin these call
sites too.
This extends existing discussions
[1](https://github.com/apache/spark/pull/60031#discussion_r4448436218),
[2](https://github.com/apache/spark/pull/60031#discussion_r4470227689).
**Verification:**
- **Regression:** On a backport whose commit supplies a committed title and
whose current PR title would update JIRA differently, update_jira_for_pr()
receives the committed title by default, and the notice and question are
printed right before the JIRA update. Passing None instead of the found commit
message to title_for_run(), dropping the jira_title_for_backport() call, or
swapping its two title arguments each makes this coverage fail.
- **Behavior:** A merge run passes the prepared title, including a component
entered at the prompt, to update_jira_for_pr(), and a backport whose commit
supplies no committed title passes the prepared current PR title.
##########
dev/merge_spark_pr.py:
##########
@@ -670,6 +670,32 @@ def message_of(commit_hash):
return None, None
+def committed_title(pr_num: Union[int, str], message: Optional[str]) ->
Optional[str]:
+ """Return the subject of the merge commit ``message`` if ``merge_pr`` made
it for this PR.
+
+ That is the title that was committed, and a backport run reads it for the
JIRA update
+ instead of the current PR title, which may have been edited since the
merge. It is
+ used only when the merge footer of ``message`` closes ``pr_num``;
otherwise this
+ returns None, and the PR title is used as before, e.g. for a commit made
with the GitHub
+ squash button, whose subject ends with ``(#N)``, or a commit of another PR.
+
+ >>> footer = "\\n\\nBody\\n\\nCloses #1 from a/b.\\n\\nAuthored-by: A
<[email protected]>"
+ >>> committed_title(1, 'Revert "Revert "[SPARK-1][SQL] Fix"" \\r' + footer)
+ 'Revert "Revert "[SPARK-1][SQL] Fix""'
+ >>> committed_title("1", "[SPARK-1][SQL] Fix" + footer)
+ '[SPARK-1][SQL] Fix'
+ >>> committed_title(1, 'Revert "[SPARK-1][SQL] Fix" (#1)\\n\\nBody') is
None
+ True
+ >>> committed_title(1, "[SPARK-2][SQL] Other" + footer.replace("#1",
"#2")) is None
+ True
+ >>> committed_title(1, None) is None
+ True
+ """
+ if message is None or not has_merge_footer(message, pr_num):
Review Comment:
**Non-blocking (P2):** This returns None for every commit without this PR's
merge footer, including a GitHub squash-button commit. `.asf.yaml` enables that
button with `PR_TITLE_AND_DESC`, so the commit's subject is the PR title at
merge time plus ` (#N)`, and its body is the PR description with no `Closes #N
from ...` footer (e.g. 6bae835ccbc). `find_merge_commit()` takes the commit
linked to the PR's `closed` event without a footer check, and the doctests here
and in `title_for_run()` and `jira_title_for_backport()` treat such a commit as
a backport input. Its backport then prepares the current PR title, and
`jira_title_for_backport()` returns that title without a notice.
So a backport of a squash-merged PR still updates JIRA from a title edited
after the merge, which is the failure this PR fixes for footer commits. For
example, if a PR is squash-merged as `Revert "[SPARK-1][SQL] X"` and later
retitled `[SPARK-1][SQL] X`, its backport offers to resolve SPARK-1 or add fix
versions to it instead of removing them, and nothing shows the committed title.
The ` (#N)` suffix is all that separates the subject from the committed title.
Such merges are rare (6 of the 50,881 commit subjects on master at the base of
this PR end with ` (#N)`), and the later JIRA prompts still show the ticket.
**Recommended change:** Teach committed_title() to recognize this PR's
GitHub squash-button commit, whose subject is the PR title at merge time
followed by ' (#N)' with this PR's number, and return that subject without the
suffix. A squash-button backport then reads the committed title as a footer
backport does: title_for_run() uses it without preparing the current PR title,
and jira_title_for_backport() shows the notice and offers the current PR title
when the two would update JIRA differently. Update the committed_title(),
title_for_run(), jira_title_for_backport() and update_jira_for_pr() docstrings
and the doctests whose squash-button cases now yield the stripped subject, and
the PR description's sentences about squash-button commits.
**Why this works:** The suffix names the PR the commit was squash-merged
from, just as the footer names the PR merge_pr closed. Stripping it exposes the
committed title, so jira_action() reads the committed ids, skip tags and
direction, and the existing notice and choice handle a PR title edited after
the merge.
**Scope:** Extend backport committed-title detection to GitHub squash-button
commits and update the related docstrings, doctests and PR description.
**Compatibility:** Commits with this PR's merge footer, merge runs, and
commits that belong to other PRs keep their current title handling.
**Risks:** A squash-button backport whose PR title has no primary component
no longer prompts for one, and one whose current PR title fails Title.parse()
no longer stops, matching what the PR already does for footer commits; removing
the suffix check restores the current fallback. A squash commit title the
merger edited in GitHub's dialog becomes the default JIRA title, though the
notice and choice still show the current PR title when the two would update
JIRA differently. This assumes, as this PR's docstrings and the author's
replies do, that GitHub links a squash-button merge commit to the PR's closed
event, so that find_merge_commit() returns it for a backport. If it does not, a
squash-merged PR never reaches the backport path and this change has no effect
for it.
**Constraints:** has_merge_footer() remains the first check, so commits with
this PR's merge footer keep their current subject handling. Only a trailing '
(#N)' with this PR's own number is stripped; any other commit without the
footer still falls back to the prepared PR title.
**Exclusions:**
- Running check_wrapped_title() on backports stays out of scope, as for
footer commits, since a committed title cannot be retitled before merging.
**Success:** A backport of this PR's squash-button commit whose subject is
'T (#N)' updates JIRA from T by default. When the current PR title would update
JIRA differently from T, the notice and the question about using the current PR
title appear right before the JIRA update, as for footer commits. Footer
commits, other PRs' commits and merge runs keep their current titles, prompts
and JIRA actions.
**Verification:**
- **Regression:** A backport of this PR's squash-button commit 'T (#N)'
whose current PR title would update JIRA differently from T, through other skip
tags, other ticket ids or the other direction, updates JIRA from T by default
and prints the notice and question right before the JIRA update; e.g. 'Revert
"[SPARK-1][SQL] X" (#N)' with the current title '[SPARK-1][SQL] X' takes the
revert path for SPARK-1.
- **Behavior:** committed_title() returns the subject without its ' (#N)'
suffix for this PR's squash-button message, still returns the stripped subject
for a message with this PR's footer, and returns None for another PR's
squash-button or footer commit and for a message with neither.
- **Compatibility:** Merge runs and footer-commit backports produce the same
titles, prompts and JIRA actions as before the change.
##########
dev/merge_spark_pr.py:
##########
@@ -2012,6 +2088,150 @@ def update_jira_for_pr(pr_num, title, merge_branches,
title_components, is_rever
)
+def _bracketed(tags):
+ """Format tags as in a title, e.g. ``[MINOR], [FOLLOWUP]``, for
messages."""
+ return ", ".join("[%s]" % tag for tag in tags)
+
+
+def committed_title_notice(subject: str, pr_title: str) -> Optional[str]:
+ """Say that ``subject`` and ``pr_title`` would update JIRA differently, or
return None.
+
+ Both are compared by :func:`jira_action`: whether JIRA is skipped, and if
not, which
+ tickets and in which direction. The components are not compared: they
differ whenever
+ one was entered at the merge prompt, and are only reconciled with an
unresolved ticket.
+ A resolve of a ticket that is already resolved as Fixed only adds fix
versions, and a
Review Comment:
**Nit (P3):** For a ticket already resolved as Fixed, `resolve_jira_issue()`
does more than add fix versions. It calls
`maybe_reconcile_jira_affects_versions()` when every inferred fix version is
already present, when the committer declines the addition, when no new version
is chosen, and before `add_fix_versions()`, and that call can prompt for and
change the Affects Version/s. The comparison here is unaffected, since that
step does not depend on the title. The sentence could say, e.g., that such a
resolve adds fix versions and may offer to correct the Affects Version/s,
without reconciling components.
**Verification:**
- **Inspection:** The docstring's description of resolving a ticket already
resolved as Fixed matches resolve_jira_issue(): fix versions are added, Affects
Version/s may be corrected, and components are not reconciled.
--
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]
