sarutak commented on code in PR #60031:
URL: https://github.com/apache/spark/pull/60031#discussion_r4471183818
##########
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:
Done. Before merging, `check_wrapped_title()` fails with `Empty title inside
the Revert/Reapply wrappers of the PR title: ...` when the innermost title is
empty after stripping. The doctest covers `Revert ""`, `Revert " "`, `Reapply
""`, and `Revert "Revert """`.
##########
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 is moot now, with [17]: `update_jira_for_pr` derives the direction from
the title, and the helper passes only the title. What remains in `main()` is
which title it passes. On a backport that now comes from
`backport_jira_title()`, which has its own doctest (see [13]). No doctest
reaches `main()`, so none of the lines this PR changes there is pinned by one:
the `prepare_merge_title()` assignment, `check_wrapped_title()`,
`backport_jira_title()`, and both `update_jira_for_pr` calls. I checked them
with a mocked run of `main()` through the merge and the backport path, which
runs the real `update_jira_for_pr`. It kills the three mutants there that
survive the doctests: passing the raw PR title on a merge, which would drop a
component entered at the prompt; passing the PR title on a backport; and not
calling `check_wrapped_title()`.
##########
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:
Done, and the same for `title_components`, which is also determined by the
title: `prepare_merge_title()` writes the normalized components, including any
entered at the prompt, into the title it returns. `update_jira_for_pr(pr_num,
title, merge_branches)` now reads the ids, the skip tags, the direction
(`_is_net_revert` over the wrapper words), and the components from the title,
and `prepare_merge_title()` returns only the title, as does the new
`backport_jira_title()` (see [13]). On a merge, the reconciled components are
the same as before for every title I tried. `jira_action()` and
`describe_jira_action()` moved out with [20], so the summary line in that PR
will derive it from the title too.
##########
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:
This moved to SPARK-60883 with [20], since only the warning change uses
`is_wrapped`, and I'll handle it there. In this PR, after [17], every consumer
reads the wrappers from the title through `_unwrap_wrappers()`, so there is one
definition of the wrapper rules and no extra value threaded through `main()`.
--
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]
