Skip to content
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

Consider running full Test Suite before push to Docker #681

Open
lukehesluke opened this issue Apr 2, 2024 · 0 comments
Open

Consider running full Test Suite before push to Docker #681

lukehesluke opened this issue Apr 2, 2024 · 0 comments

Comments

@lukehesluke
Copy link
Contributor

lukehesluke commented Apr 2, 2024

Spawned from this comment: #597 (comment)

IMPACT: Without this, Test Suite has some (low-ish) chance of publishing Docker images which are broken. These Docker images are ideally used by Booking System implementations' CI pipelines to test their implementations, which means that this could break several CI pipelines simultaneously.


Currently, here's how things work:

  1. For each new PR that has master as its base branch, the .github/workflows/reference-implementation.yml workflow runs, which runs Test Suite against the Reference Implementation with a matrix of different configuration (e.g. auth, no auth, single seller, multiple sellers, etc). This is an extremely thorough test
  2. When the PR is merged into master, the .github/workflows/docker.yml workflow runs, which, after just a very basic set of code testing, pushes the code to a new Docker image. Test Suite is not run here with its matrix of different configurations
  3. This GitHub project is set up with branch protection rules to ensure that a PR can only be merged into master if it passes CI AND is already up-to-date with master (and is approved)

Because of point 3, it can be assumed that point 2 already takes on the thoroughly-tested attributes of point 1, as any changes to master must come in through a PR which must have passed testing.

However, it is possible (and may at some unfortunate time be necessary) to bypass branch rules.

And so, it would still be useful to add all that testing into the docker workflow.

The only downside is that it would add to the GitHub test runner load that unfortunately slows down throughput of OpenActive PRs, but I expect this is a reasonable compromise


Solution Design Notes

It might be possible to do this by chaining workflows so that the reference-implementation.yml workflow runs first and then tasks the docker.yml workflow to start if it is successful (the author of this comment is not too familiar with this process but it seems like it should work). This would require:

  1. Updating reference-implementation.yml to also run on pushes to master.
  2. Having an action that runs after everything else, AND ONLY WHEN ON THE master BRANCH i.e. if: ${{ github.ref == 'refs/heads/master' }} and invokes a repository dispatch
  3. Having the docker.yml be triggered only by the above repository dispatch

Some solution design thoughts from the comment that spawned this issue:

This needs to be expanded to test before push (https://docs.docker.com/build/ci/github-actions/test-before-push/), to ensure that everything works as expected.

The reference implementation service (https://github.com/openactive/OpenActive.Server.NET/blob/d6e2f57dd8b6a7835cf8bcd747eef7553a7128f4/.github/workflows/docker-test.yaml#L16-L32) can be added here to facilitate this.

The config can also be passed in (https://github.com/openactive/OpenActive.Server.NET/blob/d6e2f57dd8b6a7835cf8bcd747eef7553a7128f4/.github/workflows/docker-test.yaml#L53-L56)

Though note that we'd need to get ghcr.io/openactive/reference.bookingsystem.identity running to actually test Puppeteer (which requires figuring out how to get it running on HTTP, perhaps by overriding the "urls" property in appsettings.json via an env var as per dotnet/AspNetCore.Docs#25626 (comment))

Also note that the two use the same fake database and there may be assumptions tied to this running on the same machine (as it writes to the same location on local disk), therefore something like this may be more appropriate https://github.com/openactive/OpenActive.Server.NET/blob/d6e2f57dd8b6a7835cf8bcd747eef7553a7128f4/.github/workflows/docker-test-2.yaml#L18-L38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 💡Ideas
Development

No branches or pull requests

1 participant