dongjoon-hyun commented on code in PR #59708:
URL: https://github.com/apache/spark/pull/59708#discussion_r4371925624
##########
dev/merge_spark_pr.py:
##########
@@ -2853,6 +2949,26 @@ def main():
... resolver.assert_not_called()
... confirm.assert_not_called()
+ Reapply titles are unwrapped too, so their skip tags apply as well.
+
+ >>> for title in ('Reapply "[MINOR][SQL] Fix"', 'Reapply
"[SPARK-1234][SQL][FOLLOWUP] Fix"'):
+ ... update(title, ["master"])
+ ... reverter.assert_not_called()
+ ... resolver.assert_not_called()
+ ... confirm.assert_not_called()
+
+ Nested wrappers are unwrapped before the skip-tag and id checks, so an
inner
+ skip tag is still honored and an inner id still reaches the revert path. A
+ revert of a reapply is used here because it is a revert both by its
outermost
+ wrapper, which is how main() sets is_revert today, and by wrapper nesting.
+
+ >>> update('Revert "Reapply "[MINOR][SQL] Fix""', ["master"],
is_revert=True)
+ >>> reverter.assert_not_called()
+ >>> resolver.assert_not_called()
+ >>> confirm.assert_not_called()
+ >>> update('Revert "Reapply "[SPARK-1234][SQL] Fix""', ["master"],
is_revert=True)
+ >>> reverter.assert_called_once_with(["master"], "SPARK-1234")
+
Ordinary merges and reapplies still resolve the issue or add fix versions.
>>> for title in ('[SPARK-1234][SQL] Fix', 'Reapply "[SPARK-1234][SQL]
Fix"'):
Review Comment:
Minor: no doctest now checks the id that `resolve_jira_issues` extracts
(L1871), which is where my item 1 regressed. This loop mocks
`resolve_jira_issues` as a whole (L2922-L2923), so it only checks that the
wrapped title is passed on. The `leading_jira_ids('Reapply "[SPARK-1234][SQL]
Fix"')` doctest added for my item 5 was dropped in 205a17f with the examples
that repeat `_unwrap_title` inputs. And since my item 15, L1894 passes the
already-unwrapped `jira_title`, so the `update_jira_for_revert` cases no longer
reach the unwrap in `leading_jira_ids` with a wrapper either. As a result, if
L2241 unwrapped only `Revert "..."` (or only one layer), every doctest would
still pass, while a `Reapply "[SPARK-1234][SQL] Fix"` PR would again skip the
Epic/Umbrella blocker and the match prompt in `main()` and get `Enter a JIRA id
[]:` on merge. The #59587 case, a body id on an ordinary merge, is not checked
on this path either. Could you patch `resolve_jira_issue` instead of
`resolve_jira_is
sues` in `update()`, and assert for both titles here that `"SPARK-1234"`
reaches it, e.g. `resolver.assert_called_once_with(["master"], "Issue resolved
by pull request 123\\n[https://github.com/apache/spark/pull/123]";,
"SPARK-1234", title_components=[])`? A body reference such as `Fix mentions
SPARK-9999` in the first title would cover #59587 as well. Restoring the
dropped example would be the minimal alternative.
##########
dev/merge_spark_pr.py:
##########
@@ -1888,7 +1888,10 @@ def resolve_jira_issues(title, merge_branches, comment,
title_components=()):
def update_jira_for_pr(pr_num, title, merge_branches, title_components,
is_revert=False):
skip_jira_title_tags = ("MINOR", "TRIVIAL", "FOLLOWUP")
tags = set(title_components)
- jira_title = title[len('Revert "') : -1] if is_revert else title
+ # Revert/Reapply PRs keep their title verbatim, so unwrap before parsing
to see the
+ # wrapped id, markers, and components.
+ jira_title = _unwrap_title(title)
+ jira_ids = leading_jira_ids(jira_title)
try:
parsed = Title.parse(jira_title)
tags.update(parsed.leading)
Review Comment:
**A wrapped `[FOLLOW-UP]` is not skipped.** `[FOLLOW-UP]` is a registered
alias of FOLLOWUP (`Component("FOLLOWUP", ("FOLLOW-UP",))`, L2018), and
`main()` maps it to FOLLOWUP for ordinary titles (L2487-L2491). A
Revert/Reapply title keeps `title_components == []`, though, and the components
added on L1898 are compared raw against `skip_jira_title_tags` (L1901). With
this code, #54151's title, `Revert "[SPARK-55313][PYTHON][FOLLOW-UP] Only add
condabin to PATH for pip tests"`, would not be skipped: the script asks to
update JIRA and calls `revert_jira_issue(..., "SPARK-55313")`, which offers to
remove the parent ticket's fix versions and to reopen it, although only the
follow-up was reverted. The same title with `[FOLLOWUP]` is skipped, and so is
a direct merge of the `[FOLLOW-UP]` title. For `Revert` this predates this PR
(SPARK-59808). But this PR extends the skip to `Reapply` and nested titles, and
the description says that a `Reapply` or a revert of a revert of a `[FOLLOWUP]`
c
hange now skips the JIRA update, while `Reapply "...[FOLLOW-UP]..."` still
offers to add fix versions to the parent ticket. Titles have been normalized at
merge time since SPARK-56979, but 17 of the 617 wrapped subjects in master's
history use `[FOLLOW-UP]` (22 use `[FOLLOWUP]`), and about 1,200 older commits
carry it. Could you normalize the components on L1898 as `main()` does, e.g.
`tags.update(c.tag if (c := Component.find(t)) else t for t in
parsed.components)`, and add `'Revert "[SPARK-1234][SQL][FOLLOW-UP] Fix"'` to
the skip-tag loop? A follow-up would also be fine, since `Revert` titles
already behave this way on master.
##########
dev/merge_spark_pr.py:
##########
@@ -2023,6 +2025,28 @@ def find(cls, token):
_SPARK_ID_RE = re.compile(r"^SPARK-\d+$", re.IGNORECASE)
_VERSION_TAG_RE = re.compile(r"^\d+\.(\d+|X)$")
_LEADING_TAGS = frozenset({"MINOR", "TRIVIAL"})
+# A SPARK id that opens a title without brackets, e.g. the old subject
"SPARK-49004".
+_BARE_SPARK_ID_RE = re.compile(r"SPARK-[0-9]+\b", re.IGNORECASE | re.ASCII)
+# A Revert "..." / Reapply "..." wrapper, which main() keeps verbatim in the
PR title.
+_WRAPPER_RE = re.compile(r'(?:Revert|Reapply) "(.*)"', re.DOTALL)
+
+
+def _iter_bracket_tags(raw: str) -> Iterator[Tuple[str, int]]:
Review Comment:
Nit: `Tuple` is used only here. `Iterator[tuple[str, int]]` would match
`parse_version`'s `Optional[tuple[int, int, int]]` (L140), the file's only
other tuple annotation, and keep the import on L47 as `from typing import
Iterator, List, Optional`.
##########
dev/merge_spark_pr.py:
##########
@@ -2843,6 +2937,8 @@ def main():
>>> reverter.assert_called_once_with(["branch-4.2"], "SPARK-1234")
>>> update('Revert "Fix"', ["master"], is_revert=True)
>>> reverter.assert_called_once_with(["master"], "")
+ >>> update('Revert "SPARK-49004"', ["master"], is_revert=True)
Review Comment:
Nit: this case runs on `master` and finds its id, so neither sentence of the
paragraph above it (L2934, "Backport-only runs use the same revert path. A
missing JIRA id is entered interactively.") describes it. Could you add a
sentence for it, e.g. "A bare leading id, as in the old subject SPARK-49004,
still reaches the revert path."? While here, the opening sentence on L2911,
"Reverts update each referenced JIRA once ...", no longer fits the first case,
where the referenced SPARK-9999 is deliberately skipped; "each leading JIRA id"
would.
--
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]
