[SPARK-59005][INFRA] Offer to update the JIRA Affects Version from the fix version in the merge script - #58288
Conversation
|
Is this obsoleted by #58300? |
I think no, this is orthogonal to #58300. That PR fixes the Fix Version prompt hanging or being skipped when no version can be inferred. This PR adds a new prompt to update Affects Version/s (derived from the fix version) when the affected-version floor is above the version where the fix lands. |
There was a problem hiding this comment.
Not really an issue specific to this PR, but the heavy level of interactivity makes me think we should consider adopting a library like questionary to make things easier. Of course, that implies adopting a workflow like uv run with inline dependencies, which is a larger conversation in itself.
Just thinking out loud since this has been on my mind for some time and is relevant to several of our dev scripts.
| # Affects Version/s may legitimately name an already-released version, so validate the | ||
| # affects prompt against all unarchived x.y.z versions, not just the unreleased fix ones. | ||
| affects_available = { | ||
| x.name for x in all_versions if not x.raw["archived"] and re.match(r"\d+\.\d+\.\d+", x.name) |
There was a problem hiding this comment.
We repeat this pattern match in several places in this script. Perhaps we should capture it in a utility? The utility could even parse the string into a tuple so that we're always working with the structured object for comparisons and the like.
There was a problem hiding this comment.
yup moved to a util func, thanks!
There was a problem hiding this comment.
Did you mean to use parse_version here?
| def update_affects_versions(self, issue, new_names): | ||
| print( | ||
| "DRY-RUN: would set JIRA %s Affects Version/s to: %s" | ||
| % (issue.key, ", ".join(new_names)) | ||
| ) | ||
|
|
There was a problem hiding this comment.
Nit: For any new code we should generally use f-strings or .format(), not % substitutions. I didn't comment on this in the dry run PR because you were refactoring existing code.
| return [version for version in inferred_versions if version not in existing] | ||
|
|
||
|
|
||
| def parse_affects_versions_input(raw, available_versions): |
There was a problem hiding this comment.
Could we add types to all the new function signatures? Makes them a bit easier to understand at a glance.
There was a problem hiding this comment.
thanks, added type annotations now
| def reconcile_jira_affects_versions(issue, fix_version_names, affects_available): | ||
| """Prompt the committer to update the JIRA Affects Version/s during a merge. | ||
|
|
||
| Meant for the case the caller gates on with ``fix_precedes_affects``: the affected | ||
| floor sits above the earliest fix, so a fixed release is not admitted as affected. | ||
| Mirrors the Fix Version prompt but targets ``issue.fields.versions``: it shows the | ||
| current Affects Version/s and the fix version(s) being set, offers a default | ||
| inferred from the fix version(s) via ``suggest_affects_versions``, then reads a | ||
| comma-separated entry validated against ``affects_available`` (all unarchived | ||
| versions, since an affected version may be released) with a retry loop. A blank | ||
| entry accepts the suggested default; otherwise the parsed versions replace the | ||
| current ones (through ``jira_ops`` so a dry run only logs the intended write). | ||
| """ |
There was a problem hiding this comment.
Maybe it's just me, but this docstring is very difficult to parse. Does something like this still capture the essence?
The "Fix Version" must always be greater than the "Affects Version". If not, prompt the committer to adjust the latter until the versions make sense.
The extra prose describing what the function does step by step does not seem that helpful to me, and couples the docstring more tightly to details that may easily change over time.
(By the way, super nit but: Jira is not an acronym. So not "JIRA". I know, we use "JIRA" all over the place...)
There was a problem hiding this comment.
thanks for the suggestion, rewrote this part and just stating the invariant (affects Version/s should reach down to the earliest fixed release).
For Jira, I left it as JIRA to match the rest of file, we could update them (and all other places together) later.
| while True: | ||
| try: | ||
| raw = bold_input("Enter comma-separated affects version(s) [%s]: " % default_str) | ||
| if raw.strip() == "": | ||
| raw = default_str | ||
| new_names, valid = parse_affects_versions_input(raw, affects_available) | ||
| if valid and new_names: | ||
| break | ||
| print( | ||
| "Specified version(s) [%s] not found in the available versions, try " | ||
| "again (or leave blank to accept the suggestion)." % ", ".join(new_names) | ||
| ) |
There was a problem hiding this comment.
Couldn't we simplify this by giving the user a list of potential versions to choose from by index? Like how we do with the Assignee. Then we avoid needing to loop, parse, or validate their manually inputted versions.
There was a problem hiding this comment.
Good point, but I agree with @szehon-ho that it’s difficult to determine the affected version automatically. A predefined list of potential versions may not be flexible enough, so I’ve changed the prompt to let the committer enter the affected version manually.
I realize this is less convenient, but I think the added flexibility is worth the trade-off.
| kept_affects = [ | ||
| n for n in affects_version_names if semver(n) is not None and semver(n) <= max_fix | ||
| ] | ||
| merged = list(dict.fromkeys(list(fix_version_names) + kept_affects)) |
There was a problem hiding this comment.
Could we avoid adding every Fix Version directly to Affects Version/s? For a patch backport, the fix release is usually not itself affected. For example, a bug affecting 4.2.0 and fixed by a backport in 4.2.1 would make this default suggest 4.2.1 as affected; pressing Enter would then record incorrect Jira metadata. Since the merge target does not tell us when the issue was introduced, perhaps patch backports should require an explicit affected-version choice, or at least should not default the newly added patch Fix Version as affected.
There was a problem hiding this comment.
Good point, agreed. I removed the pre-filled default entirely -- since the merge target cannot tell us when the bug was introduced, we cannot reliably guess the affected version, so there is no default to Enter-through anymore. When the recorded Affects Version/s sit above the fix, the script now just flags the inconsistency and asks the committer to type the correct version(s) explicitly (blank leaves them untouched). That is the deliberate difference from the Fix Version prompt, which can safely pre-fill because the merged branches tell us exactly which releases contain the fix.
|
Also, to understand, its not all affect. Only 'wrong' affect versions right? I also dont know much about @nchammas suggestion for python questionary, i guess that could be a nice (separate) improvement |
Yes, to be clear, I wasn't proposing that for this PR, just reflecting out loud for the future. It would be a big change since we generally avoid third-party libraries. In any case it's definitely a separate discussion. |
4001dc2 to
4490385
Compare
So the intention is that many of Jira tickets are open at affected version 5.0.0 then we fix and backport to 4.x (4.4.0). It will have a weird situation: the issue affecting 5.0.0 but fixed in an earlier version of 4.4.0. I think in this case, the original reported affected version (i.e., 5.0.0) is already incorrect. I usually just manually fix it on Jira to change the affected versions to
Yeah thanks @nchammas for the suggestion. I see how it could become messy now. agreed to push it later in a separate improvement by itself. |
4490385 to
49115d6
Compare
|
actually iiuc, this will change the script to replace affect version, should we it append to it instead? |
49115d6 to
723a384
Compare
723a384 to
ba2f586
Compare
IMO, many Jira tickets have an incorrect affected version, which is often discovered only when running the merge script. We therefore need a way to correct it. In other cases, as you mentioned, we may want to add another affected version. To support both use cases, I think replacing the existing value is the better default. |
|
One concern with replacing by default: Suppose Could we offer append/overwrite/keep, like the component prompt? |
3 participants
What changes were proposed in this pull request?
resolve_jira_issueindev/merge_spark_pr.pynow offers to update the JIRA Affects Version/s, mirroring the existing Fix Version prompt (previously they were display-only). It only prompts when the earliest fix version precedes the earliest affected version, so consistent tickets are untouched. This covers a too-high affected version on a fresh resolve (fixed4.4.0, affects only5.0.0) and a backport that adds an earlier fix line (affects4.4.0, backport adds fix4.3.1, so4.3is affected too). The suggested default is derived from the fix version(s), validated against all unarchived versions, and the write goes throughjira_opsso--dry-runonly logs. Fix Version, assignee, and component logic are unchanged. New pure helpersparse_affects_versions_input,fix_precedes_affects, andsuggest_affects_versionscarry doctests.Why are the changes needed?
The script already reconciles assignee, components, and Fix Version/s, but Affects Version/s were read-only, so a version inconsistent with where the fix lands (too high, or missing a backported line) could not be corrected in-flow.
Does this PR introduce any user-facing change?
No. Committer-facing merge tooling only.
How was this patch tested?
Doctests for the three helpers (
python -m doctest dev/merge_spark_pr.py, all pass) plus a local dry-run of the fresh-resolve and backport flows with a stubbed issue andDryRunJira.Was this patch authored or co-authored using generative AI tooling?
No.