dongjoon-hyun commented on code in PR #59708:
URL: https://github.com/apache/spark/pull/59708#discussion_r4419144117
##########
dev/merge_spark_pr.py:
##########
@@ -1905,14 +1971,17 @@ def update_jira_for_pr(pr_num, title, merge_branches,
title_components, is_rever
return
skip_jira_title_tags = ("MINOR", "TRIVIAL", "FOLLOWUP")
- tags = set(title_components)
- jira_title = title[len('Revert "') : -1] if is_revert else title
- try:
- parsed = Title.parse(jira_title)
- tags.update(parsed.leading)
- tags.update(parsed.components)
- except ValueError:
- pass
+ jira_ids = leading_jira_ids(title)
+ # Revert/Reapply PRs keep their title verbatim, 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:
Review Comment:
**A revert or reapply of a commit whose `[MINOR]` was entered at the
component prompt updates the ticket that its merge skipped.** The comment above
names two sources of an id next to a marker, older subjects and the component
prompt, but only the first one is merged as the fix for that id. The prompt
produces exactly the subject this rule updates:
1. Merging `[SPARK-1234] Fix`, which has no primary component, the committer
enters `SQL,MINOR` at the prompt (L2662). `main()` normalizes the title to
`[SPARK-1234][SQL][MINOR] Fix` (L2686-L2688), commits it with that subject
(L858), and skips the JIRA update because `title_components` has `MINOR`
(L1984). This is the doctest at L3080.
2. Reverting that commit opens `Revert "[SPARK-1234][SQL][MINOR] Fix"`. Its
`title_components` is `[]` (L2643), so L1982 drops `MINOR` and the script calls
`revert_jira_issue(..., "SPARK-1234")`. This is the doctest at L3078, with the
same subject.
So the revert offers to remove the unreleased fix versions of SPARK-1234 for
its branches and to reopen the ticket, although this merge never added them;
they come from the change that resolved SPARK-1234. A `Reapply` of the same
commit reaches `resolve_jira_issue` and offers to add fix versions. As the PR
description says, no subject in master's history since the skip tags were added
has this shape yet, but every future merge that adds `MINOR` or `TRIVIAL` at
the prompt to a title with an id produces one. Item 29 suggests a way to remove
this source.
##########
dev/merge_spark_pr.py:
##########
@@ -2467,7 +2657,7 @@ def main():
c = Component.find(tag)
if c is not None and c.primary:
has_primary = True
- components.append(c.tag if c is not None else tag)
+ components.append(Component.normalize(tag))
if not has_primary:
new_tags = prompt_for_components()
Review Comment:
**`[MINOR]` or `[TRIVIAL]` entered at the component prompt always yields a
title that `Title.parse` rejects.** `Title.parse` requires the title to open
with an id, `[MINOR]`, or `[TRIVIAL]` (L2255), so `parsed.leading` is never
empty here, and it rejects a marker next to an id or a second marker
(L2275-L2276). So `[SPARK-1234] Fix` plus `SQL,MINOR` at this prompt becomes
`[SPARK-1234][SQL][MINOR] Fix`, and `[MINOR] Fix` plus `SQL,TRIVIAL` becomes
`[MINOR][SQL][TRIVIAL] Fix`. Typed as the PR title, either one fails at L2651.
This is the only way a new subject gets an id next to a marker, which is what
makes item 28 possible. The marker also lives only in this run's
`title_components`, so a later run cannot recover it from the subject. For
example, a backport run of the same PR, whose GitHub title still has no primary
component, prompts again and adds fix versions if the committer leaves `MINOR`
out this time.
Could the prompt reject `MINOR` and `TRIVIAL`, as it already rejects a list
without a primary component? Then an id next to a marker in a subject would
mean that the subject predates the rule, the comment at L1979-L1981 would hold
as written, the case at L3080 would go away, and `tags |=
set(title_components)` at L1984 would no longer have a marker to bring back.
##########
dev/merge_spark_pr.py:
##########
@@ -2916,24 +3050,89 @@ def main():
>>> update('Revert "Fix"', ["master"], is_revert=True)
>>> reverter.assert_called_once_with(["master"], "")
- Wrapped skip tags preserve the existing policy for MINOR, TRIVIAL, and
FOLLOWUP changes.
+ A bare leading id, as in the old subject SPARK-49004, still reaches the
revert path.
+
+ >>> update('Revert "SPARK-49004"', ["master"], is_revert=True)
+ >>> reverter.assert_called_once_with(["master"], "SPARK-49004")
+
+ Wrapped skip tags follow the same policy as direct merges for MINOR,
TRIVIAL, and FOLLOWUP
Review Comment:
Nit: with the rule at L1978-L1982, this sentence no longer holds for an id
next to `MINOR` or `TRIVIAL`. The doctests at L3078 and L3080 pass the same
subject, `[SPARK-1234][SQL][MINOR] Fix`, and the wrapped one is updated while
the direct one is skipped. Could it say that wrapped tags follow the
direct-merge policy for `FOLLOWUP`, and for `MINOR` and `TRIVIAL` only when
there is no leading id? If item 29 is taken, the case at L3080 can go, and the
paragraph at L3073-L3076 can describe only the older subjects.
##########
dev/merge_spark_pr.py:
##########
@@ -2001,6 +2069,16 @@ def find(cls, token):
return c
return None
+ @classmethod
+ def normalize(cls, token):
+ """Return the registered tag for ``token``, or ``token`` itself if
none matches.
+
+ >>> Component.normalize("FOLLOW-UP"), Component.normalize("BOGUS")
+ ('FOLLOWUP', 'BOGUS')
+ """
+ c = cls.find(token)
+ return c.tag if c is not None else token
Review Comment:
Nit: `find` matches on `token.strip().upper()` (L2066), but on a miss this
returns `token` unchanged, so `Component.normalize(" docs ")` is `'DOC'` while
`Component.normalize(" bogus ")` is `' bogus '`. All three callers pass a
stripped, upper-cased token today, so nothing breaks. Since the docstring
describes a general helper, could a miss return `token.strip().upper()` as
well, or could the docstring say that the token is expected to be normalized
already?
--
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]
