Update cache tags to prevent collisions#360
Conversation
Test Results1 113 tests 1 113 ✅ 13m 27s ⏱️ Results for commit 257bb53. |
There was a problem hiding this comment.
Pull request overview
This pull request updates cache tag naming to prevent collisions between single-platform Docker builds by adding platform-specific suffixes. The change addresses a critical issue where single-platform builds (e.g., linux/amd64 only) would share the same cache as multi-platform builds, potentially causing build failures or incorrect cached layers.
Changes:
- Modified cache naming to include platform suffix (e.g.,
-amd64) for single-platform builds while omitting it for multi-platform builds - Changed version regex pattern to preserve pre-release identifiers while still stripping build metadata
- Converted
cache_namefrom a property to a method accepting an optional platform parameter
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| posit_bakery/image/image_target.py | Core logic: converted cache_name to method, added platform suffix logic, updated regex to preserve pre-release versions |
| posit_bakery/image/bake/bake.py | Updated bake target generation to compute and pass platform parameter to cache_name |
| posit_bakery/config/config.py | Updated build_targets to pass platforms to bake plan, fixed clean_caches to call cache_name as method |
| test/image/test_image_target.py | Updated test to call cache_name as method with platform parameter |
| test/image/bake/testdata/*.json | Updated expected test data to reflect new platform-specific cache tags |
| README.md | Removed GitHub authentication requirements (repository is now public) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ianpittwood
left a comment
ianpittwood
left a comment
There was a problem hiding this comment.
I think this all makes sense. It's confusing how we handle platforms at the moment, but I'm also not sure how it can be improved yet.
| :param dry_run: If True, print what would be deleted without actually deleting anything. | ||
| """ | ||
| target_caches = list(set([target.cache_name.split(":")[0] for target in self.targets])) | ||
| target_caches = list(set([cn.split(":")[0] for target in self.targets if (cn := target.cache_name())])) |
There was a problem hiding this comment.
This may be the first time I've ever seen the walrus operator used in the real world 😆, clever way of doing it!
| cache_platform = effective_platforms[0] if len(effective_platforms) == 1 else None | ||
| cache_name = image_target.cache_name(platform=cache_platform) | ||
| if cache_name is not None: | ||
| kwargs["cache_from"] = [{"type": "registry", "ref": cache_name}] | ||
| kwargs["cache_to"] = [{"type": "registry", "ref": cache_name, "mode": "max"}] |
There was a problem hiding this comment.
So in theory, bake should handle shared caching correctly for multiplatform builds?
There was a problem hiding this comment.
That is correct. When we use bake, the multi-platform builds are handled natively.
2 participants
Related PRs testing this change: