feat: Add release.sh script and detect breaking changes
#5366
Conversation
This commit introduces three new scripts: - `release.sh` To be run by a user on their local machine to preview and create a new release (tag + push) - `check_commit_metadata.sh` For e.g. detecting breaking changes - `genereate_release_notes.sh` To display the generated release notes, used for previews and in `publish_release.sh` The `release.sh` script can be run without arguments, and it will automatically determine if we're to do a patch or minor release. A minor release can be forced via `--minor` flag. Breaking changes can be annotated either via commit/merge title prefix (`feat!:`, `feat(api)!:`), or by adding the `release/breaking` label to the PR that was merged (on GitHub). Related #5233 This work will be followed up by changes to move the tag creation from `release.sh` to CI workflows, the `release.sh` will then become a lightweight wrapper to run the workflow.
|
Looks great!! What do you think about failing the release workflow if the user did a manual |
I'm definitely supportive of that idea. Do you think we should have it in this PR already or is it OK to leave to a follow up one? I'm going to do some workflow changes and a bit more refactoring of One easy way to throw it into this PR would be to change the tag message |
A follow-up one (and issue) makes sense! I wouldn't consider it a blocker at all :) |
scripts/check_commit_metadata.sh
Outdated
| @@ -0,0 +1,108 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
WDYT about placing all these scripts in the separate release directory?
There was a problem hiding this comment.
I guess we have enough release related scripts to put them there, I'd still like to keep /scripts/release.sh (because used by humans) but the rest can go in /scripts/release/*.sh.
scripts/check_commit_metadata.sh
Outdated
| continue | ||
| fi | ||
|
|
||
| if [[ $commit_prefix =~ $prefix_pattern ]]; then |
There was a problem hiding this comment.
nit: I still have a feeling that we should switch to Python or mage at this point :)
There was a problem hiding this comment.
I won't get behind Python, but I'm indifferent to mage, so it's not me you have to convince. 😉
scripts/check_commit_metadata.sh
Outdated
| # | ||
| # Example: ./check_commit_tags.sh v0.13.1..971e3678 | ||
| # | ||
| # When sourced, this script will populate the COMMIT_METADATA_* variables |
There was a problem hiding this comment.
Applicable only to squash & merge - Do you think that we may need another script executed in CI to check if the title of PR is consistent?
There was a problem hiding this comment.
Hmm, what do you mean by Applicable only to squash & merge? I guess we could make it clearer this script is only intended for the commit log on the main branch.
Yes, we'll add an action/lint for PR title 👍🏻 (follow up PR)
There was a problem hiding this comment.
Yes, we'll add an action/lint for PR title 👍🏻 (follow up PR)
👍
Applicable only to squash & merge?
I thought that Create a merge commit option is also enabled for this repo, but it seems to be off. If it's on, it might be more challenging to validate also commits than only the PR title.
There was a problem hiding this comment.
Ah right, yeah we'll only validate PR titles, I made the assumption when writing that we'll only squash merge.
Obviously owners/admins could bypass this by pushing straight to main. 🙄
Co-authored-by: Marcin Tojek <mtojek@users.noreply.github.com>
It's not necessarily hard per-se, we can just
I guess the biggest risk is in potentially not detecting a breaking change and only doing patch release if something goes wrong or a future change in the script breaks it. |
190f7ed to
0f5883f
Compare
scripts/lib.sh
Outdated
| if issourced; then | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
can you put a comment here explaining why this is necessary?
There was a problem hiding this comment.
Actually, I'll just remove it. It's not really necessary if we always want to exit on error. I had the scripts set up in a different way before.
| fi | ||
|
|
||
| # Get the labels for the PR associated with this commit. | ||
| mapfile -t labels < <(gh api -H "Accept: application/vnd.github+json" "/repos/coder/coder/commits/${commit_sha}/pulls" -q '.[].labels[].name') |
There was a problem hiding this comment.
The rate limit for calls from github actions is 1k per hour per repository. I'd say an upper bound for the amount of PRs in a release would be 100, but combined with other things in the repo we might be pushing on 1k per hour during busy periods. Can this instead use a list endpoints?
The commits we release are a contiguous block on main so if you list from first ref to end ref then you will get them all in much less requests.
There was a problem hiding this comment.
I did consider this but it's not clear how we'd trigger it. Now I don't remember but do we run dry-run for every merged commit? I considered adding a --skip-label-check to this script used in dry-run scenarios, but omitted it for simplicity and because it would generate "false" release notes.
The commits we release are a contiguous block on main so if you list from first ref to end ref then you will get them all in much less requests.
The problem isn't listing the commits, but in that endpoint they won't include the metadata we're looking for (PR labels). I tried to find a way to get all of this information in one go, but each one has caveats.. 😅
We could use /repos/coder/coder/pulls, but even with filtering ?state=closed&base=main&per_page=100 (maybe sort=updated), we might have to keep traversing multiple pages to find all commits that we're looking for (say we have a month old PR that got merged).
The current API is also interesting in that, I'm speculating here, but it lists PRs related to the commit. As such I'm thinking maybe it would also list a PR that has been merged into a PR that then is merged into main. This could be useful.
But alas, that benefit is theoretical and ultimately, in most cases, not beneficial. The pulls endpoint will likely result in less requests so maybe we go with that.
There was a problem hiding this comment.
do we run dry-run for every merged commit
We do not
What if you instead listed all PRs that had the breaking label and then checked if the merge commit is from any of those PRs?
There was a problem hiding this comment.
@deansheather Thanks for your suggestion, this put me on the right path:
❯ old_tagged_commit=$(git rev-list -n 1 v0.13.2)
❯ old_commit_date=$(git show --no-patch --date=short --format=%cd "$old_tagged_commit")
# 2022-12-12
❯ gh pr list --base main --state merged --limit 9999 --search "merged:>=$old_commit_date" --json mergeCommit,labels --jq '.[] | .mergeCommit.oid + " " + ([.labels[].name] | join(" "))'
2b864cee9e71341dac86691805b9552b3277ff5f
1907f13c5f509f28be5101354f870a67a578a5e2
88bb901283055397c9d04d27ad51ce3d4417d645
08a6a182265e7351979defbce512e36a058006f0
e7fc21e28568b8ae0c9959a670cc7df237bd8aea
I think this is a pretty nice solution, and it'll be easily extensible in the future too. It's quite fast so I'd venture it only does one graphql lookup or some such. If we really wanted to we could add --label release/breaking, but we may want to look up other metadata in the future too. Although, if we have more than 9999 commits in a release, we're screwed 😅... So maybe we should start by filtering by the one label.
Could also consider using API endpoints straight (instead of gh pr list), but I couldn't find how to filter on coder/coder, kept getting results from all of GitHub ¯\_(ツ)_/¯.
(I'm hoping the gh command will support gh pr list in CI as well.)
There was a problem hiding this comment.
@deansheather this is now implemented in c871cb7 (#5366).
I opted to not filter the search results based on a single tag because I wanted to create a safety-check that ensures we fetched PR data for each commit.
NOTE: This is not the final form of how releases will be made, see below.
This commit introduces three new scripts:
release.shTo be run by a user on their local machine to preview andcreate a new release (tag + push)
check_commit_metadata.shFor e.g. detecting breaking changesgenereate_release_notes.shTo display the generated release notes,used for previews and in
publish_release.shThe
release.shscript can be run without arguments, and it willautomatically determine if we're to do a patch or minor release. A minor
release can be forced via
--minorflag.Breaking changes can be annotated either via commit/merge title prefix
(
feat!:,feat(api)!:), or by adding therelease/breakinglabel tothe PR that was merged (on GitHub).
Related #5233
This work will be followed up by changes to move the tag creation from
release.shto CI workflows, therelease.shwill then become alightweight wrapper to run the workflow.
Documented here: #5381, #5382
Example output: