ci(release): generate release notes against an explicit previous tag - #588
Conversation
github-actions
Bot
added
the
[Type] Build Tooling
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/588")Built from 7614c35 |
aabd91a to
528e52e
Compare
Release notes have restated every PR back to v0.16.0 since v0.17.1. GitHub infers the notes base by walking tags newest-to-oldest and taking the first whose commit is an ancestor of the one being released. Our release tags never satisfy that: each points at a Package.swift rewrite committed on a local release/vX.Y.Z branch that is never pushed, so no release tag is reachable from any other. GitHub falls back to v0.16.0 — the last tag that does sit on trunk, created before this flow existed. Resolve the base explicitly instead. set_github_release cannot express previous_tag_name, so call the generate-notes endpoint directly and pass the result through as `description`. The base is the most recent stable release older than the version being published; prereleases are skipped as candidates, matching GitHub's default. Resolution is also run in `validate`, before anything is published, so a wrong base surfaces while a re-run is still free. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
528e52e to
cccfe3d
Compare
AliSoftware
left a comment
AliSoftware
left a comment
There was a problem hiding this comment.
Most of this is already available via some actions we expose in release-toolkit, so no need to reinvent the wheel 😉
| # Reads the Releases API rather than local tags: CI checkouts may not have | ||
| # fetched every tag, the API reports `prerelease` authoritatively, and it ignores | ||
| # stray tags never published as releases (e.g. `vtest-s3-xcframework-*`). | ||
| def previous_release_tag(version:, token:) |
There was a problem hiding this comment.
release-toolkit for that already
So no need to re-implement via API calls in your Fastfile.
There was a problem hiding this comment.
Thanks for the guidance! 🙇🏻♂️
Unfortunately, it seems find_previous_tag may not satisfy the need here. It uses git describe --tags --abbrev=0, which only finds tags reachable from the current commit—the same constraint that breaks GitHub's own inference. Our release tags are each committed on a local release/vX.Y.Z branch that's never pushed, so no release tag is an ancestor of any other and it always falls back to the last tag on trunk:
RELEASE find_previous_tag previous_release_tag
v0.17.2 v0.16.0 v0.17.1
v0.18.0 v0.16.0 v0.17.2
v0.18.1 v0.16.0 v0.18.0
v0.19.0 v0.16.0 v0.18.1
v0.20.0-alpha.0 v0.16.0 v0.19.0
For v0.20.0-alpha.0 that's 25 PRs vs. the 3 that were actually new.
previous_release_tag asks a different question ("highest stable release below this version?"), which needs no ancestry. Reading the Releases API rather than local tags also means CI doesn't need every tag fetched, prerelease is authoritative rather than inferred from the name, and stray tags like vtest-s3-xcframework-* are ignored.
WYDT?
| # | ||
| # Fails loudly rather than falling back to auto-generated notes: a wrong base | ||
| # looks like a successful release, caught only by someone reading the page later. | ||
| def generated_release_notes(version:, token:) |
There was a problem hiding this comment.
release-toolkit so no need to re-implement it in your Fastfile either.
There was a problem hiding this comment.
Good call. I dropped the custom API call in c608c5e.
I went with GithubHelper#generate_release_notes—the helper that action wraps—rather than the action itself. get_prs_between_tags rescues every StandardError and returns the message as the changelog, so a transient API failure would publish a release whose notes read ❌ Error computing the list of PRs since v0.19.0: …. Since we push the tag before creating the Release, that needs to fail the step rather than land in the release body.
I suppose we could consider adding a fail_on_error: opt-out to the upstream action.
There was a problem hiding this comment.
Good catch.
I think adding a fail_on_error upstream would actually benefit all apps and is probably better approach if you're up for it!
There was a problem hiding this comment.
Thanks for the nudge. As you noted, I implemented fail_on_error: in wordpress-mobile/release-toolkit#772.
I deferred relying on this new parameter until a new release-toolkit release is published. Tracked in #591. I suggest we merge the current implementation to avoid incorrect GBK release notes now.
Replace the hand-rolled `releases/generate-notes` API call with the release-toolkit's `GithubHelper#generate_release_notes`, and fold the duplicated previous-tag nil check into a `previous_release_tag!` wrapper shared by `validate` and the publish lane. Uses `GithubHelper` rather than the `get_prs_between_tags` action that wraps it: the action rescues every `StandardError` and returns the message as the changelog, which would publish a release whose notes read "❌ Error computing the list of PRs…". The tag is already pushed by that point, so the failure needs to fail the step instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AliSoftware
left a comment
AliSoftware
left a comment
There was a problem hiding this comment.
I'll try and take a closer look at it on my tomorrow. But ata glance I think it makes indeed sense to use the gh_api for finding last release's tag given those are not ancestors
| candidates.select { |tag| Gem::Version.new(tag.delete_prefix('v')) < target } | ||
| .max_by { |tag| Gem::Version.new(tag.delete_prefix('v')) } |
There was a problem hiding this comment.
I haven't thought about this deeply, but I wonder how it would behave (and how we want it to behave) for cases of hotfixes 🤔
Say:
- On week 1, we publish 12.3
- On week 3, we publish 13.0
- On week 4, we publish 12.3.1 hotfix
- On week 5, we publish 13.1
- On week 6, we publish 13.1.1
- On week 7, we publish 13.2
Then:
- Should the 12.3.1 hotfix use 12.3 or 13.0 as previous tag?
- Should 13.1 use 12.3.1 or 13.0 as previous tag?
- Shoild 13.2 use 13.1.1 or 13.1 as previous tag?
I think you cover it here by only considering versions before current as candidates (so 13.0 won't be a candidate for 12.3.1, and while 12.3.1 will be a candidate for 13.1 because the ts < target it'll be shadowed by 13.0 having a higher version.
But that will still make 13.2 use 13.1.1 as previous tag… which might be what we want, but maybe it (maybe it'd make more sense to pick 13.1 as latest non-hotfix instead?) 🤔🤷♂️
Anyway, since I'm trying to think of edge cases I wanted us to double-check such possible edge cases we might not have thought of at first (especially because it's too late for me to think clearly enough about those right now 😅😴)
There was a problem hiding this comment.
Thank you for thinking through this so thoroughly. I agree with your v12.3.1 and v13.1.0 choices. I believe v13.2.0 should choose v13.1.1 as the latest stable release.
I had Claude verify our current logic achieves this, and it claimed it will. If future issues arise in release notes, we can fix them and adjust this logic accordingly.
AliSoftware
left a comment
AliSoftware
left a comment
There was a problem hiding this comment.
I still think it'd be nicer to add a fail_on_error: parameter to the generate_release_notes action in release-toolkit so we can use that action directly instead of the Helper.
Otherwise, the logic LGTM, and I'm ok with using the github_api directly to get the latest stable release (I simply suggested an implementation improvement)
| candidates = releases.reject { |release| release['draft'] || release['prerelease'] } | ||
| .map { |release| release['tag_name'] } | ||
| .reject { |tag| tag == version } | ||
| .select { |tag| tag =~ /\Av\d+\.\d+\.\d+\z/ } | ||
|
|
||
| target = Gem::Version.new(version.delete_prefix('v').split('-').first) | ||
| candidates.select { |tag| Gem::Version.new(tag.delete_prefix('v')) < target } | ||
| .max_by { |tag| Gem::Version.new(tag.delete_prefix('v')) } |
There was a problem hiding this comment.
Not tested, but I think this small refactor might work the same while being a bit more compact (and avoiding to redo the Gem::Version.new(tag.delete_prefix('v') twice)
| candidates = releases.reject { |release| release['draft'] || release['prerelease'] } | |
| .map { |release| release['tag_name'] } | |
| .reject { |tag| tag == version } | |
| .select { |tag| tag =~ /\Av\d+\.\d+\.\d+\z/ } | |
| target = Gem::Version.new(version.delete_prefix('v').split('-').first) | |
| candidates.select { |tag| Gem::Version.new(tag.delete_prefix('v')) < target } | |
| .max_by { |tag| Gem::Version.new(tag.delete_prefix('v')) } | |
| candidates = releases.reject { |release| release['draft'] || release['prerelease'] } | |
| .map { |release| release['tag_name'] } | |
| .reject { |tag| tag == version } | |
| .select { |tag| tag =~ /\Av\d+\.\d+\.\d+\z/ } | |
| .map { |tag| Gem::Version.new(tag.delete_prefix('v')) } | |
| target = Gem::Version.new(version.delete_prefix('v').split('-').first) | |
| candidates.select { |tag_version| tag_version < target }.max |
There was a problem hiding this comment.
Implemented this spirit of this (deduplication) in 223f319. Claude adjusted the implementation to address an issue flagged with the suggestion that resulted in 400 responses from the GitHub API due to the logic returning an incorrect tag name (e.g., 0.19.0 instead of v0.19.0).
There was a problem hiding this comment.
Ah good catch, we max by the Gem::Version but we want to return the tag name not the parsed version 👍
PS: Technically I think we could have used max_by(&:last) as a shorter version instead of .max_by { |_tag, tag_version| tag_version }… but that's probably too nitpicky at this point 😉 😜
`previous_release_tag` built a `Gem::Version` three times per candidate: once to compare against the target, once to rank, and once more in the regex filter's neighbouring block. Pair each tag with its parsed version up front, then destructure in the comparison blocks. The pairing keeps the tag string available, so the method still returns `v0.19.0` rather than the bare `0.19.0` that `previous_tag_name` rejects with a 400. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Note the trigger for refactoring this code to avoid this code lingering for longer than necessary. Co-authored-by: Olivier Halligon <olivier@halligon.net>
4 participants
What?
Generated release notes restated every PR back to
v0.16.0on every release sincev0.17.1. This makes the release lane pass an explicit previous tag instead of letting GitHub infer one.The already-published notes have been regenerated separately (see below), so this PR is what stops the next release from reintroducing the problem.
Why?
Every GitHub Release from
v0.17.1throughv0.20.0-alpha.0listed months of already-shipped work —v0.20.0-alpha.0showed 24 PRs where only 3 were new — and all of them reportedcompare/v0.16.0....GitHub picks the notes base by walking tags newest-to-oldest and taking the first whose commit is an ancestor of the one being released. Our release tags never satisfy that.
publish_release_to_githubtags aPackage.swiftrewrite committed on a localrelease/vX.Y.Zbranch that is never pushed, so no release tag is reachable from any other:GitHub falls back to
v0.16.0and re-lists everything since. Releases beforev0.17.1were unaffected — they predate this tagging flow, so the inference worked.The off-trunk tag placement is deliberate and documented — it's what lets the tag carry a
Package.swiftpointing at the prebuilt XCFramework whiletrunkkeeps.local. This PR leaves that design alone and fixes the notes instead.How?
set_github_releasecan't expressprevious_tag_name(it only forwards agenerate_release_notesboolean), sogenerated_release_notescalls thereleases/generate-notesendpoint directly and passes the result through asdescription.previous_release_tagresolves the base as the most recent stable release older than the version being published. Prereleases are skipped as candidates, matching GitHub's default — a stable release reports everything since the last stable release, including work already listed in its own alphas.It reads the Releases API rather than local tags: CI checkouts may not have fetched every tag, the API reports
prereleaseauthoritatively rather than us inferring it from the tag name, and it ignores stray tags never published as releases (vtest-s3-xcframework-*).Resolution fails loudly if no base is found. A silently wrong base looks like a successful release and is only caught by someone reading the release page later.
validatealso resolves and logs the base. That step runs before anything is published, so a wrong base surfaces while a re-run is still free. It recomputes rather than threading the value through, because Buildkite runsvalidateas a separate step — separate agent, separate process, no sharedlane_context.Testing Instructions
It's not possible to test this without publishing a new release. We'll just need to verify the release notes of the next few releases when they occur.
Already-published notes
Regenerated ahead of this PR, oldest first, so the history reads correctly today rather than waiting on a merge:
v0.17.1v0.16.0v0.17.2v0.16.0v0.17.1v0.18.0v0.16.0v0.17.2v0.18.1v0.16.0v0.18.0v0.19.0-alpha.0v0.16.0v0.18.1v0.19.0v0.16.0v0.18.1v0.20.0-alpha.0v0.16.0v0.19.0Only the release
bodychanged — tags, assets, checksums, and prerelease flags are untouched, so nothing SPM consumers resolve is affected.