-
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
Add Continuous Integration (CI) as GitHub Action #272
Conversation
be0504a
to
ae94153
Compare
.github/workflows/ci.yml
Outdated
# The AWS role is configured as a GitHUb Repo secret, the value is the cloudformation-output of the | ||
# 'Facia-Scala-Client-CI-Role-Provider' cloudformation stack | ||
role-to-assume: ${{ secrets.AWS_ROLE_FOR_TESTS }} |
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've got a convention of making the AWS Role ARN a secret (tho' the system should be secure even if the ARN was public, as the Role trusted-entities configuration specifies that only GitHub Actions in this repo can assume the role). The AWS_ROLE_FOR_TESTS
is configured as a repository secret here:
https://github.com/guardian/facia-scala-client/settings/secrets/actions
The value is the ARN generated by the CDK-generated cloudformation as an output, you can view it in the AWS Cloudformation Console.
githubOrganisation: "guardian", | ||
repositories: "facia-scala-client:*" |
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.
This configuration produces the repo:guardian/facia-scala-client:*
value we want:
- Action: sts:AssumeRoleWithWebIdentity
Condition:
StringLike:
token.actions.githubusercontent.com:sub: repo:guardian/facia-scala-client:*
It's a bit unexpected though, GuCDK should probably be updated to allow you to just say repositories: "facia-scala-client"
ae94153
to
f367a56
Compare
cdk/package.json
Outdated
"diff": "cdk diff --path-metadata false --version-reporting false" | ||
}, | ||
"devDependencies": { | ||
"@guardian/cdk": "^45.1.3", |
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.
Note that we need at least GU CDK version 45.1.3, to get the fix from guardian/cdk#1350 - otherwise we'll get an 'Incorrect token audience' error
when running the aws-actions/configure-aws-credentials
GitHub Action.
f72f978
to
6a2497f
Compare
Running the tests for this project requires read access to s3://facia-tool-store/DEV/, so we need to provide the GitHub Action with AWS credentials for a AWS role that allows that. We're using https://github.com/aws-actions/configure-aws-credentials to grant the credentials, and https://github.com/guardian/cdk to create the AWS Role. Co-authored-by: Akash Askoolum <akash.askoolum@guardian.co.uk>
6a2497f
to
6490ae5
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.
This looks good, and I can see the CI runs doing what looks like the right thing in GHA. Great! One change to suggest, to make the Typescript configuration slightly less verbose.
There are a couple of things that crossed my mind when reviewing this code:
- this approach requires a fair amount of scaffolding (there's more code)
- it looks like we must manually deploy the test stack after changes – although, speaking pragmatically, it's unlikely this'll happen too often (there are more moving parts)
Did you consider Localstack as one alternative? It might result in less code and fewer moving parts: there's an example of a config in one of our projects here – sadly this runs in TC at the moment, but examples of using this in GHA look fairly concise (the link's just as an illustration, I haven't given that a try, although looking at the repo the action has appeared to trigger successfully).
(One wrinkle – we'd need to make sure the tests pointed our AWS clients at the appropriate URL, which might be a pain. Another wrinkle – Localstack has a paid-for tier for some services, and although well used, obviously runs the risk of imperfectly replicating AWS services. These objections might be enough to sink it.)
Anyhow, this approach looks fine, and will be a useful template if others use this approach elsewhere (it will definitely be necessary with more complicated services that localstack doesn't support). Interested to hear your opinion on Localstack, as it'll be a useful steer on how we implement integration tests in CI elsewhere.
"declaration": true, | ||
"strict": true, | ||
"noImplicitAny": true, | ||
"strictNullChecks": true, |
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.
It looks like strictNullChecks
defaults to true when strict
is true, and I suspect this will be true for many of the properties that look strictness-related – might be worth going through these to slim down this configuration.
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.
Ah, this comes via https://github.com/guardian/cdk/blob/main/src/bin/commands/new-project/template/tsconfig.json.template. Can we make changes there too 🙏🏽 ?
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.
These look like good improvements! For myself I'm about 3 or 4 layers of indirection deep from a completely different task (trying to get a Scala 2.13 upgrade for Frontend!), so I don't want to try and pursue this right now. It feels like improving the GU CDK templates for clarity is probably a good thing for DevX to pick up in the first instance?
Yep, there is quite a lot of new code in the One alternative is to just to store the cloudformation YAML, which is just 1 file and about 50 lines long (longer than the 32 lines of
Yep, Akash and I did talk about making the cloudformation riff-raff deployable - it would have to be a new artifact of course - and decided in the end, as you say, because the changes would come very infrequently, it's probably ok to make this manually deployable for now.
That's an interesting option, I wasn't aware of Localstack! The imperfection of replicating AWS services is always a tricky one. In some cases, if a test suite is really slow to execute because of AWS speed (eg, it's always really slow to create new DynamoDB tables) I think services like AWS DynamoDB Local, or Localstack, can be a definite preferable choice. Here, the speed isn't really a problem, and I guess the only downside here is the price we're paying in the GU-CDK boilerplate, and the added complexity of a new dedicated cloudformation stack. I think it is a price worth paying for CI - and I guess we could say that the cloudformation stack kind of nicely documents & proves the permissions required to use |
This replaces the old release process which had developers manually running `sbt release` on their own laptops - each developer had to obtain their own PGP key and Sonatype credentials, which was an elaborate & fiddly process. Now there's a single set of release credentials, available through GitHub Organisation Secrets, like we already have with NPM. ### Required changes The changes required to adopt the automated workflow: * No need to set these sbt configuration keys, as they're now taken care of by the workflow: * `sonatypeProfileName` * `publishTo` * `scmInfo` & `pomExtra` * Remove the sign, publish, release & push steps of sbt-release's `releaseProcess` configuration, because the workflow does those now, and the workflow only wants `sbt release` to create the versioning commits, and tag them appropriately. The workflow does the rest itself. * Remove `sbt-pgp` plugin because it's no longer used - the workflow does the signing using `gpg` directly. * Grant this repo access to the GitHub Organisation Secrets containing the Maven Release credentials with guardian/github-secret-access#21 * Unusually, drop running the tests as part of the release for now, as the tests in this project require special credentials (see #272) Additionally, we add **automatic version numbering** based on compatibility assessment performed by `sbt-version-policy`: * Add the `sbt-version-policy` plugin, to allow it to do the compatibility assessment. This also sets the `versionScheme` for this library to `early-semver`, which is the recommended versioning for Scala libraries, and `sbt-version-policy` & correct sbt-eviction-issue-detection pretty much depend on the `versionScheme` being `early-semver`. Thus we also need to switch to using a new semver version number for our library version! * Add the `releaseVersion := fromAggregatedAssessedCompatibilityWithLatestRelease().value` sbt setting, which will intelligently set the release version based on `sbt-version-policy`'s compatibility assessment, thanks to scalacenter/sbt-version-policy#187 . * Use `publish / skip := true`, rather than other hacks like `publish := {}` or `publishArtifact := false`, to tell sbt not to publish modules that we don't want published (typically, the 'root' module) - this is important because `sbt-version-policy` won't understand those hacks, but _will_ understand `publish / skip := true` means that it doesn't need to assess compatibility there. Recent prior example of adding `gha-scala-library-release-workflow` to a repo: guardian/play-googleauth#208
Running the tests for this project requires read access to
s3://facia-tool-store/DEV/
, so we need to provide the GitHub Action with AWS credentials for a AWS role that allows that.We're using https://github.com/aws-actions/configure-aws-credentials to grant the credentials, and https://github.com/guardian/cdk to create the AWS Role - the new cloudformation stack is here.
Specific IAM permissions required
Even though all the FAPI client does, in terms of S3 API calls, is call
getObject
, we need more than thes3:GetObject
permission. We also need
s3:ListBucket
because FAPI sometimes has to request objects that don't exist ...and withouts3:ListBucket
, S3 will throw aAccessDenied
error even tho' you possess thes3:GetObject permission
: https://stackoverflow.com/a/56027548/438886Abusing the repositories field
Note that I seem to be having to abuse the
repositories
field a bit (is this field badly named?) in order to get thisrepo:guardian/facia-scala-client:*
value:...which is apparently the format required:
aws-actions/configure-aws-credentials#306 (comment)
You can see what happens if you leave the
:*
off here:https://github.com/guardian/facia-scala-client/runs/7110152225?check_suite_focus=true#step:3:6
...you get a
Error: Not authorized to perform sts:AssumeRoleWithWebIdentity
from theaws-actions/configure-aws-credentials
GitHub Action.Co-authored-by: @akash1810