From 2eca94774a64260759702abb00d296d041551285 Mon Sep 17 00:00:00 2001 From: Jens Geyer Date: Wed, 16 Sep 2026 23:04:42 +0200 Subject: [PATCH 1/2] THRIFT-6277: List only tickets fixed in the release in the CHANGES draft Client: build The CHANGES draft listed every THRIFT ticket a commit message mentions, without looking at the ticket's Fix Version/s, resolution or status. The 0.25.0 draft therefore carried THRIFT-1337, fixed in 2011, because the THRIFT-6183 commit cites it as history. It also carried other old tickets cited the same way, a Won't Do ticket and tickets that are still open. With --jira-version, the extra lookup of tickets found in git had the same gap. Fetch Fix Version/s, status and resolution with each ticket. Keep a ticket found in git only if JIRA files it under the release version, using the same test as the fixVersion query: the version is among its Fix Version/s, it has a resolution, and it is not Open. The version is --jira-version if given, otherwise --version or configure.ac. A commit whose tickets all fail the test is listed as a commit link under its Client: section instead, and the skipped tickets are named on stderr. The fixVersion query and that test now also leave out tickets resolved without a change, such as Won't Do, Won't Fix, Not A Problem or Cannot Reproduce, even when they carry the fix version. Duplicate still counts. Commit links now also drop a "Patch: Name" or "Autor: Name" trailer from the subject. Such commits used to be hidden behind their ticket. Both JIRA queries now build their entries with one shared helper. Co-Authored-By: Claude Opus 5 (1M context) --- build/generate-changes.py | 187 +++++++++++++----- build/test_generate_changes.py | 350 ++++++++++++++++++++++++++++++++- 2 files changed, 482 insertions(+), 55 deletions(-) diff --git a/build/generate-changes.py b/build/generate-changes.py index 9fefd54838a..8c873edc75d 100755 --- a/build/generate-changes.py +++ b/build/generate-changes.py @@ -24,14 +24,18 @@ Three complementary data sources are combined: 1. JIRA (primary, when --jira-version is given) - Queries all tickets where fixVersion = VERSION and status is resolved. + Queries all tickets where fixVersion = VERSION and status is resolved, + except those resolved without a change (Won't Fix, Not A Problem, ...). This is the authoritative list used in actual releases. 2. Git commits (always) Walks commits between the last v* tag (or --from) and the branch tip. Extracts THRIFT-NNNN references from commit messages and fetches their - JIRA summaries. Commits with no ticket reference are included as - GitHub commit links, grouped by their "Client:" trailer. + JIRA summaries. A referenced ticket is listed only if JIRA files it + under the release version the same way the fixVersion query does, so a + commit that merely mentions an older ticket does not list it. Commits + with no such ticket are included as GitHub commit links, grouped by + their "Client:" trailer. 3. GitHub PR labels (fallback for commits without a "Client:" trailer) When a commit was merged via a PR (subject ends with "(#NNN)") and has @@ -41,8 +45,10 @@ With --github-token the script also resolves PR numbers for commits whose subject lacks the "(#NNN)" suffix, so all commit links point to their PR. -When --jira-version is NOT given the script is git-only (useful while a -release is still in progress and fixVersions haven't been assigned in JIRA). +When --jira-version is NOT given the script is git-only: tickets come from +commit messages alone, filtered by the version from --version or +configure.ac. A commit whose ticket has no fixVersion assigned yet is still +listed, as a commit link. Usage: generate-changes.py [options] @@ -50,7 +56,9 @@ Options: --branch BRANCH Branch to analyze (default: current branch or master) --from TAG Starting tag or commit ref (default: auto-detect latest v* tag) - --version VERSION Release version for the ## header (default: from configure.ac) + --version VERSION Release version for the ## header and, without + --jira-version, the fixVersion a referenced ticket + needs to be listed (default: from configure.ac) --jira-version VERSION Also query JIRA for all tickets with this fixVersion; overrides git-extracted tickets as the primary source --no-commits Exclude ticket-less commits from output (default: include them) @@ -316,6 +324,9 @@ def clean_subject(subject): subject = re.sub(r'^No\s+ticket:\s*', '', subject, flags=re.IGNORECASE) # Remove trailing "Client: ..." trailer that appears on the subject line subject = re.sub(r'\s+Client:\s*\S.*$', '', subject, flags=re.IGNORECASE) + # ... and the contributor trailers extract_client_sections() strips, also + # when they precede the Client: trailer + subject = re.sub(r'\s+(?:Patch|Autor):.*$', '', subject) # codespell:ignore # Strip trailing PR reference " (#NNN)" subject = re.sub(r'\s+\(#\d+\)\s*$', '', subject) return subject.strip() @@ -336,11 +347,57 @@ def jira_component_to_section(comp_name): return JIRA_COMPONENT_MAP.get(base, base) +# Issue fields both JIRA queries request; jira_issue_entry() reads them. +JIRA_FIELDS = "summary,components,fixVersions,status,resolution" + +# Resolutions that close a ticket without a change to report. Such tickets +# stay out of the release notes even when they carry the fix version. +NON_FIX_RESOLUTIONS = ( + "Won't Do", + "Won't Fix", + "Not A Problem", + "Not A Bug", + "Cannot Reproduce", + "Works for Me", + "Invalid", + "Incomplete", + "Information Provided", + "Later", + "Abandoned", + "Auto Closed", +) + + +def jira_issue_entry(fields): + """Turn a JIRA issue's "fields" object into the entry the fetch helpers + return: + {"summary": str, "sections": [str], "fix_versions": [str], + "status": str, "resolution": str or None} + """ + raw_sections = [ + jira_component_to_section(c["name"]) + for c in fields.get("components") or [] + ] + # Deduplicate, preserving order + seen: set = set() + sections = [] + for s in raw_sections: + if s not in seen: + seen.add(s) + sections.append(s) + return { + "summary": fields["summary"], + "sections": sections if sections else ["(No Section)"], + "fix_versions": [v["name"] for v in fields.get("fixVersions") or []], + "status": (fields.get("status") or {}).get("name"), + "resolution": (fields.get("resolution") or {}).get("name"), + } + + def fetch_jira_issues(ticket_ids): - """Query JIRA for summary + components. + """Query JIRA for the given tickets. - Returns dict mapping ticket_id (uppercase) to - {"summary": str, "sections": [str]} + Returns dict mapping ticket_id (uppercase) to a jira_issue_entry(). Unknown / unreachable tickets are absent from the result. """ if not ticket_ids: @@ -354,7 +411,7 @@ def fetch_jira_issues(ticket_ids): keys = ",".join(batch) params = urlencode({ "jql": f"key in ({keys})", - "fields": "summary,components", + "fields": JIRA_FIELDS, "maxResults": 50, }) url = f"{JIRA_BASE}/rest/api/2/search?{params}" @@ -363,23 +420,7 @@ def fetch_jira_issues(ticket_ids): with urllib.request.urlopen(req, timeout=30) as resp: data = json.loads(resp.read()) for issue in data.get("issues", []): - key = issue["key"].upper() - summary = issue["fields"]["summary"] - raw_sections = [ - jira_component_to_section(c["name"]) - for c in issue["fields"].get("components", []) - ] - # Deduplicate, preserving order - seen: set = set() - sections = [] - for s in raw_sections: - if s not in seen: - seen.add(s) - sections.append(s) - result[key] = { - "summary": summary, - "sections": sections if sections else ["(No Section)"], - } + result[issue["key"].upper()] = jira_issue_entry(issue["fields"]) except (urllib.error.URLError, urllib.error.HTTPError, json.JSONDecodeError) as exc: print(f"Warning: JIRA query failed: {exc}", file=sys.stderr) @@ -391,7 +432,8 @@ def fetch_jira_issues(ticket_ids): def fetch_jira_by_fixversion(fix_version): """Return the same dict format as fetch_jira_issues, for all tickets that - have fixVersion = fix_version and are resolved/closed. + have fixVersion = fix_version and are resolved/closed, except those with + a resolution in NON_FIX_RESOLUTIONS. This is the authoritative JIRA query described in ReleaseManagement.md: project = THRIFT AND resolution = Fixed @@ -401,18 +443,22 @@ def fetch_jira_by_fixversion(fix_version): start_at = 0 page_size = 100 - # Include any resolved/closed ticket regardless of resolution sub-type. - # The release manager occasionally assigns fixVersion to Duplicate or - # similar tickets when they were addressed as part of the release. + # Include resolved/closed tickets beyond resolution = Fixed: the release + # manager occasionally assigns fixVersion to Duplicate or similar tickets + # when they were addressed as part of the release. Resolutions that + # record no change (NON_FIX_RESOLUTIONS) stay out. + # jira_ticket_in_release() applies the same test to tickets found in git. + non_fix = ", ".join(f'"{name}"' for name in NON_FIX_RESOLUTIONS) jql = ( f'project = THRIFT AND resolution != Unresolved ' + f'AND resolution not in ({non_fix}) ' f'AND fixVersion = "{fix_version}" AND status != Open' ) while True: params = urlencode({ "jql": jql, - "fields": "summary,components", + "fields": JIRA_FIELDS, "maxResults": page_size, "startAt": start_at, }) @@ -426,22 +472,7 @@ def fetch_jira_by_fixversion(fix_version): break for issue in data.get("issues", []): - key = issue["key"].upper() - summary = issue["fields"]["summary"] - raw_sections = [ - jira_component_to_section(c["name"]) - for c in issue["fields"].get("components", []) - ] - seen: set = set() - sections = [] - for s in raw_sections: - if s not in seen: - seen.add(s) - sections.append(s) - result[key] = { - "summary": summary, - "sections": sections if sections else ["(No Section)"], - } + result[issue["key"].upper()] = jira_issue_entry(issue["fields"]) total = data.get("total", 0) start_at += page_size @@ -452,6 +483,51 @@ def fetch_jira_by_fixversion(fix_version): return result +def jira_ticket_in_release(entry, version): + """True if JIRA files the ticket under version the way the fixVersion + query in fetch_jira_by_fixversion() does: version is among its fix + versions, it has a resolution that is not in NON_FIX_RESOLUTIONS, and + it is not Open.""" + return ( + version in entry["fix_versions"] and + entry["resolution"] is not None and + entry["resolution"] not in NON_FIX_RESOLUTIONS and + entry["status"] != "Open" + ) + + +def filter_release_tickets(jira_data, version): + """Split fetched JIRA entries by jira_ticket_in_release(). + + Returns (kept, skipped): the entries in the release, and the keys of the + others in ticket-number order.""" + kept = {} + skipped = [] + for key, entry in jira_data.items(): + if jira_ticket_in_release(entry, version): + kept[key] = entry + else: + skipped.append(key) + skipped.sort(key=lambda key: int(key.rsplit("-", 1)[1])) + return kept, skipped + + +def fetch_release_tickets(ticket_ids, version): + """fetch_jira_issues() restricted to the tickets in the release. + + A commit message may name a ticket that belongs to another release, for + instance as history. Such tickets are reported and left out, so commits + that reference nothing else are listed as commit links instead.""" + kept, skipped = filter_release_tickets(fetch_jira_issues(ticket_ids), version) + if skipped: + print( + f"Skipping {len(skipped)} referenced tickets that JIRA does not " + f"list as fixed in {version}: {', '.join(skipped)}", + file=sys.stderr, + ) + return kept + + # --------------------------------------------------------------------------- # GitHub helpers # --------------------------------------------------------------------------- @@ -662,6 +738,8 @@ def generate_changes(args): # --- Version --- version = args.version or version_from_configure(repo_root) or "X.Y.Z" print(f"Version : {version}", file=sys.stderr) + # The fixVersion a ticket referenced in git must carry to be listed. + release_version = args.jira_version or version # --- Commits --- commits = get_commits(since, branch, repo_root) @@ -700,11 +778,11 @@ def generate_changes(args): f"Fetching {len(extra)} additional git-referenced tickets from JIRA ...", file=sys.stderr, ) - jira_data.update(fetch_jira_issues(extra)) + jira_data.update(fetch_release_tickets(extra, release_version)) else: if all_tickets: print(f"Querying JIRA for {len(all_tickets)} git-referenced tickets ...", file=sys.stderr) - jira_data = fetch_jira_issues(all_tickets) + jira_data = fetch_release_tickets(all_tickets, release_version) print(f"Total JIRA entries: {len(jira_data)}", file=sys.stderr) @@ -859,14 +937,19 @@ def main(): ) parser.add_argument( "--version", metavar="VERSION", - help="release version for the ## header (default: read from configure.ac)", + help=( + "release version for the ## header and, without --jira-version, " + "the fixVersion a git-referenced ticket needs to be listed " + "(default: read from configure.ac)" + ), ) parser.add_argument( "--jira-version", dest="jira_version", metavar="VERSION", help=( "query JIRA for all tickets with this fixVersion as the primary " "source (recommended for release prep once fixVersions are assigned); " - "git-extracted tickets are merged in as a supplement" + "git-extracted tickets with the same fixVersion are merged in as a " + "supplement" ), ) parser.add_argument( diff --git a/build/test_generate_changes.py b/build/test_generate_changes.py index f335f9981b7..cbdbde5a028 100644 --- a/build/test_generate_changes.py +++ b/build/test_generate_changes.py @@ -18,7 +18,7 @@ # under the License. # -"""Unit tests for build/generate-changes.py section-assignment logic. +"""Unit tests for build/generate-changes.py. These cover the three cases that previously produced "(No Section)" entries: @@ -29,13 +29,27 @@ 3. JIRA tickets with no usable component, filed under the section named by their commit's Client: trailer instead (JIRA -> trailer fallback). -No network access is required: only the pure mapping/assignment helpers are -exercised. +They also cover which referenced tickets get a JIRA line: only those JIRA +files under the release version. A commit that merely mentions an old +ticket (THRIFT-6183 citing THRIFT-1337) must not list it. Those tests run +generate_changes() against a temporary git repository and a fake JIRA. + +No network access is required. """ +import argparse +import contextlib import importlib.util +import io +import json import os +import re +import shutil +import subprocess +import tempfile import unittest +import urllib.parse +from unittest import mock # generate-changes.py has a hyphen in its name, so it cannot be imported with a # plain ``import``; load it as a module from its path instead. @@ -59,6 +73,90 @@ def make_commit(sha="0" * 40, pr_num=None, sections=None, tickets=()): } +def jira_fields(summary, components=(), fix_versions=(), status="Resolved", + resolution="Fixed"): + """Build an issue's "fields" object as the JIRA REST API returns it.""" + return { + "summary": summary, + "components": [{"name": c} for c in components], + "fixVersions": [{"name": v} for v in fix_versions], + "status": {"name": status}, + "resolution": {"name": resolution} if resolution else None, + } + + +class FakeJira: + """Stands in for urllib.request.urlopen and answers the two JIRA searches + generate-changes.py sends ("key in (...)" and the fixVersion query) from + a {key: fields} table. Like the real REST API it returns only the + fields the request asked for. Any other request fails the test.""" + + def __init__(self, issues): + self.issues = issues + + @staticmethod + def matches_fix_version_query(jql, fields, version): + # resolution != Unresolved [AND resolution not in (...)] + # AND fixVersion = version AND status != Open + excluded = re.search(r"resolution not in \(([^)]*)\)", jql) + names = ( + [n.strip().strip('"') for n in excluded.group(1).split(",")] + if excluded else [] + ) + resolution = fields["resolution"] + return ( + resolution is not None and + resolution["name"] not in names and + version in [v["name"] for v in fields["fixVersions"]] and + fields["status"]["name"] != "Open" + ) + + def urlopen(self, req, timeout=None): + prefix = f"{gc.JIRA_BASE}/rest/api/2/search?" + url = req.full_url + if not url.startswith(prefix): + raise AssertionError(f"unexpected request: {url}") + query = urllib.parse.parse_qs(url[len(prefix):]) + jql = query["jql"][0] + wanted = query["fields"][0].split(",") + start = int(query.get("startAt", ["0"])[0]) + keys = re.fullmatch(r"key in \((.*)\)", jql) + if keys: + hits = [k for k in keys.group(1).split(",") if k in self.issues] + else: + version = re.search(r'fixVersion = "([^"]+)"', jql).group(1) + hits = [ + k for k, f in self.issues.items() + if self.matches_fix_version_query(jql, f, version) + ] + body = { + "total": len(hits), + "issues": [ + { + "key": k, + "fields": { + name: self.issues[k][name] + for name in wanted if name in self.issues[k] + }, + } + for k in hits[start:] + ], + } + return io.BytesIO(json.dumps(body).encode("utf-8")) + + +def sections_of(draft): + """Map each ### heading of a rendered draft to its bullet lines.""" + result = {} + bullets = None + for line in draft.splitlines(): + if line.startswith("### "): + bullets = result.setdefault(line[len("### "):], []) + elif line.startswith("- ") and bullets is not None: + bullets.append(line) + return result + + class LabelMappingTests(unittest.TestCase): """Fix 1: dependabot / CI labels route to the Build Process section.""" @@ -212,5 +310,251 @@ def test_jira_components_map_to_zig(self): self.assertEqual(gc.jira_component_to_section("Zig - Compiler"), "Zig") +class CleanSubjectTests(unittest.TestCase): + """A commit line shows the subject without its ticket and trailers.""" + + def test_patch_trailer_before_client_trailer_is_stripped(self): + # Mirrors 2ae9c11db, listed by commit while THRIFT-6108 is open. + self.assertEqual( + gc.clean_subject( + "THRIFT-6108: Consolidate replace_all() into t_oop_generator" + " Patch: A. Contributor Client: dart,delphi" + ), + "Consolidate replace_all() into t_oop_generator", + ) + + def test_autor_trailer_is_stripped(self): + self.assertEqual( + gc.clean_subject("THRIFT-1: Fix the build Autor: A. Contributor"), # codespell:ignore + "Fix the build", + ) + + +TICKET_6183 = jira_fields( + "Use the library-wide default frame size in TNonblockingServer", + components=["C++ - Library"], fix_versions=["0.25.0"], +) + +# Fixed in 2011 and never given a Fix Version/s. +TICKET_1337 = jira_fields( + "thrift: support maximum frame size in TNonblockingServer", + components=["C++ - Library"], status="Closed", +) + + +class ReleaseTicketFilterTests(unittest.TestCase): + """A ticket is in the release only if JIRA files it under that version.""" + + def in_release(self, version="0.25.0", **fields): + entry = gc.jira_issue_entry(jira_fields("summary", **fields)) + return gc.jira_ticket_in_release(entry, version) + + def test_resolved_ticket_with_the_release_fix_version_is_in(self): + self.assertTrue(self.in_release(fix_versions=["0.25.0"])) + + def test_ticket_without_a_fix_version_is_out(self): + self.assertFalse(self.in_release(status="Closed")) + + def test_ticket_fixed_in_another_version_is_out(self): + self.assertFalse(self.in_release(fix_versions=["0.24.0"])) + + def test_release_among_several_fix_versions_is_in(self): + self.assertTrue(self.in_release(fix_versions=["0.24.1", "0.25.0"])) + + def test_open_ticket_is_out_even_with_the_release_fix_version(self): + self.assertFalse(self.in_release( + fix_versions=["0.25.0"], status="Open", resolution=None + )) + + def test_unresolved_ticket_is_out_even_when_not_open(self): + self.assertFalse(self.in_release( + fix_versions=["0.25.0"], status="In Progress", resolution=None + )) + + def test_resolutions_that_record_a_change_are_in(self): + # Duplicate included: the release manager sometimes gives a duplicate + # the fix version when the release addressed it. + for resolution in ["Fixed", "Done", "Implemented", "Duplicate"]: + with self.subTest(resolution=resolution): + self.assertTrue(self.in_release( + fix_versions=["0.25.0"], status="Closed", + resolution=resolution, + )) + + def test_resolutions_without_a_fix_are_out(self): + # THRIFT-5917 (Won't Do): not a change to report, whatever its + # Fix Version/s says. + for resolution in [ + "Won't Do", "Won't Fix", "Not A Problem", "Not A Bug", + "Cannot Reproduce", "Works for Me", "Invalid", "Incomplete", + "Information Provided", "Later", "Abandoned", "Auto Closed", + ]: + with self.subTest(resolution=resolution): + self.assertFalse(self.in_release( + fix_versions=["0.25.0"], status="Closed", + resolution=resolution, + )) + + def test_entry_keeps_the_fields_the_filter_needs(self): + entry = gc.jira_issue_entry(jira_fields( + "summary", + components=["C++ - Library", "C++ - Compiler"], + fix_versions=["0.25.0"], + )) + self.assertEqual(entry, { + "summary": "summary", + "sections": ["C++"], + "fix_versions": ["0.25.0"], + "status": "Resolved", + "resolution": "Fixed", + }) + + def test_filter_reports_skipped_tickets_in_ticket_order(self): + data = { + "THRIFT-6183": gc.jira_issue_entry(TICKET_6183), + "THRIFT-1337": gc.jira_issue_entry(TICKET_1337), + "THRIFT-892": gc.jira_issue_entry( + jira_fields("summary", fix_versions=["0.7"]) + ), + } + kept, skipped = gc.filter_release_tickets(data, "0.25.0") + self.assertEqual(list(kept), ["THRIFT-6183"]) + self.assertEqual(skipped, ["THRIFT-892", "THRIFT-1337"]) + + +@unittest.skipUnless(shutil.which("git"), "git is not installed") +class ReleaseTicketDraftTests(unittest.TestCase): + """End to end: which referenced tickets the rendered draft lists.""" + + CPP_6183 = ( + "- [THRIFT-6183](https://issues.apache.org/jira/browse/THRIFT-6183)" + " - Use the library-wide default frame size in TNonblockingServer" + ) + + def setUp(self): + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + self.repo = os.path.join(tmp.name, "repo") + self.output = os.path.join(tmp.name, "CHANGES-draft.md") + os.mkdir(self.repo) + # Keep the user's git configuration away from both the setup below and + # the script's own git calls. + env = mock.patch.dict(os.environ, { + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_AUTHOR_NAME": "Test", + "GIT_AUTHOR_EMAIL": "test@example.org", + "GIT_COMMITTER_NAME": "Test", + "GIT_COMMITTER_EMAIL": "test@example.org", + }) + env.start() + self.addCleanup(env.stop) + self.git("init", "-q") + configure_ac = os.path.join(self.repo, "configure.ac") + with open(configure_ac, "w", encoding="utf-8") as f: + f.write("AC_INIT([thrift], [0.25.0], [dev@thrift.apache.org])\n") + self.git("add", "configure.ac") + self.git("commit", "-q", "-m", "Set the version to 0.25.0") + self.git("tag", "v0.24.0") + + def git(self, *args): + subprocess.run( + ["git"] + list(args), cwd=self.repo, check=True, capture_output=True + ) + + def commit(self, message): + self.git("commit", "-q", "--allow-empty", "-m", message) + + def generate(self, issues, jira_version=None): + """Run generate_changes() on the repository; return (draft, stderr).""" + args = argparse.Namespace( + branch=None, from_tag=None, version=None, jira_version=jira_version, + no_commits=False, github_token=None, repo="apache/thrift", + output=self.output, + ) + log = io.StringIO() + fake = FakeJira(issues) + with mock.patch.object(gc, "find_repo_root", return_value=self.repo), \ + mock.patch.object(gc.urllib.request, "urlopen", fake.urlopen), \ + contextlib.redirect_stderr(log): + gc.generate_changes(args) + with open(self.output, encoding="utf-8") as f: + return f.read(), log.getvalue() + + def commit_6183(self): + # Mirrors 8b79397f7, whose body names THRIFT-1337 only as history. + self.commit( + "THRIFT-6183: Use the library-wide default frame size in" + " TNonblockingServer\n" + "\n" + "TNonblockingServer caps the frame it will accept at its own\n" + "MAX_FRAME_SIZE, 256 * 1024 * 1024 since THRIFT-1337 landed it in\n" + "2011.\n" + "\n" + "Client: cpp\n" + ) + + def test_ticket_mentioned_in_a_commit_body_is_not_listed(self): + self.commit_6183() + draft, log = self.generate( + {"THRIFT-6183": TICKET_6183, "THRIFT-1337": TICKET_1337} + ) + self.assertEqual(sections_of(draft), {"C++": [self.CPP_6183]}) + self.assertIn("THRIFT-1337", log) + + def test_jira_version_mode_does_not_add_it_back(self): + # The fixVersion query finds THRIFT-6183; THRIFT-1337 only comes in + # through the extra lookup of tickets the commits reference. + self.commit_6183() + draft, log = self.generate( + {"THRIFT-6183": TICKET_6183, "THRIFT-1337": TICKET_1337}, + jira_version="0.25.0", + ) + self.assertEqual(sections_of(draft), {"C++": [self.CPP_6183]}) + self.assertIn("THRIFT-1337", log) + + def test_subject_ticket_outside_the_release_is_listed_by_commit(self): + # Mirrors f62e1b4bf: THRIFT-1941 was closed without a Fix Version/s, + # so the commit is listed by its PR and subject instead. + self.commit( + "THRIFT-1941: Add PHP serializer regression coverage (#3794)\n" + "\n" + "Client: php\n" + ) + draft, _ = self.generate({ + "THRIFT-1941": jira_fields( + "PHP Serializer deserialize doesn't work", + components=["PHP - Library"], status="Closed", + ), + }) + self.assertEqual(sections_of(draft), {"PHP": [ + "- [#3794](https://github.com/apache/thrift/pull/3794)" + " - Add PHP serializer regression coverage" + ]}) + + def test_ticket_resolved_without_a_fix_is_listed_by_commit(self): + # Like THRIFT-5917, but carrying the fix version, so that the + # fixVersion query itself has to leave it out. + self.commit( + "THRIFT-5917: Remove Rust deprecation warning (#3637)\n" + "\n" + "Client: rs\n" + ) + issues = { + "THRIFT-5917": jira_fields( + "Drop Rust support?", components=["Rust - Library"], + fix_versions=["0.25.0"], status="Closed", resolution="Won't Do", + ), + } + expected = {"Rust": [ + "- [#3637](https://github.com/apache/thrift/pull/3637)" + " - Remove Rust deprecation warning" + ]} + for jira_version in [None, "0.25.0"]: + with self.subTest(jira_version=jira_version): + draft, _ = self.generate(issues, jira_version=jira_version) + self.assertEqual(sections_of(draft), expected) + + if __name__ == "__main__": unittest.main() From 3bfc4482f0be73ae2322e2bdb1a8042c9db9dcf4 Mon Sep 17 00:00:00 2001 From: Jens Geyer Date: Wed, 16 Sep 2026 23:34:47 +0200 Subject: [PATCH 2/2] THRIFT-6278: Don't lose a CHANGES draft JIRA lookup to one unknown key Client: build generate-changes.py looks up the tickets it finds in commit messages with JIRA "key in (...)" searches of up to 50 keys. For a list of 25 keys or fewer, JIRA checks that every key exists and rejects the whole search if one does not, so every ticket in that search was lost. Longer lists are not checked. Such keys do occur. The ticket pattern ignores case, so the path "thrift-0.24.0/lib/cpp/..." in the body of da6ed655d was read as THRIFT-0. On current master this made the extra lookup of the --jira-version run fail every time. Turn the check off for the key lookup and report the keys JIRA does not return instead. The fixVersion query keeps the check: without it, a mistyped version or resolution name would quietly change the result instead of failing. The ticket pattern now skips a number followed by ".digit", as in a version string. Across the whole history this drops only THRIFT-0. Co-Authored-By: Claude Opus 5 (1M context) --- build/generate-changes.py | 27 +++++++++-- build/test_generate_changes.py | 85 +++++++++++++++++++++++++++++++++- 2 files changed, 107 insertions(+), 5 deletions(-) diff --git a/build/generate-changes.py b/build/generate-changes.py index 8c873edc75d..ac76948c0fb 100755 --- a/build/generate-changes.py +++ b/build/generate-changes.py @@ -223,7 +223,9 @@ # Add any section names you want pinned to the bottom here. LATE_SECTIONS = {"(All Languages)", "(No Section)"} -TICKET_RE = re.compile(r'\bTHRIFT-(\d+)\b', re.IGNORECASE) +# A number followed by ".digit" belongs to a version, as in the path +# "thrift-0.24.0/lib/...", and is not a ticket. +TICKET_RE = re.compile(r'\bTHRIFT-(\d+)\b(?!\.\d)', re.IGNORECASE) CLIENT_TRAILER_RE = re.compile(r'\bClient:\s*(.+)', re.IGNORECASE) PR_RE = re.compile(r'\(#(\d+)\)\s*$') @@ -299,6 +301,11 @@ def extract_tickets(subject, body): return {f"THRIFT-{m.group(1)}" for m in TICKET_RE.finditer(text)} +def ticket_number(ticket_id): + """Sort key for 'THRIFT-NNNN' strings: the number NNNN.""" + return int(ticket_id.rsplit("-", 1)[1]) + + def extract_client_sections(subject, body): """Return list of canonical section names from the Client: trailer.""" text = f"{subject}\n{body}" @@ -398,12 +405,14 @@ def fetch_jira_issues(ticket_ids): """Query JIRA for the given tickets. Returns dict mapping ticket_id (uppercase) to a jira_issue_entry(). - Unknown / unreachable tickets are absent from the result. + Unknown / unreachable tickets are absent from the result; the unknown + ones are reported on stderr. """ if not ticket_ids: return {} result = {} + unknown = [] ticket_list = sorted(ticket_ids) for i in range(0, len(ticket_list), 50): @@ -413,6 +422,11 @@ def fetch_jira_issues(ticket_ids): "jql": f"key in ({keys})", "fields": JIRA_FIELDS, "maxResults": 50, + # A validating JIRA rejects a list of up to 25 keys as a whole if + # one of them does not exist, losing all the others. Only keys + # are looked up here, so skip the check and report the keys that + # come back empty instead. + "validateQuery": "false", }) url = f"{JIRA_BASE}/rest/api/2/search?{params}" try: @@ -421,12 +435,19 @@ def fetch_jira_issues(ticket_ids): data = json.loads(resp.read()) for issue in data.get("issues", []): result[issue["key"].upper()] = jira_issue_entry(issue["fields"]) + unknown.extend(k for k in batch if k.upper() not in result) except (urllib.error.URLError, urllib.error.HTTPError, json.JSONDecodeError) as exc: print(f"Warning: JIRA query failed: {exc}", file=sys.stderr) if i + 50 < len(ticket_list): time.sleep(0.3) + if unknown: + unknown.sort(key=ticket_number) + print( + f"Warning: referenced tickets not found in JIRA: {', '.join(unknown)}", + file=sys.stderr, + ) return result @@ -508,7 +529,7 @@ def filter_release_tickets(jira_data, version): kept[key] = entry else: skipped.append(key) - skipped.sort(key=lambda key: int(key.rsplit("-", 1)[1])) + skipped.sort(key=ticket_number) return kept, skipped diff --git a/build/test_generate_changes.py b/build/test_generate_changes.py index cbdbde5a028..d36e1c8617b 100644 --- a/build/test_generate_changes.py +++ b/build/test_generate_changes.py @@ -48,6 +48,7 @@ import subprocess import tempfile import unittest +import urllib.error import urllib.parse from unittest import mock @@ -89,7 +90,13 @@ class FakeJira: """Stands in for urllib.request.urlopen and answers the two JIRA searches generate-changes.py sends ("key in (...)" and the fixVersion query) from a {key: fields} table. Like the real REST API it returns only the - fields the request asked for. Any other request fails the test.""" + fields the request asked for, and it rejects a key list that names an + unknown key when it validates the query. Any other request fails the + test.""" + + # issues.apache.org checks that each key exists only in key lists up to + # this length, and only while validateQuery is on (the default). + VALIDATED_KEY_LIST = 25 def __init__(self, issues): self.issues = issues @@ -122,7 +129,19 @@ def urlopen(self, req, timeout=None): start = int(query.get("startAt", ["0"])[0]) keys = re.fullmatch(r"key in \((.*)\)", jql) if keys: - hits = [k for k in keys.group(1).split(",") if k in self.issues] + requested = keys.group(1).split(",") + unknown = [k for k in requested if k not in self.issues] + validate = query.get("validateQuery", ["true"])[0].lower() == "true" + if validate and unknown and len(requested) <= self.VALIDATED_KEY_LIST: + error = {"errorMessages": [ + f"An issue with key '{k}' does not exist for field 'key'." + for k in unknown + ], "errors": {}} + raise urllib.error.HTTPError( + url, 400, "Bad Request", None, + io.BytesIO(json.dumps(error).encode("utf-8")), + ) + hits = [k for k in requested if k in self.issues] else: version = re.search(r'fixVersion = "([^"]+)"', jql).group(1) hits = [ @@ -310,6 +329,29 @@ def test_jira_components_map_to_zig(self): self.assertEqual(gc.jira_component_to_section("Zig - Compiler"), "Zig") +class TicketExtractionTests(unittest.TestCase): + """Which THRIFT-NNNN mentions in a commit message count as tickets.""" + + def test_version_string_is_not_a_ticket(self): + # Mirrors da6ed655d, whose body quotes a path in the 0.24.0 tarball. + self.assertEqual(gc.extract_tickets( + "Add cstddef include to fix build error with 6.3.0", + "thrift-0.24.0/lib/cpp/src/thrift/transport/TBufferTransports.h:110:32:", + ), set()) + + def test_ticket_at_the_end_of_a_sentence_is_kept(self): + self.assertEqual( + gc.extract_tickets("Fix the frame size", "Follows up on THRIFT-1337."), + {"THRIFT-1337"}, + ) + + def test_ticket_prefix_in_any_case_is_kept(self): + self.assertEqual( + gc.extract_tickets("Thrift-2600: 0.9.2 release", ""), + {"THRIFT-2600"}, + ) + + class CleanSubjectTests(unittest.TestCase): """A commit line shows the subject without its ticket and trailers.""" @@ -555,6 +597,45 @@ def test_ticket_resolved_without_a_fix_is_listed_by_commit(self): draft, _ = self.generate(issues, jira_version=jira_version) self.assertEqual(sections_of(draft), expected) + def test_version_string_in_a_commit_body_costs_no_ticket(self): + # Mirrors da6ed655d. "thrift-0.24.0/..." was read as THRIFT-0, and + # JIRA then rejected the whole lookup, THRIFT-6183 included. + self.commit_6183() + self.commit( + "Add cstddef include to fix build error with 6.3.0 (#3801)\n" + "\n" + "thrift-0.24.0/lib/cpp/src/thrift/transport/TBufferTransports.h:110:32:\n" + " error: 'ptrdiff_t' does not name a type\n" + "\n" + "Client: cpp\n" + ) + draft, log = self.generate({"THRIFT-6183": TICKET_6183}) + self.assertEqual(sections_of(draft), {"C++": [ + self.CPP_6183, + "- [#3801](https://github.com/apache/thrift/pull/3801)" + " - Add cstddef include to fix build error with 6.3.0", + ]}) + self.assertNotRegex(log, r"\bTHRIFT-0\b") + + def test_unknown_ticket_key_costs_no_other_ticket(self): + # A key that does not exist, such as a typo, must not hide the + # tickets that other commits reference. + self.commit_6183() + self.commit( + "Clarify the frame size documentation (#3900)\n" + "\n" + "Follows up on THRIFT-99999.\n" + "\n" + "Client: cpp\n" + ) + draft, log = self.generate({"THRIFT-6183": TICKET_6183}) + self.assertEqual(sections_of(draft), {"C++": [ + self.CPP_6183, + "- [#3900](https://github.com/apache/thrift/pull/3900)" + " - Clarify the frame size documentation", + ]}) + self.assertIn("THRIFT-99999", log) + if __name__ == "__main__": unittest.main()