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

Build container images only on changes #332

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

p5
Copy link
Contributor

@p5 p5 commented Oct 18, 2024

This PR splits the container image builds into a separate GHA workflow so we only need to run these when the ./container-images/ directory is changed.

We could have done this inside the existing workflow, but it would have either involved some verbose Git commands and/or introducing a new third party dependency (e.g. tj-actions/changed-files), but I feel we benefit more to having minimal dependencies or complexity to help with keeping the repo secure and easy to contribute to.

rhatdan and others added 2 commits October 18, 2024 14:21
@ericcurtin
Copy link
Collaborator

It is important we build the cpuonly container to be used in any of the builds where we run tests or execute ramalama, rather than pull a container...

Because we want to test that Containerfile changes actually work...

@p5
Copy link
Contributor Author

p5 commented Oct 18, 2024

It is important we build the cpuonly container to be used in any of the builds where we run tests or execute ramalama, rather than pull a container...

Because we want to test that Containerfile changes actually work...

This PR is based on Dan's branch, which has split the builds into separate jobs.
Here we move an entire job from one workflow to another, so I feel this should be changed in
#331

@rhatdan
Copy link
Member

rhatdan commented Oct 18, 2024

Can we just build the ramalama cpuonly container and not the Cuda one which takes forever.

@p5
Copy link
Contributor Author

p5 commented Oct 18, 2024

Can we just build the ramalama cpuonly container and not the Cuda one which takes forever.

Yeah, if the bats-docker step only requires the cpuonly build, we can certainly do that

@p5
Copy link
Contributor Author

p5 commented Oct 18, 2024

@rhatdan @ericcurtin
I've pushed a new commit (f62705a) which updates the container_build.sh script to support specifying a single container, plus adds the cpuonly build as a prerequisite to the bats-docker build.

I'm trying to figure out how to test the workflow, so will leave this as a draft for now.

Edit: realised the Makefile needs updating too, so will get that fixed and pushed shortly. I am now remembering how much I don't know about working with Makefiles

@ericcurtin
Copy link
Collaborator

Can we just build the ramalama cpuonly container and not the Cuda one which takes forever.

Yeah, cuda and rocm container builds aren't important unless those Containerfile's are actually changed.

The cpuonly one is because we use that for testing in CI.

@ericcurtin
Copy link
Collaborator

ericcurtin commented Oct 18, 2024

Like one thing that is cool about the renovate setup you did @p5 is that when we attempt a version bump, a PR is automatically opened, which means that version bump can be tested before we merge. It's better than blindly trusting version bumps.

@p5
Copy link
Contributor Author

p5 commented Oct 18, 2024

Yeah, the reasoning behind wanting the builds makes complete sense.

So if I can figure out the correct Makefile syntax, do you think my latest commit would solve the requirement? Changes to any of the Containerfiles or related config files cause all images to be built, but the regular changes will only build the cpuonly Containerfile just before running docker-bats

@p5
Copy link
Contributor Author

p5 commented Oct 18, 2024

We're also able to split each container build into separate jobs so they run in parallel (which could reduce the time waiting for CI by up to 50%), though I'm aware there have been some limitations with the number of concurrent GitHub Actions available within the org.
I'll not implement this for now, but please let me know if you want this.

@ericcurtin
Copy link
Collaborator

What's here so far LGTM 😄

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.

3 participants