dongjoon-hyun commented on code in PR #60031:
URL: https://github.com/apache/spark/pull/60031#discussion_r4451350173
##########
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:
**[7] A re-land still warns about a ticket resolved as Fixed.**
This goes back to your reply to [2], where a re-land is treated like a fresh
merge on purpose. The reason given here doesn't match the revert path, though.
`revert_jira_issue` asks `Reopen JIRA issue %s so the change can be reworked?
(y/N)`, and the default is N. So after a typical revert the ticket stays
Resolved (Fixed), with only its fix versions removed, and that is the state a
later `Revert "Revert "[SPARK-N] ...""` finds:
- `check_linked_jira_issues` shows `Resolved (Fixed) <-- WARNING: already
Resolved/Closed` in red.
- The `jira` line right after it says `resolve SPARK-N (or add fix versions
if already resolved as Fixed)`, and the resolve path does add the fix versions
back.
So on the default path the warning flags the expected state. The warning
itself is not new, since `Reapply "..."` titles got it before this PR as well.
Now that the direction is decided here, though, the Fixed exemption could cover
a re-land too, for example any title with wrappers, while other resolutions
keep the warning. If you'd rather keep the warning, could this docstring say
that the ticket is often still Resolved (Fixed) after a revert, instead of that
the revert usually reopened it?
##########
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:
**[9] The unbalanced-title error could name the `Reapply` form.**
When the title comes from reverting an old unquoted revert, such as `Revert
"Revert [SPARK-54758][SQL] ..."`, the reverted title has no quotes to begin
with. The message's `each layer is Revert "..." or Reapply "..." around the
reverted title` then has no exact answer, and the likely retitle, `Revert
"Revert "[SPARK-54758][SQL] ...""`, adds quotes that the reverted subject never
had. For a revert of a revert, `Reapply "[SPARK-54758][SQL] ..."` is the
accurate title, and it is the form git itself uses for a revert of `Revert
"X"`. Could the message suggest it? For example:
```python
fail(
"Unbalanced Revert/Reapply wrappers in PR title: %s. Retitle
the PR so that "
'each layer is Revert "..." or Reapply "...", e.g. Reapply
"[SPARK-N] ..." '
"for a revert of a revert." % title
)
```
##########
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:
**[10] `describe_jira_action` repeats the decisions of
`update_jira_for_pr`.**
This function and `update_jira_for_pr` now go through the same steps
separately: `jira_disabled`, then `jira_skip_tags`, then `is_revert`, then the
fallback to an id entered at the prompt. A later change to one of them, such as
a new skip reason or a different behavior without an id, can leave the `jira`
line promising something other than what runs after the push, and no doctest
compares the two. One way to keep them in step is a single helper that returns
the decision, used by both:
```python
def jira_action(title, is_revert):
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)
```
Not blocking, since both are small today.
##########
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:
**[11] `prepare_merge_title` unwraps the title twice.**
`words` from L2609 already holds the wrapper words, and
`_is_revert_title(title)` unwraps the same title again to count them. That
shape comes from my suggestion in [3], but now that this branch has `words`, a
helper that takes the words would let it reuse them:
```python
def _is_net_revert(words: list[str]) -> bool:
return words.count("Revert") % 2 == 1
```
with `is_revert = _is_net_revert(words)` here. A small nit, feel free to
ignore.
##########
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:
**[8] The `jira` line promises an update for a ticket with another
resolution.**
For a ticket that is Resolved or Closed with a resolution other than Fixed,
`get_jira_issue` prints `already has status ...` and returns None, so neither
path updates it, as your reply to [2] says. The `jira` line still promises an
update, though. For example, a revert of a ticket resolved as Duplicate shows
```
Status: Resolved (Duplicate) <-- WARNING: already Resolved/Closed
...
jira revert SPARK-N (remove fix versions; offer to reopen if resolved)
```
and nothing happens to SPARK-N after the push. The resolve path is the same:
`resolve SPARK-N (or add fix versions if already resolved as Fixed)` also
leaves a Duplicate ticket as it is. Since this line is there to show the
outcome before the push, could both strings mention that case? A wording change
would be enough, e.g. `...; a ticket resolved other than as Fixed is left as
is`.
--
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]
