-
Notifications
You must be signed in to change notification settings - Fork 1
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
Coexist with branch-protection: use Github App for pushing commits #26
Conversation
These permissions are insufficient to update version.sbt by git push: https://github.com/guardian/redirect-resolver/actions/runs/7854563439 |
These permissions are sufficient to update version.sbt by git push: https://github.com/guardian/redirect-resolver/actions/runs/7854575289 |
These permissions still allow us to update https://github.com/guardian/redirect-resolver/actions/runs/7854597175 It looks like we can't control the files that a git push can update using the 'single file' permission - maybe it only controls changes performed using the https://docs.github.com/en/rest/repos/contents?apiVersion=2022-11-28#create-or-update-file-contents |
348a63f
to
9735866
Compare
f5926c1
to
b5df698
Compare
ce0054f
to
3304c42
Compare
a0f95fe
to
af9e65a
Compare
3013233
to
3ead2c8
Compare
This changes the way we authenticate and make the 1 or 2 updates to `version.sbt` required for a release. Before: * Authenticate as: default `github-actions` bot * version.sbt update method: Cherry-pick the commits created by sbt-release, then push them to GitHub using `git push`, with the default `github-actions` bot using its credentials to make the push After: * Authenticate as: `gu-scala-library-release` GitHub App - https://github.com/apps/gu-scala-library-release * version.sbt update method: GitHub REST API for Repository Contents (PUT /repos/{owner}/{repo}/contents/{path}) https://docs.github.com/en/rest/repos/contents?apiVersion=2022-11-28#create-or-update-file-contents Now we're not really specifying the whole commit, just the content change to one file. This has a few different benefits: * Addresses the need to coexist with our branch-protection rulesets, because GitHub Apps can be exempted from rules, as discussed in issue #5 * Produces `Verified` commits - the commits show up as `Verified` in the GitHub UI, and have a `gpgsig` header entry that is signed by GitHub itself, essentially GitHub attesting that the author of the commit authenticated with GitHub to perform the file update. https://git-scm.com/docs/signature-format#_commit_signatures https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification#signature-verification-for-bots https://blog.gitbutler.com/signing-commits-in-git-explained/#github-verification The commits now appear to be attributed to `gu-scala-library-release`, rather than, eg, '@rtyley using gha-scala-library-release-workflow' - it's a bit of shame that the person triggering the release is no longer so clearly visible, but it's probably less confusing. To compensate for that, the commit message itself has been updated to specifically state the responsible user.
3ead2c8
to
26bf362
Compare
This change passes on the new `AUTOMATED_MAVEN_RELEASE_GITHUB_APP_PRIVATE_KEY` Organisation Secret required for guardian/gha-scala-library-release-workflow#26 - a GitHub App **private** key for https://github.com/apps/gu-scala-library-release . This allows `gha-scala-library-release-workflow` to work with repos that use branch-protection rulesets that prevent most users from pushing to the default (`main`) branch - the workflow can authenticate as our GitHub App, and our GitHub App can be granted permission to bypass the ruleset, so that it can push to `main` to make a release, while developers can not!
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.
Looks great! (Also reviewed by @tjsilver and @chrislomaxjones )
description: | ||
"App ID for a GitHub App that is allowed to push directly to the default branch. Eg, App ID on: | ||
https://github.com/organizations/guardian/settings/apps/gu-scala-library-release" | ||
default: '807361' # Only for use by the Guardian! |
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.
could we make this value a repository secret? It's not top secret info, but I'm not sure we want it to be public.
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'd really prefer GITHUB_APP_ID
to not be a secret if possible! Every repository/organisation secret we add needs to be passed down through every release.yml
in every repo that uses gha-scala-library-release-workflow
- so 1 additional secret becomes ~50 additional lines of config, spread across the Guardian's estate - I've really tried as hard as I can to make sure that the boilerplate release.yml that gets added to each project that uses gha-scala-library-release-workflow
is as minimal as possible.
GitHub's guidance, in both their documentation for "Making authenticated API requests with a GitHub App in a GitHub Actions workflow", and the official actions/create-github-app-token GitHub Action we use to create the GitHub App token, is to treat only the private key as secret, and to treat the App Id as a regular input, eg as an environment variable, which will not be redacted:
- Store the app ID of your GitHub App as a GitHub Actions configuration variable.
- Generate a private key for your app. Store the contents of the resulting file as a secret.
@@ -142,7 +156,15 @@ jobs: | |||
release_tag: ${{ steps.create-commit.outputs.release_tag }} | |||
release_version: ${{ steps.create-commit.outputs.release_version }} | |||
release_commit_id: ${{ steps.create-commit.outputs.release_commit_id }} | |||
version_file_path: ${{ steps.create-commit.outputs.version_file_path }} |
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.
is it possible to specify the output after the steps for easier reading? from @chrislomaxjones
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.
That's an interesting idea, I can see where you're coming from! If we put the outputs:
stanza after the steps:
stanza in a job, it will tend to come closer to where the output is actually generated, though not in every case - for instance, in the 🔒 Init
job, more of the output is actually output in the 1st step of the job rather than the 2nd.
In GitHub's documentation for outputs:
they put the outputs:
stanza before the steps:
, so I guess that's the convention I was going with.
Conceptually, we might also think of output as the thing that comes at the end of a chunk of code - like a return
statement. However, the outputs:
stanza can also be thought of as being like part of the signature of a function, where a GitHub job corresponds to the entirety of the 'function' - something that accepts inputs and gives an output. In many programming languages, including Typescript and Scala, we're used to a function declaration having the method name, its inputs, and then its output declaration, all before the actual implementation of the function starts:
func isTextWellFormatted(text: string): boolean {
...lots and lots of code...
}
This lets someone looking for an overview look at the beginning of a function declaration, and see the key things about it, without having to read the entire implementation. Of course, for something that has lots of side-effects - like most GitHub Jobs - you still end up having to read the implementation to understand everything it's doing, but at least it's something!
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.
Thanks! I will merge this after lunch hopefully...
description: | ||
"App ID for a GitHub App that is allowed to push directly to the default branch. Eg, App ID on: | ||
https://github.com/organizations/guardian/settings/apps/gu-scala-library-release" | ||
default: '807361' # Only for use by the Guardian! |
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'd really prefer GITHUB_APP_ID
to not be a secret if possible! Every repository/organisation secret we add needs to be passed down through every release.yml
in every repo that uses gha-scala-library-release-workflow
- so 1 additional secret becomes ~50 additional lines of config, spread across the Guardian's estate - I've really tried as hard as I can to make sure that the boilerplate release.yml that gets added to each project that uses gha-scala-library-release-workflow
is as minimal as possible.
GitHub's guidance, in both their documentation for "Making authenticated API requests with a GitHub App in a GitHub Actions workflow", and the official actions/create-github-app-token GitHub Action we use to create the GitHub App token, is to treat only the private key as secret, and to treat the App Id as a regular input, eg as an environment variable, which will not be redacted:
- Store the app ID of your GitHub App as a GitHub Actions configuration variable.
- Generate a private key for your app. Store the contents of the resulting file as a secret.
@@ -142,7 +156,15 @@ jobs: | |||
release_tag: ${{ steps.create-commit.outputs.release_tag }} | |||
release_version: ${{ steps.create-commit.outputs.release_version }} | |||
release_commit_id: ${{ steps.create-commit.outputs.release_commit_id }} | |||
version_file_path: ${{ steps.create-commit.outputs.version_file_path }} |
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.
That's an interesting idea, I can see where you're coming from! If we put the outputs:
stanza after the steps:
stanza in a job, it will tend to come closer to where the output is actually generated, though not in every case - for instance, in the 🔒 Init
job, more of the output is actually output in the 1st step of the job rather than the 2nd.
In GitHub's documentation for outputs:
they put the outputs:
stanza before the steps:
, so I guess that's the convention I was going with.
Conceptually, we might also think of output as the thing that comes at the end of a chunk of code - like a return
statement. However, the outputs:
stanza can also be thought of as being like part of the signature of a function, where a GitHub job corresponds to the entirety of the 'function' - something that accepts inputs and gives an output. In many programming languages, including Typescript and Scala, we're used to a function declaration having the method name, its inputs, and then its output declaration, all before the actual implementation of the function starts:
func isTextWellFormatted(text: string): boolean {
...lots and lots of code...
}
This lets someone looking for an overview look at the beginning of a function declaration, and see the key things about it, without having to read the entire implementation. Of course, for something that has lots of side-effects - like most GitHub Jobs - you still end up having to read the implementation to understand everything it's doing, but at least it's something!
After merging this PR, I've manually granted the Guardian's installation of GU Scala Library Release repository access to the 24 repositories that are currently set up to use The next step is to update all the corresponding release.yml files...! |
Following on from #26, which introduced the need to have a GitHub App, this updates the docs further to explain that the GitHub App must be granted access to each repo that it acts upon. If the GitHub App does not have access to the repo, the release flow will fail, like https://github.com/guardian/content-entity/actions/runs/8347150774 : > Maven Release / 🔒 Push Release Commit > Not Found - https://docs.github.com/rest/apps/apps#get-a-repository-installation-for-the-authenticated-app
This should fix #37 - ie, a Full Main-Branch release should still succeed, even if someone adds additional commits to the `main` branch while the release workflow is running. Note that this change in behaviour only applies to Full Main-Branch releases, not Preview releases (#19), as Preview releases don't ever attempt to update any branch. To avoid the noise of an additional merge commit (ie a 3rd commit) we prefer a fast-forward, and only create a merge commit if it's _necessary_ - ie because we have to merge on top of some commits that someone else has added. ## Two full Main-Branch releases can't run concurrently Although we can now handle extra work on the `main` branch, running **two releases at once** is _not_ recommended! ## Using the GitHub API to create & merge commits, rather than `git` We're now using the GitHub API, rather than `git`, to do both these operations: * [Update the branch](https://docs.github.com/en/rest/git/refs?apiVersion=2022-11-28#update-a-reference) to try fast-forwarding - using the GitHub API means we don't need to clone a deep history of the repo to make that update * [Merge the branch](https://docs.github.com/en/rest/branches/branches?apiVersion=2022-11-28#merge-a-branch) if the fast-forward fails - using the GitHub API means that the merge commit will be signed/verified by GitHub too, [just like other commits created by the GitHub App](#26).
This fixes a small documentation mistake I made in #26, where usage of a Github App was introduced. I supplied a link in the docs to help create a new GitHub App, this link was supposed to pre-populate the form with the 3 necessary permissions (Contents: Read and write, Pull Requests: Read and write, and Metadata: Read-only). However, checking it now, it only provides 2 of 3, and the 'Pull Requests' permission wasn't set. It turned out that I should have been using `pull_requests=write` in the url, rather than `pull-requests=write` - this isn't documented, so far as I can see: https://docs.github.com/en/apps/sharing-github-apps/registering-a-github-app-using-url-parameters ...but in the end I managed to guess it!
You can just click this link to get taken to a pre-filled page to create a new GitHub App - you'll just need to | ||
customise the app name: | ||
|
||
https://github.com/settings/apps/new?name=scala-library-release&url=https://github.com/guardian/gha-scala-library-release-workflow&public=false&contents=write&pull-requests=write&webhook_active=false |
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.
Whoops, this should have been pull_requests=write
(with an underscore), not pull-requests=write
- fixed in #41
This fixes #5, addressing the need to coexist with branch-protection rulesets. Rulesets can allow specific Github Apps to bypass rules - so our GitHub App can push to
main
to do a release, while developers can not!I've created
gu-scala-library-release
as a GitHub App to do the pushing, as well as PR comments & creation of GitHub Releases.Testing
@NovemberTang setup a 'branch protection test' ruleset for me, which for testing targeted the guardian/redirect-resolver repo and prevented pushing on to the default branch:
Using this PR (see repo config) & the
gu-scala-library-release
GitHub App,gha-scala-library-release-workflow
was able to successfully perform a release and bypass the ruleset to push onto the default branch ✨Changes
New GitHub App private key
In order for the release workflow to authenticate and perform actions as the GitHub app, each repo needs access to the new
AUTOMATED_MAVEN_RELEASE_GITHUB_APP_PRIVATE_KEY
Organisation Secret, and to pass it on in their call to the reusable workflow. This necessitates these changes:gha-scala-library-release-workflow
need to update theirrelease.yml
to pass the additional secret value on. Pass creds forgu-scala-library-release
GitHub App etag-caching#42 is an example of the change. I'll apply this rapidly to all repos on merging this PR.Presentation of Releases
Commits
Before
Unverified
in the GitHub UI, because GitHub can not find the PGP key for the supposed author/committer of the commit,@rtyley using gha-scala-library-release-workflow <[email protected]>
(gha-scala-library-release-workflow
constructed that username to be 'informative', but there is no GitHub account that actually has that email and username!)@rtyley
is in the committer name, the commit message itself does not @-mention @rtyley, and so does not send a GitHub mention-notification)After
The commits are now authored by
@gu-scala-library-release[bot]
, and committed & signed by GitHub themselves, because we're making theversion.sbt
changes using the GitHub API for Repository ContentsThe last commit (the post-release snapshot-version commit - not the first commit that marks the release) @-mentions the person who initiated the release. This will result in them getting an email notification like this:
One email notification is probably enough, so we deliberately don't @-mention the user in the first commit. For a PREVIEW release, there's only the first commit, but we still don't @-mention the user in the commit - they'll still get an email notification from the PR comment made by the bot.
Because, for some reason, the email text removes the newlines in the commit message, smushing it together into a single line, we've deliberately kept the 2nd commit message short and limited the number of links included.
The release tag, which tags the release commit, is still signed by the artifact-signing GPG key - consequently commit verification can still be performed without relying on GitHub's GPG key. The tagger name is still the informative one constructed by
gha-scala-library-release-workflow
:PR Comments
PR comments no longer use the generic
github-actions
bot identity, using the distinctivegu-scala-library-release
one instead:Before
After
Execution of
version.sbt
updatesBefore
The 1 or 2 git
version.sbt
update commits would be constructed in a local copy of the git repo by the workflow at the🔒 Push Release Commit
stage. Depending on the release type:FULL_MAIN_BRANCH
: the 2 commits would begit push
ed directly onto the default (main
) branchPREVIEW_FEATURE_BRANCH
: the 1 commit would be tagged with a 'preliminarytag, and that tag
git push`ed to GitHub (not modifying the feature branch). The preliminary tag would not get cleaned up after release.After
A temporary
release-workflow/temporary/${{ github.run_id }}
branch is created (and cleaned up at the end of the workflow run), based on whatever branch we're running on. The release update toversion.sbt
is made using the GitHub API for Repository Contents at the🔒 Push Release Commit
stage, pushing onto the temporary branch.Subsequently, in the
🔒 Sign
stage, the signed release tag is pushed for the release commit, and, if this is aFULL_MAIN_BRANCH
release, themain
branch is fast-forwarded to the release commit. This marks the point at which we're committed to actually doing the release - if we somehow fail to make the release, we end up in an inconsistent state.Finally, in the
🔒 Update GitHub
stage, we clean-up (delete) the temporary branch, and, for aFULL_MAIN_BRANCH
release, make the 2nd & finalversion.sbt
update (with the GitHub API again), setting the version to a -SNAPSHOT again, ready for future dev work.Permit customised-permissioning for tests in release?
The temporary
release-workflow/temporary/*
branches open up a possible improvement togha-scala-library-release-workflow
in a subsequent PR. Rather than executing the tests ourselves, in the🎊 Test & Version
phase, we could instead configure the CI workflow to execute onrelease-workflow/temporary/*
branches, and allow the test phase to run in parallel - in a separate workflow - while the🎊 Create artifacts
workflow is running. This would allow running tests during release that use CI with a customised-permissions workflow (see egfacia-scala-client
) without trying to shoehorn that customisation into thegha-scala-library-release-workflow
flow itself.