Optimize docker build #7898
Optimize docker build #7898
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes Docker builds in CI by implementing parallel builds and skipping builds for unchanged images. It introduces a new architecture that automatically discovers buildable images from Docker Compose files and only builds them if the relevant files have changed, otherwise retagging existing images.
- Parallelized Docker image builds using a matrix strategy
- Added logic to skip builds for unchanged images by checking git history and existing image tags
- Simplified adding new images by using Docker Compose configuration discovery
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docker-compose.yml | Updated API image reference to use configurable registry and repository variables |
| docker-compose.override.yml | Unset image for development to avoid download issues |
| .github/workflows/reusable-e2e-tests-build.yml | Updated API image tag format for consistency |
| .github/workflows/reusable-build-and-push.yml | Complete rewrite to support parallel builds and conditional building based on file changes |
| .docker-hub/docker-compose.yml | New file defining build configurations for frontend, print, and varnish images |
|
@CodeRabbit review this |
|
@manuelmeister I'll review the PR changes for optimizing the Docker build process. ✅ Actions performedReview triggered.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a Docker Hub-oriented docker-compose file for building three images. Overhauls the reusable build-and-push GitHub workflow to a matrix-based, tag-aware pipeline with a discovery step. Updates API image references to parameterized registry/owner/tag in compose and adjusts E2E workflow to use docker.io/ecamp/ecamp3-api:latest. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Reusable Workflow Caller
participant GH as GitHub Actions
participant BI as Job: build-info
participant BP as Job: build-and-push (matrix)
participant Reg as Docker Registry
Caller->>GH: Trigger workflow (push/workflow_dispatch)
GH->>BI: Run discovery
BI-->>GH: Outputs build-config[], repo-owner, tags
loop per image in build-config (matrix)
GH->>BP: Start job with matrix.build-config
BP->>Reg: Check existing tags / latest commit scope
BP->>BP: Compute/expand tags + build-args
BP->>Reg: Build & push image with tags
alt image existed
BP->>Reg: Retag and push derived tags
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🔇 Additional comments (11)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
And simplify adding new image. Now you just have to add a docker compose with an image name and a build section. Also unset the image of api for dev, some people had problems when there was an image defined which cannot be downloaded. Populate the buildArgValues in the build-and-push step because passing secrets between jobs seems to be discouraged by github. https://github.com/orgs/community/discussions/37942
There was a problem hiding this comment.
So as far as I understand, this essentially replaces the explicit workflow yml for each image build with a single matrix build which builds the images. I see how this reduces code duplication (however, this PR includes 265 added lines and 71 deleted lines, so our code actually got larger...)
Assuming this refactoring towards DRY is good for maintainability. But how is this faster than before? Were the image builds sequential steps before? Why couldn't we just convert the workflow steps to workflow jobs?
| "SENTRY_AUTH_TOKEN" : "${{ secrets.SENTRY_AUTH_TOKEN }}", | ||
| "SENTRY_FRONTEND_PROJECT" : "${{ vars.SENTRY_FRONTEND_PROJECT }}", | ||
| "SENTRY_ORG" : "${{ vars.SENTRY_ORG }}", | ||
| "SENTRY_PRINT_PROJECT" : "${{ vars.SENTRY_PRINT_PROJECT }}", | ||
| "SENTRY_RELEASE_NAME" : "${{ inputs.sha }}", |
There was a problem hiding this comment.
No need to change in this PR, but can you explain why the sentry settings are build args instead of environment variables? This way they're baked into the images, right? Why is that beneficial for the sentry settings?
There was a problem hiding this comment.
I was too lazy until now to split sentry release bundle generation and docker build.
| for (const arg in args) { | ||
| if (buildArgValues[arg]) { | ||
| args[arg] = buildArgValues[arg] | ||
| } | ||
| } |
There was a problem hiding this comment.
Why do we have an override mechanism for these? As far as I can tell, the variable values should be populated in the new docker-compose.yml already, no? Why overwrite them again here with the same values?
There was a problem hiding this comment.
It's not really an override, its more filling in the build-args from the secret.
| frontend-image: | ||
| image: ${REGISTRY:-docker.io}/${REPO_OWNER:-ecamp}/ecamp3-frontend:${VERSION:-latest} | ||
| build: | ||
| context: ../ | ||
| dockerfile: .docker-hub/frontend/Dockerfile | ||
| # they have to be registered in GitHub under exactly this name in secrets or vars | ||
| args: | ||
| SENTRY_AUTH_TOKEN: ${SENTRY_AUTH_TOKEN:-} | ||
| SENTRY_ORG: ${SENTRY_ORG:-} | ||
| SENTRY_FRONTEND_PROJECT: ${SENTRY_FRONTEND_PROJECT:-} | ||
| SENTRY_RELEASE_NAME: ${RELEASE_NAME:-} | ||
| print-image: | ||
| image: ${REGISTRY:-docker.io}/${REPO_OWNER:-ecamp}/ecamp3-print:${VERSION:-latest} | ||
| build: | ||
| context: ../ | ||
| dockerfile: .docker-hub/print/Dockerfile | ||
| # they have to be registered in GitHub under exactly this name in secrets or vars | ||
| args: | ||
| SENTRY_AUTH_TOKEN: ${SENTRY_AUTH_TOKEN:-} | ||
| SENTRY_ORG: ${SENTRY_ORG:-} | ||
| SENTRY_PRINT_PROJECT: ${SENTRY_PRINT_PROJECT:-} | ||
| SENTRY_RELEASE_NAME: ${RELEASE_NAME:-} | ||
| varnish-image: | ||
| image: ${REGISTRY:-docker.io}/${REPO_OWNER:-ecamp}/ecamp3-varnish:${VERSION:-latest} | ||
| build: | ||
| context: ../ | ||
| dockerfile: .docker-hub/varnish/Dockerfile |
There was a problem hiding this comment.
What happened to the api and the db-backup-restore images which we were building before?
BacLuc
force-pushed
the
optimize-docker-build
branch
from
167b2f3 to
96c3a34
Compare
|
Here is my summary of what this PR does, for later reference. Maybe we could improve the documentation about this in the Wiki, maybe even with some screenshots or diagrams, so this isn't such a big black box if you haven't read the code. Previously, we had a GitHub Workflow which had explicit steps for each image which we build (frontend, api, print, varnish, etc.). Each of these build steps would call the docker/build-push-action with different parameters suited to the image to be built. This could be viewed as a kind of code duplication (the parameter values were different for each image, but the same docker/build-and-push-action was mentioned multiple times in the workflow file). This PR introduces a matrix build, meaning it collects the parameters for building all the images in a JSON array, and then runs a parallel GitHub Actions Job for building each image. So most important of all, the builds are parallelized now. So in short, the benefits are parallelization, potentially better skipping of image builds and the declarative approach "any image specified in any docker-compose.yml file in the repo will be built in this single centralized build action". I approved the PR because @BacLuc is the main driver of our deployment pipeline, and his work getting easier is important. But I also expressed reluctance to accept more abstraction complexity in this area in the future. I think it is very important that multiple members of the core team carry the knowledge of how exactly our deployment works. Future improvements which we discussed:
|
| steps: | ||
| - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5 | ||
| with: | ||
| ref: ${{ inputs.sha }} | ||
| fetch-depth: 100 |
CI: parallelize docker build
And simplify adding new image.
Now you just have to add a docker compose with an image name and a build section.
Also unset the image of api for dev, some people had problems when
there was an image defined which cannot be downloaded.
Populate the buildArgValues in the build-and-push step
because passing secrets between jobs seems to be discouraged by github.
https://github.com/orgs/community/discussions/37942
CI: only build and push images if they changed
Else just retag and push the existing image.
Deployment can be seen here: