dongjoon-hyun commented on code in PR #60031:
URL: https://github.com/apache/spark/pull/60031#discussion_r4470227639
##########
dev/merge_spark_pr.py:
##########
@@ -2446,6 +2636,79 @@ def prompt_for_components():
return components
+def prepare_merge_title(raw_title: str) -> tuple[str, list[str], bool]:
+ """Return ``(title, title_components, is_revert)`` for the title of the PR
to merge.
+
+ 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. It is
+ rejected when the innermost title still starts with ``Revert`` or
``Reapply``, e.g.
+ with a quote dropped: that wrapper would not be counted when deciding the
direction
+ (see :func:`_is_net_revert`), and its JIRA id and skip tags would not be
found
+ either. 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. A title that changes is printed.
+ """
+ title = raw_title.strip()
+ inner, words = _unwrap_wrappers(title)
+ if words:
+ if _WRAPPER_WORD_RE.match(inner):
Review Comment:
This rejects any title whose innermost text merely starts with the word
`Revert` or `Reapply`, not only a wrapper with a dropped quote. For example, a
git default revert of a commit titled `Revert to using comma-delimited split`
gives `Revert "Revert to using comma-delimited split"`, which now fails with no
way around it. The retitle suggested by the error message (`Reapply "..."`)
would actually flip the direction to resolve in that case. The PR description
counts 56 existing subjects that would be rejected this way.
Could this be narrowed to the dropped-quote case, or turned into a warning
with a confirmation prompt (`continue_maybe`) instead of `fail()`?
##########
dev/merge_spark_pr.py:
##########
@@ -2446,6 +2636,79 @@ def prompt_for_components():
return components
+def prepare_merge_title(raw_title: str) -> tuple[str, list[str], bool]:
+ """Return ``(title, title_components, is_revert)`` for the title of the PR
to merge.
+
+ 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. It is
+ rejected when the innermost title still starts with ``Revert`` or
``Reapply``, e.g.
+ with a quote dropped: that wrapper would not be counted when deciding the
direction
+ (see :func:`_is_net_revert`), and its JIRA id and skip tags would not be
found
+ either. 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. A title that changes is printed.
+ """
+ title = raw_title.strip()
+ inner, words = _unwrap_wrappers(title)
Review Comment:
An empty innermost title is accepted: `Revert ""` and `Revert " "` both
return `('Revert ""', [], True)` (resp. `'Revert " "'`), so the merge goes
ahead with a meaningless subject and takes the revert path with the JIRA id
prompt. Should an empty `inner` be rejected here as well, e.g. `if words and
not inner: fail(...)`?
##########
dev/merge_spark_pr.py:
##########
@@ -3060,24 +3275,25 @@ def main():
... resolve_jira_issue=resolver,
continue_maybe=confirm),
... redirect_stdout(StringIO()),
... ):
- ... update_jira_for_pr("123", title, branches, list(components),
is_revert=is_revert)
+ ... update_jira_for_pr("123", title, branches, list(components),
+ ...
is_revert=_is_net_revert(_unwrap_wrappers(title)[1]))
Review Comment:
This helper recomputes `is_revert` from the title, so the doctest exercises
`update_jira_for_pr()` but not the path that `main()` actually uses
(`prepare_merge_title()` -> `is_revert_pr` -> `update_jira_for_pr(...,
is_revert=is_revert_pr)`). Passing the wrong variable at one of the call sites
in `main()` would still pass these tests. Using `prepare_merge_title(title)[2]`
here would at least tie the test to the production computation (or this becomes
moot with the suggestion on `update_jira_for_pr`'s signature).
##########
dev/merge_spark_pr.py:
##########
@@ -1960,41 +2008,124 @@ def resolve_jira_issues(jira_ids, merge_branches,
comment, title_components=()):
)
-def update_jira_for_pr(pr_num, title, merge_branches, title_components,
is_revert=False):
- if jira_disabled:
- # JIRA was disabled at startup (unreachable); nothing to update.
- print()
- print_error(
- f"JIRA is disabled; skipping JIRA update for PR #{pr_num}. "
- f"Reconcile the ticket(s) manually."
- )
- return
+def jira_skip_tags(title):
+ """The title tags that make :func:`update_jira_for_pr` skip the JIRA
update.
+ >>> jira_skip_tags('Revert "[MINOR][SQL] Fix"')
+ ['MINOR']
+ >>> jira_skip_tags('[SPARK-1234][SQL][FOLLOWUP] Fix')
+ ['FOLLOWUP']
+ >>> jira_skip_tags('[SPARK-1234][SQL] Fix')
+ []
+ """
skip_jira_title_tags = ("MINOR", "TRIVIAL", "FOLLOWUP")
- jira_ids = leading_jira_ids(title)
- # Revert/Reapply PRs keep their title verbatim, so the tags are read
inside the wrappers,
+ # Revert/Reapply PRs keep their title as is, so the tags are read inside
the wrappers,
# without Title.parse, which rejects some older titles such as
[SPARK-N][FOLLOWUP][MINOR].
tags = leading_title_tags(title)
- if jira_ids:
+ if leading_jira_ids(title):
# Title.parse rejects an id next to [MINOR] or [TRIVIAL], and
prompt_for_components()
# rejects both markers, so only a subject that predates the rule or
was not merged with
# this script has both. Such a commit was normally merged as the fix
for that id, so
# the id is updated.
tags -= _LEADING_TAGS
- skipped = [tag for tag in skip_jira_title_tags if tag in tags]
+ return [tag for tag in skip_jira_title_tags if tag in tags]
+
+
+def _bracketed(tags):
+ """Format tags as in a title, e.g. ``[MINOR], [FOLLOWUP]``, for JIRA
messages."""
+ return ", ".join("[%s]" % tag for tag in tags)
+
+
+def jira_action(title, is_revert):
+ """Decide what :func:`update_jira_for_pr` does, as ``(action, detail)``.
+
+ ``action`` is ``"disabled"`` when JIRA is unreachable, ``"skip"`` when the
title has
+ skip tags (``detail`` lists them), and otherwise ``"revert"`` or
``"resolve"``, with
+ the leading JIRA ids as ``detail``; with no id, the committer enters one
at the
+ prompt. :func:`describe_jira_action` shows the same decision before the
push.
+
+ >>> jira_action('[SPARK-1234][SPARK-5678][SQL] Fix', False)
+ ('resolve', ['SPARK-1234', 'SPARK-5678'])
+ >>> jira_action('Revert "Fix"', True)
+ ('revert', [])
+ >>> jira_action('Revert "[MINOR][SQL] Fix"', True)
+ ('skip', ['MINOR'])
+ >>> from unittest.mock import patch
+ >>> with patch.dict(jira_action.__globals__, jira_disabled=True):
+ ... jira_action('[SPARK-1234][SQL] Fix', False)
+ ('disabled', [])
+ """
+ if jira_disabled:
+ return "disabled", []
+ skipped = jira_skip_tags(title)
if skipped:
+ return "skip", skipped
+ return ("revert" if is_revert else "resolve"), leading_jira_ids(title)
Review Comment:
Minor: `jira_skip_tags()` already calls `leading_title_tags()` and
`leading_jira_ids()`, and `leading_jira_ids(title)` is called again here.
`jira_action()` itself runs from both `describe_jira_action()` and
`update_jira_for_pr()`, so the same title is unwrapped and parsed several
times. It is cheap, but reusing the ids, or computing the decision once and
passing it along, would be simpler.
##########
dev/merge_spark_pr.py:
##########
@@ -1255,10 +1263,46 @@ def format_jira_verification(
Status: In Progress
Type: Bug
Match: 0.00 (FOLLOWUP: title intentionally differs, not checked)
+
+ A Revert/Reapply title normally targets a ticket resolved as Fixed, so
that is not
+ warned about, while another resolution still is:
+
+ >>> print(format_jira_verification(
+ ... 42,
+ ... 'Revert "[SPARK-1][SQL] Add a cache"',
+ ... "SPARK-1",
+ ... "Add a cache",
+ ... "Resolved",
+ ... "Bug",
+ ... resolution="Fixed",
+ ... is_wrapped=True,
+ ... ))
+ === Verify JIRA matches PR #42 ===
+ PR title: Revert "[SPARK-1][SQL] Add a cache"
+ JIRA SPARK-1: Add a cache
+ Status: Resolved (Fixed)
+ Type: Bug
+ Match: 0.50
+ >>> print(format_jira_verification(
+ ... 42,
+ ... 'Revert "[SPARK-1][SQL] Add a cache"',
+ ... "SPARK-1",
+ ... "Add a cache",
+ ... "Resolved",
+ ... "Bug",
+ ... resolution="Duplicate",
+ ... is_wrapped=True,
+ ... ))
+ === Verify JIRA matches PR #42 ===
+ PR title: Revert "[SPARK-1][SQL] Add a cache"
+ JIRA SPARK-1: Add a cache
+ Status: Resolved (Duplicate) <-- WARNING: already Resolved/Closed
+ Type: Bug
+ Match: 0.50
"""
status_warning = ""
resolution_suffix = " (%s)" % resolution if resolution else ""
- if status in ("Resolved", "Closed"):
+ if status in ("Resolved", "Closed") and not (is_wrapped and resolution ==
"Fixed"):
Review Comment:
The suppression of the Resolved/Closed warning for wrapped titles here, and
the new `jira` summary line in `main()`, look independent of the parity fix in
SPARK-60814. Would you consider moving them to a separate JIRA/PR? Narrowing a
pre-merge safety warning would be easier to review, and to revert if needed, on
its own.
##########
dev/merge_spark_pr.py:
##########
@@ -2283,16 +2418,35 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_wrappers(title: str) -> tuple[str, list[str]]:
+ """Return the innermost title and the wrapper words
(``Revert``/``Reapply``), outermost first.
+
+ See :func:`_unwrap_title` for how the wrappers are stripped.
Review Comment:
The implementation now lives in `_unwrap_wrappers()`, but this docstring
points to `_unwrap_title()`, which is now a one-line delegate, for how the
wrappers are stripped. Moving that description here and having
`_unwrap_title()` refer to this function would keep the docs next to the code.
##########
dev/merge_spark_pr.py:
##########
@@ -1960,41 +2008,124 @@ def resolve_jira_issues(jira_ids, merge_branches,
comment, title_components=()):
)
-def update_jira_for_pr(pr_num, title, merge_branches, title_components,
is_revert=False):
- if jira_disabled:
- # JIRA was disabled at startup (unreachable); nothing to update.
- print()
- print_error(
- f"JIRA is disabled; skipping JIRA update for PR #{pr_num}. "
- f"Reconcile the ticket(s) manually."
- )
- return
+def jira_skip_tags(title):
+ """The title tags that make :func:`update_jira_for_pr` skip the JIRA
update.
+ >>> jira_skip_tags('Revert "[MINOR][SQL] Fix"')
+ ['MINOR']
+ >>> jira_skip_tags('[SPARK-1234][SQL][FOLLOWUP] Fix')
+ ['FOLLOWUP']
+ >>> jira_skip_tags('[SPARK-1234][SQL] Fix')
+ []
+ """
skip_jira_title_tags = ("MINOR", "TRIVIAL", "FOLLOWUP")
- jira_ids = leading_jira_ids(title)
- # Revert/Reapply PRs keep their title verbatim, so the tags are read
inside the wrappers,
+ # Revert/Reapply PRs keep their title as is, so the tags are read inside
the wrappers,
# without Title.parse, which rejects some older titles such as
[SPARK-N][FOLLOWUP][MINOR].
tags = leading_title_tags(title)
- if jira_ids:
+ if leading_jira_ids(title):
# Title.parse rejects an id next to [MINOR] or [TRIVIAL], and
prompt_for_components()
# rejects both markers, so only a subject that predates the rule or
was not merged with
# this script has both. Such a commit was normally merged as the fix
for that id, so
# the id is updated.
tags -= _LEADING_TAGS
- skipped = [tag for tag in skip_jira_title_tags if tag in tags]
+ return [tag for tag in skip_jira_title_tags if tag in tags]
+
+
+def _bracketed(tags):
+ """Format tags as in a title, e.g. ``[MINOR], [FOLLOWUP]``, for JIRA
messages."""
+ return ", ".join("[%s]" % tag for tag in tags)
+
+
+def jira_action(title, is_revert):
+ """Decide what :func:`update_jira_for_pr` does, as ``(action, detail)``.
+
+ ``action`` is ``"disabled"`` when JIRA is unreachable, ``"skip"`` when the
title has
+ skip tags (``detail`` lists them), and otherwise ``"revert"`` or
``"resolve"``, with
+ the leading JIRA ids as ``detail``; with no id, the committer enters one
at the
+ prompt. :func:`describe_jira_action` shows the same decision before the
push.
+
+ >>> jira_action('[SPARK-1234][SPARK-5678][SQL] Fix', False)
+ ('resolve', ['SPARK-1234', 'SPARK-5678'])
+ >>> jira_action('Revert "Fix"', True)
+ ('revert', [])
+ >>> jira_action('Revert "[MINOR][SQL] Fix"', True)
+ ('skip', ['MINOR'])
+ >>> from unittest.mock import patch
+ >>> with patch.dict(jira_action.__globals__, jira_disabled=True):
+ ... jira_action('[SPARK-1234][SQL] Fix', False)
+ ('disabled', [])
+ """
+ if jira_disabled:
+ return "disabled", []
+ skipped = jira_skip_tags(title)
if skipped:
+ return "skip", skipped
+ return ("revert" if is_revert else "resolve"), leading_jira_ids(title)
+
+
+def describe_jira_action(title, is_revert):
+ """Say what :func:`update_jira_for_pr` will do (see :func:`jira_action`),
before a push.
+
+ Both JIRA paths leave a ticket that is resolved other than as Fixed as it
is (see
+ :func:`get_jira_issue`), which the summary cannot tell without fetching
the ticket,
+ so it says so.
+
+ >>> print(describe_jira_action('[SPARK-1234][SQL] Fix', False))
+ ... # doctest: +NORMALIZE_WHITESPACE
+ resolve SPARK-1234 (or add fix versions if already resolved as Fixed;
+ a ticket resolved other than as Fixed is left as is)
+ >>> print(describe_jira_action('Revert "[SPARK-1234][SQL] Fix"', True))
+ ... # doctest: +NORMALIZE_WHITESPACE
+ revert SPARK-1234 (remove fix versions and offer to reopen if resolved;
+ a ticket resolved other than as Fixed is left as is)
+ >>> print(describe_jira_action('Revert "Fix"', True))
+ ... # doctest: +NORMALIZE_WHITESPACE
+ revert a ticket entered at the prompt (remove fix versions and offer to
reopen if resolved;
+ a ticket resolved other than as Fixed is left as is)
+ >>> describe_jira_action('Revert "[MINOR][SQL] Fix"', True)
+ 'skip (title has [MINOR])'
+ >>> from unittest.mock import patch
+ >>> with patch.dict(describe_jira_action.__globals__, jira_disabled=True):
+ ... describe_jira_action('[SPARK-1234][SQL] Fix', False)
+ 'skip (JIRA is unreachable)'
+ """
+ action, detail = jira_action(title, is_revert)
+ if action == "disabled":
+ return "skip (JIRA is unreachable)"
+ if action == "skip":
+ return "skip (title has %s)" % _bracketed(detail)
+ ids = ", ".join(detail) or "a ticket entered at the prompt"
+ other = "a ticket resolved other than as Fixed is left as is"
+ if action == "revert":
+ return "revert %s (remove fix versions and offer to reopen if
resolved; %s)" % (ids, other)
+ return "resolve %s (or add fix versions if already resolved as Fixed; %s)"
% (ids, other)
+
+
+def update_jira_for_pr(pr_num, title, merge_branches, title_components, *,
is_revert):
Review Comment:
`is_revert` is fully determined by `title`: it is
`_is_net_revert(_unwrap_wrappers(title)[1])`, and `prepare_merge_title()` sets
it to `False` for an unwrapped title, which is the same as
`_is_net_revert([])`. Yet it is threaded from `prepare_merge_title()` through
`main()` into `describe_jira_action()` and `update_jira_for_pr()` as a required
keyword argument.
Computing it inside `jira_action()` would drop the parameter, remove any
chance of the call sites disagreeing (including a missing argument surfacing as
a `TypeError` only after the push, as the PR description mentions), and the
doctest helper would not need to duplicate the computation.
##########
dev/merge_spark_pr.py:
##########
@@ -2137,8 +2268,12 @@ def normalize(cls, token):
_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)
+# A Revert "..." / Reapply "..." wrapper, which prepare_merge_title() keeps in
the PR title.
+_WRAPPER_RE = re.compile(r'(Revert|Reapply) "(.*)"', re.DOTALL)
+# Revert or Reapply, or a form of either such as Reverting, still opening a
title after
+# unwrapping, e.g. 'Revert [SPARK-N] ...'. Case is ignored, as older subjects
include
+# 'revert [SPARK-N] ...' and 'Reverting ...'.
+_WRAPPER_WORD_RE = re.compile(r"Revert|Reapply", re.IGNORECASE)
Review Comment:
Since `_WRAPPER_WORD_RE.match` is a prefix match on `Revert|Reapply`,
`Reverted`, `Reverting` and `Reapplying` are caught, but `Reapplied`,
`Reapplies` and `Re-apply` are not. For example, `Revert "Reapplied [SPARK-1]
x"` passes the check and `jira_action()` returns `('revert', [])`, i.e. the
revert path without a JIRA id, which is what this check is meant to prevent.
Something like `Revert|Re-?appl` would handle these forms consistently.
##########
dev/merge_spark_pr.py:
##########
@@ -2658,61 +2921,9 @@ 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_components are the normalized PR-title component tags, used later
to reconcile
+ # JIRA components.
+ title, title_components, is_revert_pr = prepare_merge_title(title)
Review Comment:
`prepare_merge_title()` runs before the already-merged check, so on a
backport both the direction and the new unbalanced check come from the current
GitHub PR title, not from the merge commit (`message` from
`find_merge_commit()`). If the title was edited after the merge, or a PR merged
with the previous script has a title that is now considered unbalanced, the
backport run either aborts here or updates JIRA in the opposite direction of
what was actually committed (removing fix versions vs. resolving).
Would it make sense to derive the direction from the merge commit subject in
the backport path, or at least not `fail()` there?
##########
dev/merge_spark_pr.py:
##########
@@ -1347,11 +1391,14 @@ def check_linked_jira_issues(pr_num, title):
# reference the wrong ticket -- e.g. mistaking a GitHub PR number in the
body
# ("Closes #NNNNN") for a SPARK JIRA id -- which resolves an unrelated
ticket on merge.
# Show the PR title next to each ticket's summary, warn when a ticket is
already
- # Resolved/Closed (a fresh merge should target an open one), and require
confirmation.
+ # Resolved/Closed (a fresh merge should target an open one, while a
Revert/Reapply PR
+ # normally targets one resolved as Fixed, see format_jira_verification),
and require
+ # confirmation.
# For [FOLLOWUP] PRs the title describes the follow-up fix rather than the
ticket, so
# its low similarity to the JIRA summary is expected and the warning is
suppressed.
# The tag is read inside Revert/Reapply wrappers too.
is_followup = "FOLLOWUP" in leading_title_tags(title)
+ is_wrapped = bool(_unwrap_wrappers(title)[1])
Review Comment:
`prepare_merge_title()` has already computed the wrapper words, and this
unwraps the title again. If the wrapper rules change later, the warning
suppression here and the direction decision there could drift apart. Maybe
`prepare_merge_title()` could return `is_wrapped` (or the words) and pass it in?
--
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]
