sarutak commented on code in PR #60031:
URL: https://github.com/apache/spark/pull/60031#discussion_r4471186106
##########
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:
This moved to SPARK-60883 with [20]. For the part here: `update_jira_for_pr`
now unwraps the title once and passes the innermost title to `leading_jira_ids`
and `leading_title_tags`, and computes the ids once for the skip check and the
update, as before this PR. It parses the title once more with `Title.parse` for
the components, which needs the wrapped title so that a Revert/Reapply title is
rejected and not reconciled, as before. I'll handle the double call of
`jira_action()` in SPARK-60883.
##########
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:
Agreed, they moved to SPARK-60883. This PR no longer changes
`format_jira_verification` or `check_linked_jira_issues`, and no longer has the
`jira` summary line, the `JIRA after the backport:` line, `jira_action()`,
`describe_jira_action()`, `jira_skip_tags()`, or `_bracketed()`. That also
takes out what I added here for [2], [7], [8], and [10], and the skip-tag
comment I moved to `jira_skip_tags()` for [4] is back in `update_jira_for_pr`.
##########
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:
Done. The description of how the wrappers are stripped is now in
`_unwrap_wrappers()`, and `_unwrap_title()` refers to it. `_is_net_revert()`,
`leading_jira_ids()`, and `leading_title_tags()` now point to
`_unwrap_wrappers()` too.
##########
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:
Update: since round 3 this is a warning with a confirmation on the merge
path, not a failure, and a backport does not run it; see [12] and [13].
--
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]
