szehon-ho commented on PR #58165:
URL: https://github.com/apache/spark/pull/58165#issuecomment-5375701550
Thanks for the writeup on the three flows -- that clears up my scope
question, and you're right that requiring empty Fix Versions breaks your cases
2 and 3, so drop that half of my earlier suggestion. Two suggestions below that
I think work best together: the first covers the cases where only the committer
can judge, the second removes the cases where there's nothing to judge. With
both in place `allow_resolved` can be deleted from all four functions, since
both merge paths would then get the same treatment.
Assume JIRA has `5.0.0` (master) and `4.3.0` (branch-4.x) unreleased, and
the PR is merged to master only unless noted.
- **Add a new explicit prompt before any Fix Version is added.** Prompt once
at line 1179-1189, where `default_fix_list` is already narrowed to what would
actually be added, so the prompt can show the decision instead of asking blind.
Merging a PR whose ticket is Resolved with `['4.3.0']`:
```
Does SPARK-58880 match this PR? (y/N):
Proceed with merging pull request #58150? (y/N):
Would you like to pick abc1234 into another branch? (y/N):
Would you like to update an associated JIRA? (y/N):
Enter a JIRA id [SPARK-58880]:
JIRA issue SPARK-58880 already has status 'Resolved'
Check if the JIRA information is as expected (y/N): <- newly
reached by this PR
JIRA issue SPARK-58880 has fix version(s) ['4.3.0']; inferred addition(s):
['5.0.0']
Add these fix version(s)? (y/N): <- PROPOSED
Enter comma-separated additional fix version(s) [5.0.0]: <- newly
reached by this PR
```
The added prompt gates the existing one rather than replacing it, so the
committer can still edit the list after answering `y`:
```python
if is_resolved:
default_fix_list = additional_fix_versions(default_fix_list,
existing_fix_version_names)
if not default_fix_list:
print("JIRA issue %s already contains all inferred fix versions;
no update needed." % issue.key)
return
print(
"JIRA issue %s has fix version(s) %s; inferred addition(s): %s"
% (issue.key, existing_fix_version_names, default_fix_list)
)
if get_input("Add these fix version(s)? (y/N): ", ["y", "n", ""]) !=
"y":
return
```
Nothing is added unless the committer says yes, and the ticket's current
Fix Versions are printed right next to the proposed ones, so the answer is
visible at the moment it's asked. This is also what covers the `[FOLLOWUP]`
case @Yicong-Huang raised, and revert PRs: both leave the ticket genuinely
`Fixed`, so no automatic rule can tell them apart from an interrupted merge --
only the committer can.
- **Gate on `resolution == "Fixed"`.** Skip the resolved ticket entirely,
keeping today's behavior of printing the status and moving on, unless its
resolution is `Fixed`. That excludes Duplicate, Won't Fix, Invalid, Incomplete,
and Cannot Reproduce, while letting every ticket from cases 1-3 through, since
your resolve path sets resolution `Fixed` at line 1241. Nothing currently
surfaces the resolution either -- `print_jira_issue_summary` (line 1015) and
`format_jira_verification` (line 920) both print status only -- so a committer
sees "Closed" with no hint that it was closed as a duplicate. The check belongs
as its own early return next to the existing status check at line 1122-1125,
not folded into `is_resolved` at line 1152-1153, since that flag also controls
skipping the assignee and component steps and would send a Duplicate ticket
down the resolve path and try to transition it to Resolved:
```python
status = issue.fields.status.name
if status == "Resolved" or status == "Closed":
resolution = issue.fields.resolution
resolution_name = resolution.name if resolution is not None else None
print("JIRA issue %s already has status '%s' (%s)" % (jira_id, status,
resolution_name))
# Only a ticket an earlier merge resolved as Fixed can legitimately
gain another Fix
# Version. Duplicate / Won't Fix / Invalid tickets must not be touched.
if resolution_name != "Fixed":
return None
```
| Ticket already Resolved/Closed because... | After this PR | With the gate
| Desired |
|---|---|---|---|
| earlier interrupted merge, no Fix Versions yet | `additional fix
version(s) [5.0.0]` | `additional fix version(s) [5.0.0]` | same, the gap being
closed |
| `[FOLLOWUP]`, ticket already has `5.0.0` | `no update needed` | `no update
needed` | same |
| `[FOLLOWUP]` merged to master only, ticket has `4.3.0` | `additional fix
version(s) [5.0.0]` | `additional fix version(s) [5.0.0]` | no prompt |
| the PR reverts the fix, ticket has `4.3.0` | `additional fix version(s)
[5.0.0]` | `additional fix version(s) [5.0.0]` | no prompt |
| closed as Duplicate / Won't Fix | `additional fix version(s) [5.0.0]` | no
prompt | no prompt |
| fix released long ago in `3.5.0` | `additional fix version(s) [5.0.0]` |
`additional fix version(s) [5.0.0]` | no prompt |
| case 3: separate `branch-4.x` PR, ticket has `5.0.0` | `additional fix
version(s) [4.3.0]` | `additional fix version(s) [4.3.0]` | same, target case |
| case 2: later backport-mode run, ticket has `5.0.0` | `[4.3.0]`, unchanged
| unchanged | same |
The gate on its own settles the Duplicate / Won't Fix row and costs nothing
on cases 1-3, but the three `Fixed`-but-shouldn't rows still reach the prompt
-- those are the ones the first suggestion catches. Conversely the gate keeps
the first suggestion's prompt off merges where the answer is never yes.
What do you think about these two? Happy to be talked out of either if I'm
misreading how often the resolved-ticket cases come up in practice.
--
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]
