sarutak commented on code in PR #60031:
URL: https://github.com/apache/spark/pull/60031#discussion_r4451553342
##########
dev/merge_spark_pr.py:
##########
@@ -1208,6 +1209,13 @@ def format_jira_verification(
a note instead of a warning so the expected divergence isn't flagged as a
likely wrong ticket. The Resolved/Closed warning still applies.
+ ``is_revert`` drops the Resolved/Closed warning for a ticket resolved as
Fixed, since
+ the change a revert undoes normally fixed its ticket. Any other resolution
is still
+ warned about, because the revert path cannot update such a ticket (see
+ :func:`get_jira_issue`). A revert of a revert re-lands the change and is
not a revert
+ here (see :func:`_is_revert_title`), so it keeps the warning: it usually
targets the
+ ticket that the revert reopened, so a ticket that is still resolved is
worth a look.
Review Comment:
You're right, and my reason was wrong: `revert_jira_issue` defaults to not
reopening, so after a typical revert the ticket is still Resolved (Fixed). I
took the first option. `format_jira_verification` now takes `is_wrapped`
instead of `is_revert` and drops the warning for any `Revert "..."` / `Reapply
"..."` title, in either direction, whose ticket is resolved as Fixed; other
resolutions keep it. `check_linked_jira_issues` derives `is_wrapped` from the
title's wrappers with `_unwrap_wrappers()`, as `prepare_merge_title()` does, so
it no longer takes the direction from `main()`. That updates my replies to [2],
[3], and [5]: the wrapped check is no longer only in `prepare_merge_title()`,
since `check_linked_jira_issues` computes it from the same title; `main()` now
uses the direction in four places, the two `update_jira_for_pr` calls and the
two JIRA summary lines; and a re-land of a ticket resolved as Fixed no longer
gets the warning. The doctest checks that a revert, a re-land (`
Revert "Revert "...""`), and a `Reapply` are passed on as wrapped and an
ordinary title is not, and the docstring now gives the reason above, including
that No is the default.
##########
dev/merge_spark_pr.py:
##########
@@ -1960,7 +2008,57 @@ 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):
+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")
+ # 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 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
+ return [tag for tag in skip_jira_title_tags if tag in tags]
+
+
+def describe_jira_action(title, is_revert):
+ """Say what :func:`update_jira_for_pr` will do, for the summary shown
before a push.
+
+ >>> describe_jira_action('[SPARK-1234][SQL] Fix', False)
+ 'resolve SPARK-1234 (or add fix versions if already resolved as Fixed)'
+ >>> describe_jira_action('Revert "[SPARK-1234][SQL] Fix"', True)
+ 'revert SPARK-1234 (remove fix versions; offer to reopen if resolved)'
+ >>> describe_jira_action('Revert "Fix"', True)
+ 'revert a ticket entered at the prompt (remove fix versions; offer to
reopen if resolved)'
+ >>> 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)'
+ """
+ if jira_disabled:
+ return "skip (JIRA is unreachable)"
+ skipped = jira_skip_tags(title)
+ if skipped:
+ return "skip (title has %s)" % ", ".join("[%s]" % tag for tag in
skipped)
+ ids = ", ".join(leading_jira_ids(title)) or "a ticket entered at the
prompt"
+ if is_revert:
+ return "revert %s (remove fix versions; offer to reopen if resolved)"
% ids
+ return "resolve %s (or add fix versions if already resolved as Fixed)" %
ids
Review Comment:
Done. Both strings now end with `a ticket resolved other than as Fixed is
left as is`, e.g. `revert SPARK-N (remove fix versions and offer to reopen if
resolved; a ticket resolved other than as Fixed is left as is)`. The summary
does not fetch the ticket, so it states the case instead of detecting it. On a
merge, the verification block above the line shows each linked ticket's status;
on a backport run, or for an id entered at the prompt, the status is printed
only after the push, when `get_jira_issue` fetches the ticket.
##########
dev/merge_spark_pr.py:
##########
@@ -2446,6 +2592,78 @@ 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_revert_title`), 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):
+ fail(
+ "Unbalanced Revert/Reapply wrappers in PR title: %s. Retitle
the PR so that "
+ 'each layer is Revert "..." or Reapply "..." around the
reverted title.' % title
+ )
Review Comment:
Good point. The message now uses your wording: `... Retitle the PR so that
each layer is Revert "..." or Reapply "...", e.g. Reapply "[SPARK-N] ..." for a
revert of a revert.`
##########
dev/merge_spark_pr.py:
##########
@@ -1960,7 +2008,57 @@ 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):
+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")
+ # 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 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
+ return [tag for tag in skip_jira_title_tags if tag in tags]
+
+
+def describe_jira_action(title, is_revert):
Review Comment:
Done. `jira_action(title, is_revert)` returns the decision as `(action,
detail)`: `("disabled", [])`, `("skip", tags)`, or `("revert"|"resolve", ids)`.
`update_jira_for_pr` acts on it and `describe_jira_action` only formats it, so
the two can no longer decide differently. The skip tags are also formatted by
one helper, `_bracketed()`, for both messages. The doctests fail if
`jira_action` ignores `is_revert` or the skip tags, if `update_jira_for_pr`
takes the resolve path for a `"revert"` action, or if the `jira` line shows a
revert as a resolve.
##########
dev/merge_spark_pr.py:
##########
@@ -2446,6 +2592,78 @@ 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_revert_title`), 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):
+ fail(
+ "Unbalanced Revert/Reapply wrappers in PR title: %s. Retitle
the PR so that "
+ 'each layer is Revert "..." or Reapply "..." around the
reverted title.' % title
+ )
+ title_components: list[str] = []
+ is_revert = _is_revert_title(title)
Review Comment:
Done. `_is_revert_title(title)` is now `_is_net_revert(words)`, and
`prepare_merge_title` passes the words it already has. The parity table in its
doctest still lists titles, unwrapped with `_unwrap_wrappers()` first.
##########
dev/merge_spark_pr.py:
##########
@@ -2283,6 +2285,24 @@ 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.
+
+ >>> _unwrap_wrappers('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ ('[SPARK-1234][SQL] Fix', ['Revert', 'Reapply'])
+ >>> _unwrap_wrappers('[SPARK-1234][SQL] Fix')
+ ('[SPARK-1234][SQL] Fix', [])
+ """
+ title = title.strip()
+ words = []
+ while m := _WRAPPER_RE.fullmatch(title):
Review Comment:
Follow-up to my reply above: the check now also rejects forms of the words,
e.g. `Revert "Reverted [SPARK-1234][SQL] Fix"`, which my reply said was
accepted. For such a title the wrappers cannot tell the direction and there is
no leading JIRA id, so without the check it would take the revert path with an
empty id prompt. Seven master subjects open with such a form (e.g. `Reverting
to using comma-delimited split`), so reverting any of 56 existing subjects (was
49) now gives a rejected title. Still no master subject itself is rejected.
--
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]
