nchammas commented on code in PR #58288:
URL: https://github.com/apache/spark/pull/58288#discussion_r3866816782
##########
dev/merge_spark_pr.py:
##########
@@ -350,6 +350,112 @@ def additional_fix_versions(inferred_versions,
existing_versions):
return [version for version in inferred_versions if version not in
existing]
+def parse_affects_versions_input(raw, available_versions):
Review Comment:
Could we add types to all the new function signatures? Makes them a bit
easier to understand at a glance.
##########
dev/merge_spark_pr.py:
##########
@@ -1179,6 +1285,56 @@ def reconcile_jira_components(issue, title_components):
jira_ops.update_components(issue, new_names)
+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).
+ """
+ current_names = [v.name for v in issue.fields.versions]
+ suggested = [
+ n
+ for n in suggest_affects_versions(fix_version_names, current_names)
+ if n in affects_available
+ ]
+ if not suggested:
+ return
+ default_str = ",".join(suggested)
+ print()
+ print("=" * 80)
+ print(
+ "JIRA %s Affects Version/s %s do not cover the fix version(s) %s."
+ % (issue.key, current_names if current_names else "(none)",
fix_version_names)
+ )
+ print("=" * 80)
+ 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)
+ )
Review Comment:
Couldn't we simplify this by giving the user a list of potential versions to
choose from? Like how we do with the Assignee. Then we avoid needing to loop,
parse, or validate their manually inputted versions.
##########
dev/merge_spark_pr.py:
##########
@@ -1231,14 +1387,19 @@ def resolve_jira_issue(
reconcile_jira_components(issue, title_components)
- versions = asf_jira.project_versions("SPARK")
+ all_versions = asf_jira.project_versions("SPARK")
# Consider only x.y.z, unreleased, unarchived versions
versions = [
x
- for x in versions
+ for x in all_versions
if not x.raw["released"] and not x.raw["archived"] and
re.match(r"\d+\.\d+\.\d+", x.name)
]
versions = sorted(versions, key=lambda x: x.name, reverse=True)
+ # 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)
Review 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.
##########
dev/merge_spark_pr.py:
##########
@@ -1439,6 +1620,12 @@ class DryRunJira(Jira):
def update_components(self, issue, new_names):
print("DRY-RUN: would set JIRA %s components to: %s" % (issue.key, ",
".join(new_names)))
+ 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))
+ )
+
Review 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.
##########
dev/merge_spark_pr.py:
##########
@@ -1179,6 +1285,56 @@ def reconcile_jira_components(issue, title_components):
jira_ops.update_components(issue, new_names)
+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).
+ """
Review 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...)
--
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]
