-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
ci: dockerimage multi cache #6592
ci: dockerimage multi cache #6592
Conversation
WalkthroughThe pull request updates the Docker container build workflow by modifying the tagging and caching configuration. A new tag is introduced that uses the hash of the Dockerfile, enabling versioning based on the content of the Dockerfile. In addition, the caching strategy is modified to include three references: one using the hash-based tag, another using a predefined tag from the previous steps, and a reference to the latest image. No changes have been made to the declarations of exported or public entities. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/build-container.yml (2)
26-33
: Variable Preparation StepThe “Prepare variables” step properly extracts the branch name and repository name while converting them to lower case. For improved maintainability, consider adding inline comments to explain the purpose of these variables (e.g., how the branch name is used as a tag) for future readers.
54-57
: Multi-Cache Strategy ImplementationSwitching from a single cache reference to multiple
cache-from
entries significantly enhances the caching strategy. Leveraging the Dockerfile hash, branch tag, and thelatest
image should help maximize cache hit rates. Confirm that the ordering does not introduce conflicts and aligns with the intended build behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-container.yml
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: win64-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (2)
.github/workflows/build-container.yml (2)
1-9
: Review of Workflow Metadata and OutputsThe workflow’s metadata and the outputs in the “on” section are configured correctly. The output
path
is dynamically constructed using the job outputs forrepo
andtag
, which is consistent with the intended use. Make sure that the computed values are as expected during execution.
50-53
: Enhanced Docker Image TaggingThe new tag using the Dockerfile hash (
${{ hashFiles('./contrib/containers/ci/Dockerfile') }}
) introduces a robust versioning mechanism based on the Dockerfile’s content. This should effectively reduce cache misses when the Dockerfile is modified. Please verify that the hash expression produces the expected tag format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK c3526dc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK c3526dc
Issue being fixed or feature implemented
docker/build-push-action allows us to cache from multiple refs; let's use it!
I've noticed a lot recently when we are changing docker image we get a lot of cache misses we shouldn't. Now we push an image based on the hash of the dockerfile, and restore from that. Additionally, we can now cache from the branch name.
Previously we'd get into situations where:
develop is pushed, build image: HIT
PR opened that changes image, build image: MISS and push
develop is pushed with a different PR, build image: MISS and push
PR has new commit added or rebased, build image: MISS and push
This PR should allow all of these to hit now.
What was done?
Cache from and push to a tag that is the hash of the dockerfile. Additionally, allow caching from the branch name, or latest if needed.
How Has This Been Tested?
In my own repo, did a situation similiar to as described above, and got cache hits as expected.
Breaking Changes
None
Checklist:
Go over all the following points, and put an
x
in all the boxes that apply.