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

Container workflow overhaul #198

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

jhiemstrawisc
Copy link
Collaborator

This PR overhauls some of the GHA workflows designed to build spras's docker containers. In particular, it defines a new template in .github/workflows/build-and-remove-template.yml that tries to accomplish the following:

  • Only build containers when they've been modified -- no need to build every container if their definitions haven't changed
  • Split out the container build workflow so it can happen if needed in parallel to other tests.
  • Update the docker build/push action to use the @v6 tag.

This commit splits the build + push action into its yaml definition and updates
the build+push action we rely on from v1 to v6. We hadn't updated the action
because there was some previous feature we either relied on, or a breaking
change in a newer release we wanted to avoid. In any case, we can't remember what
the feature was, so it's probably time to give the new stuff a go.

I chose to split this into its own workflow definition because some of the next
improvements I'd like to make involve updating the trigger logic. This could be
done through the same `test-spras.yml`, but is cleaner/easier to read and
understand if done as a standalone.
Previously we did an explicit `docker pull` for each of the containers
we wanted to use as a cache base in the build tests. I don't _think_
this is explicilty necessary, as the v6 docker build+push action has
a default cache-from type of "registry":
https://docs.docker.com/reference/cli/docker/buildx/build/#cache-from

In other words, each build step pulls what it needs to without the
explicit management. Barring a need to pre-load containers for some speed
optimization (I couldn't find any), it seems more prudent to let the
action manage things.
Previously this test built every container every time someone pushed changes
to a PR. While this was okay, it means that we ended up building containers
even when the actual changes to the codebase had nothing to do with those
containers.

This commit attempts to create a separate build+remove templated action that
uses `git diff` against an input path to determine if a container needs to be
built. Workflow in development!
I realized the SPRAS container build wouldn't work with the previous action
because it takes a different context not scoped to the container's Dockerfile
directory. This commit (among a few other small cleanups) adds a `context` input
to the new action so that I can pass the `./` value for the SPRAS container build.
The previous action trigger of `on: [push, pull_request]` looked like it was
double dipping -- every time you pushed a new set of commits to a pull request,
the workflow triggered twice.

This instead runs the actions on new pull requests, or on synchronize/reopened
events. The synchronize event occurs when new commits are received.

Additionally, this still runs the test whenever someone pushes to master, which
shouldn't be done...
@jhiemstrawisc
Copy link
Collaborator Author

Since this PR doesn't make changes to any of the docker-wrapper containers that might trigger individual container builds, I've set up a demonstration here. In particular, the reviewer should be able to see that the job for all pairs detected a change and rebuilt the container. Building steps for all other containers (except SPRAS) have been skipped.

@jhiemstrawisc jhiemstrawisc requested a review from agitter December 9, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant