-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
github: Add release workflow #4
Conversation
1b8fc09
to
fd087e7
Compare
ebb5c8a
to
6d82f4f
Compare
fd087e7
to
dd575d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mostly going to hold off reviewing until #3 is landed, since a lot of the same comments apply. Some high level comments though:
- Avoid actions that haven't gone through a security review. These run in a very privileged position, affecting the release of software that has access to sensitive data. We should be extremely cautious here. Let's script this ourselves.
- Let's try and do this with a single workflow if we can. I'd be fine with only
release.yml
.
75b1a87
to
83613d6
Compare
dd575d8
to
f614276
Compare
Compacted everything down to a single workflow that triggers a custom script - PTAL |
83613d6
to
a25b7c0
Compare
f614276
to
859e117
Compare
|
||
set -euo pipefail | ||
|
||
readonly RELEASE_VERSION="$1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check that exactly 1 argument was passed in, report an error if not.
Check that the version follows semver.
Verify that HEAD
is either on main
or release/vX.Y
where vX.Y.
is a prefix of $RELEASE_VERSION
. (It must not be possible to release from an unprotected branch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
infra/release.sh
Outdated
//:release_artifacts | ||
|
||
# Create release | ||
gh release create \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have the gh
command installed in the CI environment. Can do you do this step instead with the actions/create-release
or with actions/github-script
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some code to download + verify the GH CLI on-the-fly; added a TODO to see if we can get this action in a Docker container as a follow-on
859e117
to
236f790
Compare
236f790
to
bc03e17
Compare
infra/release.sh
Outdated
fi | ||
|
||
readonly RELEASE_VERSION="$1" | ||
readonly GH_CLI_URL='https://github.com/cli/cli/releases/download/v2.52.0/gh_2.52.0_linux_amd64.tar.gz' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy this file to the engflow-tools-public
GCS bucket. We vendor binaries and installable packages there so that we still have a copy if it changes or becomes unavailable.
You can copy there with gsutil cp gh_2.52.0_linux_amd64.tar.gz gs://engflow-tools-public/github.com/cli/cli/releases/download/v2.52.0/gh_2.52.0_linux_amd64.tar.gz
. Use the URL as the blob key so we know where it came from.
You can download it here by prepending storage.googleapis.com/engflow-tools-public/
to the URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have permissions to write to engflow-tools-public
:/
Are permissions controlled by terraform? Grepping through prod
and engflow
repos doesn't reveal any controlling terraform; I also can't read IAM permissions on the bucket to see who has access currently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've uploaded this for now to speed things along. Ulf or Corbin can grant you access though.
infra/release.sh
Outdated
readonly GH_CLI_EXPECTED_SHA256='3ea6ed8b2585f406a064cecd7e1501e58f56c8e7ca764ae1f3483d1b8ed68826' | ||
|
||
# Check that supplied version string follows semver | ||
grep '[0-9]\+\.[0-9]\+\.[0-9]\+' <<<${RELEASE_VERSION} || { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the user should have to specify a version number that begins with v
. Go version numbers always must start with v
, so it's surprising that they're not allowed to include that here, but we add it automatically elsewhere.
Also make sure to accept prerelease versions. semver.org has a suggested regex, but you can omit the build metadata group (Go doesn't allow that in tags). You'll likely need egrep
to parse that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable - we're no longer editing strings in files, so we can tolerate a version string with a prepended v
everywhere.
Stole the regex from semver.org - but v
isn't part of semver I guess, so that's prepended to the regex.
infra/release.sh
Outdated
readonly EXPECTED_RELEASE_BRANCH="$(sed 's|\([0-9]\+\.[0-9]\+\)\.[0-9]\+|release/v\1|' <<<${RELEASE_VERSION})" | ||
git branch \ | ||
--contains "$(git rev-parse HEAD)" \ | ||
| grep "main|${EXPECTED_RELEASE_BRANCH}" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to either use egrep
or escape the |
character in the regexp.
I'd recommend using egrep
as well as sed -E
so you don't have to escape the | ( ) +
operators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
infra/release.sh
Outdated
readonly GH_CLI_EXPECTED_SHA256='3ea6ed8b2585f406a064cecd7e1501e58f56c8e7ca764ae1f3483d1b8ed68826' | ||
|
||
# Check that supplied version string follows semver | ||
grep '[0-9]\+\.[0-9]\+\.[0-9]\+' <<<${RELEASE_VERSION} || { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I promised not to nitpick so much, but I think the ||
here is difficult to read. Using an if
statement would be clearer.
I'd also suggest using --quiet
so the grep
output isn't printed to stdout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed both
infra/release.sh
Outdated
exit 1 | ||
} | ||
|
||
# Add a tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure creating a release will also create a tag, so this may prevent that last call from working. In any case, don't create the tag until everything else has succeeded, certainly not until after the build has succeeded. Go versions cannot be modified once they're published, so this has to be the last step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looks like the docs say that either the tag is existing when you create the release, or there's an option to create the tag with the release. GH CLI docs are a bit more nebulous - there's a --verify-tag
flag that asserts the tag already exists, suggesting that tag creation is the default behavior.
Removed the tag create and push steps
54ee019
to
c3929aa
Compare
infra/release.sh
Outdated
fi | ||
|
||
readonly RELEASE_VERSION="$1" | ||
readonly GH_CLI_URL='https://github.com/cli/cli/releases/download/v2.52.0/gh_2.52.0_linux_amd64.tar.gz' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've uploaded this for now to speed things along. Ulf or Corbin can grant you access though.
.github/workflows/release.yml
Outdated
|
||
jobs: | ||
release: | ||
runs-on: ubuntu-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's run this on our own runners now that they're set up for the other workflows. You should be able to use the same runs-on configuration for linux / x64.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
This change implements two workflows that together implement the release process. The first workflow is manually triggered, takes a human-specified semver version string, and creates an automated commit that updates the version string in the required places, and then tags the new commit. The second workflow is triggered on release semver tags, and performs the build and creates the release. Tested: no - these workflows are hard to test manually, and will likely require trial+error on Github itself. Bug: linear/CUS-332
c3929aa
to
f9e2cca
Compare
This change adds a workflow that implements the release process. The majority of the release process is done by a bash script, which uses git, bazel, and the Github CLI to:
Tested: able to manually test some of the validation steps using
SCRIPT_DEBUG
:gh
CLI runs and finds files, but release creation is unsuccessful; likely due to permissions)Bug: linear/CUS-332