ci: test-build images for PRs

[mati865]

Every time I interact with GitHub Actions they prove to be insaner than I previously though. I used LLMs to make conditional cache-to and still could not come up with anything sane.
My idea is to make sure the images build (and in the future pass tests) on PR, before merging changes to them.

This got complicated a lot with base images being built in a distinct job. For PRs we really don't want to push untrusted stuff, so I followed https://docs.docker.com/build/ci/github-actions/share-image-jobs/ on how to move them between jobs.

[mati865]

Kudos to Docker for making loading local images so hard with buildx...

[davidlattimore]

My approach to testing the images was to trigger the build workflow on my fork of the repo, then update the fork to use the newly built images. So that always an option if this turns out to be too complex.

[mati865]

It was definitely annoying to get there, but I think it's not that bad once it works:

• diff of using docker driver for PR runs: main...mati865:wild:4e0bc11d50ab1d0f96c2fb9b31617354159c1026
• diff of using build-context to keep using docker-container driver: main...mati865:wild:1d8226c0e86f8ae82950246f50257044e1ae2d4c

Diff between the two approaches: 1d8226c...mati865:wild:4e0bc11d50ab1d0f96c2fb9b31617354159c1026

Neither is particularly beautiful, but I cannot tell which is better. I'll leave it up to you, @marxin and @lapla-cogito to decide whether we care enough about testing the images on CI, and if so, which way to go.

There are also other possibilities like running a local registry, or creating dedicated workflow only for Docker images testing, but having spent hours on "15 minutes job" I'm not too keen on exploring them myself.

[marxin]

Don't have a strong opinion about it - based on the current experience, using the 2 step approach works quite fine for me.
So leaving that up to you if the complexity overweight the benefits.

[davidlattimore]

Thanks for working on this! I agree that more needs to be done to make updating images easier. My original intention was to see how things went with our first image update and refine the process from there. We probably at least need some docs about how to do an update.

One possible concern is that although this verifies that the images build with a changed configuration, it doesn't verify that CI would pass with those new images. Testing that automatically seems impractical - or at least I have no idea how to do it. So if someone is making a substantial change to the images and they want to make sure that change is thoroughly tested, they likely need to do that testing in their own fork of the repo first anyway or at least test it as best they can on their local machine.

Another slight issue is that any change to any of the docker images will trigger all of them to be rebuilt, even if we just fixed a typo in a comment. This could be helped by finding ways to ensure that the resulting images have a short retention period, so that we're at least not keeping them around.

Anyway, I'm happy for this to go in if it's wanted and others find it useful - at least once retention periods are looked at.

[mati865]

One possible concern is that although this verifies that the images build with a changed configuration, it doesn't verify that CI would pass with those new images. Testing that automatically seems impractical - or at least I have no idea how to do it. So if someone is making a substantial change to the images and they want to make sure that change is thoroughly tested, they likely need to do that testing in their own fork of the repo first anyway or at least test it as best they can on their local machine.

I'm not sure if we can preform through testing without duplicating other pipelines. Or at least comparing them to conditionally depend on this one.
We could easily do basic tests, though. I didn't add them here to avoid failing Arch Linux build.

Another slight issue is that any change to any of the docker images will trigger all of them to be rebuilt, even if we just fixed a typo in a comment.

I thought the changes in this area would be rare enough not to care about it.

This could be helped by finding ways to ensure that the resulting images have a short retention period, so that we're at least not keeping them around.

Do you mean retention for the archives for the base images that are shared between steps? Note that, I disabled pushing of the images for pull requests, although for security concerns. So, they are not kept.

[davidlattimore]

This could be helped by finding ways to ensure that the resulting images have a short retention period, so that we're at least not keeping them around.

Do you mean retention for the archives for the base images that are shared between steps? Note that, I disabled pushing of the images for pull requests, although for security concerns. So, they are not kept.

Won't Upload base image archive get run and cause the base image to be kept around for whatever the default retention period is? https://github.com/actions/upload-artifact/blob/main/README.md#retention-period

[mati865]

That's true. By default it's 90 days, and looks like we cannot limit it more than to a single day.

[mati865]

My original intention was to see how things went with our first image update and refine the process from there. We probably at least need some docs about how to do an update.

Nothing really changes about updating the images here. They still require manual trigger to be built and pushed, so I'd argue this is just a step towards making it easier, but the process still remains the same.