Conversation
bschwedler
commented
Dec 22, 2025
bschwedler
commented
- Append platform to the tag for the image layer cache
- Only push the cache when pushing the image
bschwedler
changed the title
ianpittwood
left a comment
ianpittwood
left a comment
There was a problem hiding this comment.
Generally, this lgtm, but I would like to see the parameter I pointed out modified prior to merge. This may also be something worthy of a test.
Test Results862 tests 862 ✅ 11m 54s ⏱️ Results for commit 2ca7687. ♻️ This comment has been updated with latest results. |
bschwedler
force-pushed
the
cache-tag-platform
branch
from
3c06f3e to
c81d0f5
Compare
bschwedler
force-pushed
the
cache-tag-platform
branch
from
0810170 to
9af3836
Compare
bschwedler
force-pushed
the
cache-tag-platform
branch
from
cd33b78 to
2b28758
Compare
| --image-version ${{ matrix.img.version }} \ | ||
| --dev-versions ${{ inputs.dev-versions }} \ | ||
| --cache-registry "ghcr.io/${{ github.repository_owner }}" \ | ||
| ${{ inputs.push && '--push-cache' || '' }} \ |
There was a problem hiding this comment.
I implemented this as part of the initial build step.
We could easily move the --push-cache flag to after the test step or even during the push step.
There was a problem hiding this comment.
I think it should be fine where it is. Test failures are something I would consider being independent of the validity of the build cache. The cache, at least as I understand, should not push on a failed build anyways.
|
There is currently an issue consuming these changes. It is not appending the architecture to the tag: https://github.com/posit-dev/images-package-manager/actions/runs/20607424694/job/59185907098 |
bschwedler
force-pushed
the
cache-tag-platform
branch
from
2b28758 to
c828354
Compare
bschwedler
force-pushed
the
cache-tag-platform
branch
from
c828354 to
2ca7687
Compare
| kwargs["cache_to"] = [{"type": "registry", "ref": image_target.cache_name, "mode": "max"}] | ||
| cache_name = image_target.cache_name | ||
| # Append platform suffix to cache name | ||
| platform_suffix = "-".join(p.removeprefix("linux/").replace("/", "-") for p in platforms) |
There was a problem hiding this comment.
This can generate a suffix like -amd64-arm64 for a multi-platform image build for bake.
I don't know how much sense this makes.
There was a problem hiding this comment.
Hmm... I don't think this will cover us properly. image_target.image_os.platforms will get all supported platforms for the OS, not necessarily the same ones we're targeting with a build. Currently, the targeted platforms are only passed in BakePlan.build. This will require some finagling.
One possible method I can think of is to move setting the cache_to/cache_from from the BakePlan.build method. In BakeTarget, we could move cache configuration to a method called configure_cache. It could take platforms as an argument along with other stuff like push and perform this same construction and concatenation of the platform_suffix to cache_name with the appropriate platforms given to BakePlan.build. From the method, we could set the cache_to and cache_from to ensure they are properly constructed. Does that make sense as a flow? The logic seems otherwise correct, this just helps us ensure that the native builder caches remain separated while also allowing for emulated builders to utilize the cache still.
| --context ${{ inputs.context }} | ||
| - name: Test | ||
| run: | | ||
| PLATFORM=${BUILD_PLATFORM#linux/} \ |
There was a problem hiding this comment.
I am unsure what this variable is used for.
There was a problem hiding this comment.
Seems like it was something I tried before steps.normalize-platform.outputs.platform and it wasn't removed.
|
Successful run & re-run with updated branch: https://github.com/posit-dev/images-package-manager/actions/runs/20607933811?pr=35 |
| --context ${{ inputs.context }} | ||
| - name: Test | ||
| run: | | ||
| PLATFORM=${BUILD_PLATFORM#linux/} \ |
There was a problem hiding this comment.
Seems like it was something I tried before steps.normalize-platform.outputs.platform and it wasn't removed.
| --image-version ${{ matrix.img.version }} \ | ||
| --dev-versions ${{ inputs.dev-versions }} \ | ||
| --cache-registry "ghcr.io/${{ github.repository_owner }}" \ | ||
| ${{ inputs.push && '--push-cache' || '' }} \ |
There was a problem hiding this comment.
I think it should be fine where it is. Test failures are something I would consider being independent of the validity of the build cache. The cache, at least as I understand, should not push on a failed build anyways.
| kwargs["cache_to"] = [{"type": "registry", "ref": image_target.cache_name, "mode": "max"}] | ||
| cache_name = image_target.cache_name | ||
| # Append platform suffix to cache name | ||
| platform_suffix = "-".join(p.removeprefix("linux/").replace("/", "-") for p in platforms) |
There was a problem hiding this comment.
Hmm... I don't think this will cover us properly. image_target.image_os.platforms will get all supported platforms for the OS, not necessarily the same ones we're targeting with a build. Currently, the targeted platforms are only passed in BakePlan.build. This will require some finagling.
One possible method I can think of is to move setting the cache_to/cache_from from the BakePlan.build method. In BakeTarget, we could move cache configuration to a method called configure_cache. It could take platforms as an argument along with other stuff like push and perform this same construction and concatenation of the platform_suffix to cache_name with the appropriate platforms given to BakePlan.build. From the method, we could set the cache_to and cache_from to ensure they are properly constructed. Does that make sense as a flow? The logic seems otherwise correct, this just helps us ensure that the native builder caches remain separated while also allowing for emulated builders to utilize the cache still.
|
Superseded by #360 |
2 participants
