CI Approach with GitHub Actions

[kevindew]

Trello: https://trello.com/c/QrxjshEm/270-move-ruby-gems-from-jenkins-to-github-actions

Edit: I've now closed the RFC aspect of this PR after it being open for a month.

This PR is a proof-of-concept of a GitHub Actions implementation of CI for this app, which has evolved into the final implementation. Unlike most GOV.UK Ruby Gems, this one has more complex needs CI because of the pact contract tests between this gem (consumer) and applications that provide APIs (provider).

Pact refresher

As pact tests are a hard concept to remember this section serves as a quick refresher - feel free to skip.

GOV.UK uses pact contract tests to assert that changes in one codebase remain compatible with a codebase that depends on it. For GDS API Adapters, we can have confidence that an adapter provides methods that will be operational if we can confirm that the way GDS API Adapters expects an API to behave is how it actually behaves.

The way this is achieved is by pact tests that mock how the API is expected to behave. When these tests run, a JSON file is created that stores all these expectations. To confirm the API behaves as expected we run pact:verify, this will use that JSON file to confirm the application behaves as per GDS API Adapters expectations.

The way this works in CI is:

• When the API (provider) builds it verifies against the pacts of the GDS API Adapters main branch.
• When GDS API Adapters (consumer) is build it generates new pacts and then runs pact:verify on each provider against this new pacts. Should any of these fail the build will fail.

Approach

The CI process is something of a pipeline, with a number of parallel steps. It is illustrated in the diagram below.

graph LR
    A(Build) --> B[Test Ruby 2.7]
    A --> C[Test Ruby 3]
    A --> D[Test Ruby 3.1]
    B --> E{All pass?}
    C --> E
    D --> E
    E --> |No| F(Exit)
    E --> |Yes| G[Publish pacts]
    G --> H1[Verify Account API pact]
    G --> H2[Verify Asset Manager pact]
    G --> H3[Verify Collections pact]
    G --> H4[Verify Email Alert API pact]
    G --> H5[Verify Imminence pact]
    G --> H6[Verify Link Checker API pact]
    G --> H7[Verify Locations API pact]
    G --> H8[Verify Publishing API pact]
    G --> H9[Verify Whitehall pact]
    H1 --> I{All pass?}
    H2 --> I
    H3 --> I
    H4 --> I
    H5 --> I
    H6 --> I
    H7 --> I
    H8 --> I
    H9 --> I
    I --> |No| J(Exit)
    I --> |Yes| K(Publish Gem)

Loading

Testing gems against Ruby versions is already a well trodden path and publishing the pact is a relatively simple operation (running rake pact:publish:branch). The area that is unusual is how we verify the pact against applications.

To do this we can set up a workflow in each application that these will be run against (example). This workflow will be called when GDS API Adapters builds. The workflow will also be configured to run when the application itself builds - so will be used as both part of an applications standard CI and part of GDS API Adapters CI. As the pact:verify will run when the standard CI process occurs, we will remove this from the default test task (using overrideTestTask) so that we don’t test it twice.

And some rationale for the above:

• Why aren’t we keeping everything in GDS API Adapters? The previous Jenkinsfile approach seems to have slowly drifted away from how the apps are configured, with the tests being performed against very different dependencies of the apps and needing some manual hacks. If the configuration belongs to the owning app, and a part of a regular process, it seems more likely this will be kept up-to-date.
• Why run the workflow when the app builds, when it could be part of the regular CI process? If the workflow only runs infrequently it’ll be harder to identify problems with it. If it’s part of a regular build then any breakages will be caught quickly. As a minor aside, it also offers a speed boost since it runs in parallel.
• Why not have just shared workflow all the apps use to run it? We could do, but the gains seem modest as most of the workflow files comprise of app specific dependencies. A shared workflow file would add more complexity and be less flexible - though it would be nice to have less boilerplate.
• Why not run pact:verify as both part of regular CI and an extra workflow file? This redundancy of doing them both would likely give the impression that one of these is a mistake. Avoiding the duplication makes it much more explicit how it should behave.
• Why overrideTestTask when we could remove pact:verify from the default rake task? As pact:verify is part of a build testing this seems like a useful command to have locally so you can still test your app fully with one command. As the tasks that make up testing don't change infrequently in apps, it seems relatively low risk to specify them in the Jenkinsfile

Here is a list of the workflows for apps:

• Account API
• Asset Manager
• Collections
• Email Alert API
• Imminence
• Link Checker API
• Locations API
• Publishing API
• Whitehall

Consequences

For apps with GDS API pact tests:

• There will be a new GitHub Action workflow added
• These apps will have their Jenkinsfile test tasks overriden to not include pact:verify
• A build of one of these will comprise of most testing being done in Jenkins but the pact:verify is done by GitHub actions.
• A failed pact:verify on a main branch won’t prevent an integration deploy (this scenario seems unlikely)
• We should configure govuk_saas_config to require the pact_verify check before merging

For GDS API Adapters:

• It won't be concerned with how apps are configured anymore
• Contributors outside of GOV.UK won’t be able to get a passing build as their lack of access to GitHub secrets will mean the pact branch can’t be published - this seems low risk as there is little incentive for someone outside GOV.UK to contribute to GDS API Adapters
• Builds of other projects will be run even when they're not needed (no changes to pacts) - this hasn't been flagged as a past problem so I don't think it's worth adding to the complexity to check it

[ChrisBAshton]

Looks good! ⭐ nice work putting this together. One major suggestion before I come back and do a more thorough review later.

Contributors outside of GOV.UK won’t be able to get a passing build as their lack of access to GitHub secrets will mean the pact branch can’t be published

I've been having a think and I propose a small change to your approach, which would solve the above issue as well as simplify the overall architecture. Essentially, I think we should remove the Pact Broker and use GitHub itself as the broker.

Changes to the consumer

If we consider the current scenario, when a consumer (e.g. gds-api-adapters) is changed, we:

• Publish all pactfiles (under the git-ignored spec/pacts/*.json) to a new branch on the Pact Broker
• Check out and bundle each app
• Run pact tests inside the app, against the published pactfile

Instead, we would now:

• Commit the generated pactfiles to GitHub (removing them from gitignore)
• Trigger a GitHub action inside each app's repo (as you've proposed)
• ...but passing all the necessary arguments to that action (i.e. consumer name, consumer branch, perhaps name of pactfile)
• The action would fetch the pactfile based on the above args, and run the pact tests against that

Benefits:

• Knowledge of bundling each app would be contained within the app's repo
• The action would be generic enough to work with any consumer
• No pact broker complexity

Changes to provider

Currently, when a provider is changed, we run pact:verify against the master consumer branch on the pact broker.

Instead, we would now need to maintain a hardcoded list of consumers (and potentially pactfile filepaths) in the provider repository. Then, when a change is made to the provider, have CI fetch the pactfiles on that list and run pact:verify against them.

The increased coupling feels like a drawback, though it also acts as documentation (can explicitly see which consumers depend on the provider), giving some semblance of feature parity with the network graph provided by the pact broker. But the main advantage is that we could now retire the pact broker and all its associated infrastructure.

It's a relatively big / controversial change, but feels like only a modest step away from what you're already proposing, so worth considering here. What do you think?

[kevindew]

Thanks for having a look Chris 👍 and thank you for the suggestion.

I somewhat wonder if the idea to get rid of the pact-broker is a tangent from this work rather than directly related, almost like separate questions "can this run on GitHub Actions?" and "Do we need the pact-broker?", so I'm somewhat wondering if pondering that is actually a potential scope creep that could be considered separately?

I'm guessing there is some future pondering needed for the pact broker soon(ish) as it's hosted on the PAAS which is going away.

On the idea itself. I'm imagining one of the pain points of your idea would be that you have increased complications running locally where you both need to clone the repo and risk running out of date pacts. I also think it wouldn't solve the outside contributor issue as I presume we wouldn't allow our repos to clone a fork to test against automatically (though off the top of my head I'm not sure if that'd present a risk).

But yeah, my thought would be: can we defer this concern to be one considered when it comes to Platform Reliability having to think about the future hosting of the Pact Broker?

[kevindew]

@ChrisBAshton I've now opened up the dependent PRs for this in corresponding repos, which will be needed before I can merge this in.

These are:

• Add GitHub Action workflow for verifying pact tests account-api#544
• Add GitHub Action workflow for verifying pact tests asset-manager#1038
• Add GitHub Action workflow for verifying pact tests collections#3055
• Add GitHub Action workflow for verifying pact tests email-alert-api#1845
• Add GitHub Action workflow for verifying pact tests places-manager#834
• Add GitHub Action workflow for verifying pact tests link-checker-api#597
• Add GitHub Action workflow for verifying pact tests locations-api#163
• Add GitHub Action workflow for verifying pact tests publishing-api#2208
• Add GitHub Action workflow for verifying pact tests whitehall#7094

and they're all pretty much the same, just tweaked for app differences.

I notice that the previous CI approach is broken (which is presumably blocking @KludgeKML from #1176) because of a database incompatibility with Whitehall, so there may be a need to sort this sooner rather than later.

[ChrisBAshton]

Code-reviewed - happy with the approach 👍

[ChrisBAshton]

This repo is (recently) retired - can we give this PR an update?

[kevindew]

Thanks, I've sorted that.