-
Notifications
You must be signed in to change notification settings - Fork 336
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
Cached deployment of Docker using github actions #2746
base: develop
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the GitHub Actions deployment workflow configuration, introducing a manual trigger mechanism via Changes
Suggested Reviewers
Possibly related PRs
Poem
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: 1
🧹 Nitpick comments (2)
.github/workflows/deploy.yml (2)
3-3
: Remove this delightful little comment.I'm sure we all appreciate the documentation, but "Test change to trigger workflow" seems a bit... unnecessary now that we have the actual changes in place.
-# Test change to trigger workflow
91-91
: Consider adding cleanup steps.While I'm sure we're all enjoying the accumulation of Docker caches in our registry, maybe we could add some housekeeping? Consider adding cleanup steps to remove old cache entries periodically.
Here's a suggested cleanup step to add after the build:
- name: Cleanup old caches if: github.ref == 'refs/heads/develop' run: | REPOSITORY="${{ github.repository }}" # Keep only the last 5 buildcache tags gh api -X GET /user/packages/container/care/versions --jq '.[] | select(.metadata.container.tags[] | contains("buildcache"))' | \ jq -r '.id' | sort -r | tail -n +6 | \ xargs -I {} gh api -X DELETE /user/packages/container/care/versions/{} env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/deploy.yml
(3 hunks)
🔇 Additional comments (1)
.github/workflows/deploy.yml (1)
88-91
: Verify cache effectiveness with metrics.The cache configuration looks promising, but it would be absolutely wonderful if we could verify the cache hit rates. Could you share some metrics from recent builds showing the improvement from ~2.5 minutes to 45 seconds?
Let's check the recent workflow runs for build times:
✅ Verification successful
Cache configuration is technically perfect, but...
The cache setup follows all the best practices for optimal performance:
- Registry-based caching (faster than local)
- Multiple cache sources with both
buildcache
andlatest
tags- Maximum layer caching with
mode=max
While I'd love to see those promised build time improvements, the configuration itself couldn't be better. Just keep an eye on the actual build times to ensure they meet expectations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Get build durations from recent workflow runs gh api -X GET /repos/$GITHUB_REPOSITORY/actions/workflows/deploy.yml/runs \ --jq '.workflow_runs[0:5] | .[] | select(.conclusion=="success") | {run_number: .run_number, created_at: .created_at, updated_at: .updated_at}'Length of output: 315
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 (1)
.github/workflows/deploy.yml (1)
88-91
: Perhaps we could be a bit more... selective with our caching?While the registry caching implementation is quite nice (and that 70% speed improvement is impressive), using
mode=max
might be a tad... excessive. It stores absolutely everything, which could lead to larger storage costs and slower push times.Consider using
mode=min
to store only the minimum necessary layers:cache-from: | type=registry,ref=ghcr.io/${{ github.repository }}:buildcache type=registry,ref=ghcr.io/${{ github.repository }}:latest - cache-to: type=registry,ref=ghcr.io/${{ github.repository }}:buildcache,mode=max + cache-to: type=registry,ref=ghcr.io/${{ github.repository }}:buildcache,mode=minBut hey, if storage costs aren't a concern, feel free to ignore this suggestion. It's your cache party! 🎉
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/deploy.yml
(3 hunks)
🔇 Additional comments (2)
.github/workflows/deploy.yml (2)
Line range hint
4-11
: LGTM! Nice touch with the manual trigger.The addition of
workflow_dispatch
while preserving existing automation shows someone's been thinking ahead. How thoughtful.
65-67
: Oh, we're still living dangerously with 'latest', aren't we?Instead of the adventurous
latest
tag, perhaps we could use the stable versionv0.18.2
? Just a thought.with: driver-opts: | - image=moby/buildkit:latest + image=moby/buildkit:v0.18.2
Hi Kshitij, I went through your changes, I don't think this will make a difference because we are missing cache due to some different reason, and there is a limit on how much data we can push to the registry so using it as a cache will exhaust it quickly, instead of this can you try https://docs.docker.com/build/ci/github-actions/multi-platform/#distribute-build-across-multiple-runners and see if this improves the caching behavior |
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/deploy.yml (2)
90-92
: Consider optimizing cache sources.While using both
buildcache
andlatest
tags for cache sources is clever, thelatest
tag might be a bit... unpredictable. Consider using only thebuildcache
tag for more stable and predictable builds.cache-from: | type=registry,ref=ghcr.io/${{ github.repository }}:buildcache
93-93
: Watch out for registry space consumption.Using
mode=max
with registry caching is like trying to fit your entire wardrobe into a carry-on bag - it might work initially, but you'll hit limits soon. Consider:
- Implementing a cache cleanup strategy
- Using
mode=min
to store only explicitly cached layerscache-to: type=registry,ref=ghcr.io/${{ github.repository }}:buildcache,mode=minWould you like me to help create a workflow for periodic cache cleanup?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/deploy.yml
(3 hunks)
🔇 Additional comments (2)
.github/workflows/deploy.yml (2)
4-4
: Nice addition of manual trigger!The workflow_dispatch trigger will be quite handy for those... special moments when we need manual deployments.
67-69
: Oh, we're still living dangerously with 'latest', I see...Using
moby/buildkit:latest
in CI/CD is like playing Russian roulette with our builds. Perhaps we could be a bit more... conservative?with: driver-opts: | image=moby/buildkit:v0.18.2
This PR implements Docker build caching in our GitHub Actions workflow, resulting in significant performance improvements for the CI/CD pipeline.
Proposed Changes
Associated Issue
Merge Checklist
/docs
Performance Improvements
Verification
Added Docker layer caching which can be verified by the
CACHED
flags in the build output. Build logs clearly show the cache being utilized across different stages (marked asCACHED
in the build output), confirming proper cache implementation and hit rates.Only PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Summary by CodeRabbit