Add Continuous Integration (CI) as GitHub Action

[rtyley]

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 the s3:GetObject
permission. We also need s3:ListBucket because FAPI sometimes has to request objects that don't exist ...and without
s3:ListBucket, S3 will throw a AccessDenied error even tho' you possess the s3:GetObject permission: https://stackoverflow.com/a/56027548/438886

Abusing 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 this repo:guardian/facia-scala-client:* value:

          - Action: sts:AssumeRoleWithWebIdentity
            Condition:
              StringLike:
                token.actions.githubusercontent.com:sub: repo:guardian/facia-scala-client:*

...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 the aws-actions/configure-aws-credentials GitHub Action.

Co-authored-by: @akash1810

[rtyley]

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.

[rtyley]

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"

[rtyley]

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.

[jonathonherbert]

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.

[jonathonherbert]

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.

[akash1810]

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 🙏🏽 ?

[rtyley]

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?

[rtyley]

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)

Yep, there is quite a lot of new code in the cdk folder - 8 new files, with only one of them (cdk/lib/facia-scala-client-testing.ts) really containing interesting stuff. It's hard to see how to get rid of the other 7 while still allowing devs to execute that code locally, but I'm not an expert, perhaps cleverer people than me can figure that out?!

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 facia-scala-client-testing.ts tho'). About 6 lines of that YAML is interesting, the rest is AWS boilerplate and a set of obscure magical values like ThumbprintList: 6938fd4d98bab03faadb97b34396831e3780aea1 that GU CDK spares us from... I'm happy enough with the choice of GU-CDK for this, but it would be lovely if all there was to look at was those 6 lines of config!

• 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)

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.

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.)

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 facia-scala-client!