sarutak commented on code in PR #60031:
URL: https://github.com/apache/spark/pull/60031#discussion_r4477009431
##########
dev/merge_spark_pr.py:
##########
@@ -2658,61 +2876,7 @@ 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)
+ title = prepare_merge_title(title)
Review Comment:
Done. `main()` now looks up the merge commit right after the draft check,
and a new `title_for_run()` decides the title for the run. When the commit has
the PR's merge footer, `committed_title()` returns its subject, and the
backport uses it without calling `prepare_merge_title()`, so the current PR
title is not passed to `Title.parse()` and no component is asked for; the ids,
the direction, and the components come from the subject. A title without a
primary component, e.g. `[SPARK-1][TESTS] Fix`, no longer prompts on such a
backport, and one that `Title.parse()` rejects no longer stops it.
`prepare_merge_title()` runs only for a merge and for the no-footer fallback,
e.g. a squash-button commit, where it still prompts, and still fails on such a
title, as before. `title_for_run()` has a doctest for the three cases, and the
mocked run of `main()` checks that, on a backport, the component prompt appears
only in the no-footer fallback.
This replaces `backport_jira_title()`, which my replies to [13], [16], and
[17] describe. My reply to [13] also said that a backport still prepares the PR
title and stops on a title `Title.parse()` rejects, that
`check_wrapped_title()` runs after the backport branch, and that a PR title
corrected after the merge is not used; for a commit with the PR's merge footer,
none of these holds now (see [24] and [26]). My reply to [16] listed the calls
in `main()` that no doctest pins. The `prepare_merge_title()` and
`check_wrapped_title()` calls moved into `title_for_run()`, whose doctest pins
them, and the footer check moved into `jira_title_for_backport()`, which has
one too. The `title_for_run()` and `jira_title_for_backport()` calls and both
`update_jira_for_pr` calls are still in `main()`; the mocked run kills the four
mutants there.
##########
dev/merge_spark_pr.py:
##########
@@ -670,6 +670,49 @@ def message_of(commit_hash):
return None, None
+def backport_jira_title(pr_num, message: str, title: str) -> str:
+ """Return the title that the JIRA update of a backport run reads.
+
+ The JIRA update follows what was committed, so it reads the subject of the
merge commit
+ ``message`` rather than the current PR title, which may have been edited
since the
+ merge. A changed subject is printed next to the PR title. The subject is
used only when
+ the merge footer of ``message`` closes ``pr_num``, i.e. when ``merge_pr``
made the
+ commit for this PR. Otherwise the PR title is used as before, e.g. for a
commit made
+ with the GitHub squash button, whose subject ends with ``(#N)``.
+
+ >>> footer = "\\n\\nBody\\n\\nCloses #1 from a/b.\\n\\nAuthored-by: A
<[email protected]>"
+ >>> backport_jira_title(1, 'Revert "[SPARK-1][SQL] Fix"' + footer,
+ ... 'Revert "[SPARK-1][SQL] Fix"')
+ 'Revert "[SPARK-1][SQL] Fix"'
+ >>> backport_jira_title(1, 'Revert "Revert "[SPARK-1][SQL] Fix"" \\r' +
footer,
+ ... 'Revert "[SPARK-1][SQL] Fix"')
+ ... # doctest: +NORMALIZE_WHITESPACE
+ The JIRA update uses the merge commit subject: Revert "Revert
"[SPARK-1][SQL] Fix""
+ (current PR title: Revert "[SPARK-1][SQL] Fix"). If the PR title is the
right one, update
+ JIRA by hand, answering N if asked to update it.
+ 'Revert "Revert "[SPARK-1][SQL] Fix""'
+
+ A commit without this PR's merge footer, such as a squash-button merge or
a commit of
+ another PR, leaves the PR title:
+
+ >>> backport_jira_title(1, 'Revert "[SPARK-1][SQL] Fix" (#1)\\n\\nBody',
+ ... 'Revert "[SPARK-1][SQL] Fix"')
+ 'Revert "[SPARK-1][SQL] Fix"'
+ >>> backport_jira_title("1", "[SPARK-2][SQL] Other" + footer.replace("#1",
"#2"),
+ ... "[SPARK-1][SQL] Fix")
+ '[SPARK-1][SQL] Fix'
+ """
+ subject = message.split("\n", 1)[0].strip()
+ if subject == title or not has_merge_footer(message, pr_num):
Review Comment:
Done, with both parts. `jira_title_for_backport()` passes the PR title to
`committed_title_notice()`, which takes it as GitHub has it, stripped, not the
prepared one, and compares the two with `jira_action(title)`, the decision
`update_jira_for_pr` now uses too: whether JIRA is skipped, and if not, which
tickets and in which direction. So a difference in components, text, or
normalization gives no notice, e.g. for 65396d2f8d2 with `[TESTS]`, or
`[SPARK-1][CORE] Fix` merged after `CORE` was entered at the prompt. Neither do
two titles that both skip JIRA, or that list the same ids in another order.
Since the PR title is no longer prepared on that path (see [22]), the notice
cannot show a title built at this run's prompt either. When the notice does
fire, the committer can choose the PR title for the JIRA update (see [24]).
This updates my replies to [10] (both), [17], [19], and [20]:
`jira_action()` is back in this PR, now as `jira_action(title)`, without
`is_revert` or a disabled case, which the callers check. It is the one place
that decides skip, ids, and direction for both the notice and the update, so
the skip logic, with the skip-tag comment from [4], is no longer inline in
`update_jira_for_pr`; `_bracketed()` is back as well, for the skip message, the
notice, and the two tag messages in `prepare_merge_title()` and the component
prompt. The pre-merge `jira` line stays in SPARK-60883. Within one call it
computes the ids once; on a backport, the subject goes through it twice, once
for the notice and once in `update_jira_for_pr`, which I kept so that
`update_jira_for_pr` still reads only its title ([17]).
##########
dev/merge_spark_pr.py:
##########
@@ -670,6 +670,49 @@ def message_of(commit_hash):
return None, None
+def backport_jira_title(pr_num, message: str, title: str) -> str:
+ """Return the title that the JIRA update of a backport run reads.
+
+ The JIRA update follows what was committed, so it reads the subject of the
merge commit
+ ``message`` rather than the current PR title, which may have been edited
since the
+ merge. A changed subject is printed next to the PR title. The subject is
used only when
+ the merge footer of ``message`` closes ``pr_num``, i.e. when ``merge_pr``
made the
+ commit for this PR. Otherwise the PR title is used as before, e.g. for a
commit made
+ with the GitHub squash button, whose subject ends with ``(#N)``.
+
+ >>> footer = "\\n\\nBody\\n\\nCloses #1 from a/b.\\n\\nAuthored-by: A
<[email protected]>"
+ >>> backport_jira_title(1, 'Revert "[SPARK-1][SQL] Fix"' + footer,
+ ... 'Revert "[SPARK-1][SQL] Fix"')
+ 'Revert "[SPARK-1][SQL] Fix"'
+ >>> backport_jira_title(1, 'Revert "Revert "[SPARK-1][SQL] Fix"" \\r' +
footer,
+ ... 'Revert "[SPARK-1][SQL] Fix"')
+ ... # doctest: +NORMALIZE_WHITESPACE
+ The JIRA update uses the merge commit subject: Revert "Revert
"[SPARK-1][SQL] Fix""
+ (current PR title: Revert "[SPARK-1][SQL] Fix"). If the PR title is the
right one, update
+ JIRA by hand, answering N if asked to update it.
+ 'Revert "Revert "[SPARK-1][SQL] Fix""'
+
+ A commit without this PR's merge footer, such as a squash-button merge or
a commit of
+ another PR, leaves the PR title:
+
+ >>> backport_jira_title(1, 'Revert "[SPARK-1][SQL] Fix" (#1)\\n\\nBody',
+ ... 'Revert "[SPARK-1][SQL] Fix"')
+ 'Revert "[SPARK-1][SQL] Fix"'
+ >>> backport_jira_title("1", "[SPARK-2][SQL] Other" + footer.replace("#1",
"#2"),
+ ... "[SPARK-1][SQL] Fix")
+ '[SPARK-1][SQL] Fix'
+ """
+ subject = message.split("\n", 1)[0].strip()
+ if subject == title or not has_merge_footer(message, pr_num):
+ return title
+ print(
Review Comment:
Done. The notice is now printed in the `finally`, right before
`update_jira_for_pr`, after the cherry-picks, and it no longer sends the
committer to N. It shows each title and what it would do, e.g. `resolve
SPARK-1, or add fix versions to it` against `remove fix versions from SPARK-1,
and offer to reopen it if resolved`, notes that a ticket resolved other than as
Fixed is left as is either way, and then asks `Update JIRA from the current PR
title instead of the merge commit subject? (y/N)`
(`jira_title_for_backport()`). The default keeps the subject; y uses the PR
title as is, apart from surrounding whitespace, so a wrong id fixed in the
title after the merge can still reach JIRA. It is shown only on a backport of a
commit with the PR's merge footer, since otherwise there is no committed
subject to compare with; `jira_title_for_backport()` checks that itself. When
JIRA is disabled for the run, the notice is printed without the question, so
the committer knows which title to recon
cile by hand.
Answering N at `Would you like to update an associated JIRA?` still ends the
run with an error, as on master. That prompt is shared with the merge path, and
nothing points the committer to N anymore, so I left it as is. The `Pull
request ... has already been merged` and `Found commit ...` lines, and the
check that the commit is local, also moved up, to right after the lookup, so a
backport is announced, and a missing commit fails, before the body prompt. This
updates my reply to [13]: the notice quoted there is gone, and a corrected PR
title can now be chosen at the new question instead of updating JIRA by hand.
##########
dev/merge_spark_pr.py:
##########
@@ -2446,6 +2568,102 @@ def prompt_for_components():
return components
+def prepare_merge_title(raw_title: str) -> str:
+ """Return the title to merge for the PR title ``raw_title``.
+
+ Surrounding whitespace is stripped. A ``Revert "..."`` / ``Reapply "..."``
title is
+ otherwise kept as is: the wrapped title is an existing commit subject,
which may
+ predate the current title rules, so it is not parsed and has no components
(see
+ :func:`check_wrapped_title` for what the merge path checks in it). Any
other title
+ is parsed with :meth:`Title.parse`, which fails on a malformed title, and
its
+ component tags are normalized, with a prompt for a primary component when
there is
+ none, and written back into the title, from which
:func:`update_jira_for_pr` reads
+ them. A title that changes is printed.
+ """
+ title = raw_title.strip()
+ if not _unwrap_wrappers(title)[1]:
+ # 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 = str(parsed)
+ if title != raw_title:
+ print("Normalized title: %s" % title)
+ return title
+
+
+def check_wrapped_title(title: str) -> None:
+ """Check the ``Revert "..."`` / ``Reapply "..."`` wrappers of a PR title
before merging.
+
+ An empty innermost title fails, since the commit subject would say
nothing. An
+ innermost title that still starts with ``Revert`` or ``Reapply``, or a
form of either
+ such as ``Reverting`` (see ``_WRAPPER_WORD_RE``), is warned about and
needs a
+ confirmation. Such a title may have a dropped quote, e.g. ``Revert "Revert
"[SPARK-N]
+ ..."``, or wrap a subject that is not a wrapper, e.g. ``Revert to using
...``. The
+ shape does not tell which: the revert of an existing subject that GitHub
truncated
+ without its closing quote looks like a dropped quote. Either way, only the
outer
+ wrappers decide the direction, and no JIRA id or skip tag is read from it.
Other
+ titles pass silently.
+ """
+ inner, words = _unwrap_wrappers(title)
+ if not words:
+ return
+ if not inner:
+ fail("Empty title inside the Revert/Reapply wrappers of the PR title:
%s" % title)
+ if _WRAPPER_WORD_RE.match(inner):
Review Comment:
Done, with your rule. `check_wrapped_title()` also warns, with the same
confirmation, when the innermost title starts with `"`, or with one word of
letters or hyphens followed by `"`, with at most one space between
(`_WRAPPER_LIKE_RE`). This covers `Revert ""[SPARK-1][SQL] X""`, `Revert
"Reaply "[SPARK-1][SQL] X""`, and `Revert "Reaply"[SPARK-1][SQL] X""`; the
doctest covers these shapes, `Reapply "Revet "...""`, and which of the two
warnings each title gets. A quote later in the title, e.g. `Revert
"[SPARK-1][SQL] Support "foo" in X"`, is not warned about.
The rule also catches a subject that starts with a quoted word or quotes one
after its first word. No master subject itself is warned about, but reverting
any of 21 existing subjects, e.g. `Rename "jobs" to "applications" in the
standalone cluster`, now asks for a confirmation, and the warning tells the
committer to keep such a title. Requiring the innermost title to also end with
`"` would cut that to 3, but would miss a misspelled wrapper whose closing
quote was dropped, so I kept your version. A misspelled wrapper that also lost
its quote, e.g. `Revert "Reaply [SPARK-1][SQL] X"`, is still not caught, since
nothing in it looks like a wrapper. The warning names the shape that matched, a
leading quote or one word and a quote. When JIRA is disabled for the run, both
warnings say what a JIRA update would do and to reconcile the ticket by hand,
instead of pointing to a JIRA prompt that does not come.
--
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]
