Use Zstd compression for published images to reduce their size and speedup decompression#220
Use Zstd compression for published images to reduce their size and speedup decompression#220slonopotamus wants to merge 1 commit intohassio-addons:mainfrom
Conversation
|
""" WalkthroughThe GitHub Actions workflows for Docker image deployment have been updated. The build and push steps are now consolidated into a single step using the Changes
Estimated code review effort1 (<30 minutes) Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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. 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 (
|
|
Hmm... CI complains that my PR doesn't have labels. But I do not have permissions to add labels =/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/addon-deploy.yaml (1)
140-146: Sameoutputs/loadissue as in base workflowReplicate the fix here to ensure Zstd compression really applies and to avoid redundant local image loading.
- load: true - push: true - outputs: type=image,compression=zstd + push: true + outputs: type=registry,compression=zstdWithout this adjustment the published add-on images will still be gzip-compressed and the job will spend time loading an image it no longer needs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/addon-deploy.yaml(1 hunks).github/workflows/base-deploy.yaml(1 hunks)
.github/workflows/base-deploy.yaml
Outdated
| - name: 🚀 Build & Push | ||
| uses: docker/build-push-action@v6.18.0 | ||
| with: | ||
| load: true | ||
| push: true | ||
| outputs: type=image,compression=zstd | ||
| # yamllint disable rule:line-length |
There was a problem hiding this comment.
🛠️ Refactor suggestion
outputs should use type=registry; current value silently disables Zstd compression
compression=zstd is only honoured when the output type is registry (or oci); with type=image Buildx falls back to an OCI layout on disk and the pushed layers remain gzip-compressed.
Additionally, load: true forces an extra, unneeded local load which increases runner I/O time and cache size now that the explicit docker push step is gone.
- load: true
- push: true
- outputs: type=image,compression=zstd
+ push: true
+ outputs: type=registry,compression=zstdThis keeps the job fast, avoids the extra local artefact, and actually produces Zstd-compressed layers on GHCR.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: 🚀 Build & Push | |
| uses: docker/build-push-action@v6.18.0 | |
| with: | |
| load: true | |
| push: true | |
| outputs: type=image,compression=zstd | |
| # yamllint disable rule:line-length | |
| - name: 🚀 Build & Push | |
| uses: docker/build-push-action@v6.18.0 | |
| with: | |
| push: true | |
| outputs: type=registry,compression=zstd | |
| # yamllint disable rule:line-length |
🤖 Prompt for AI Agents
In .github/workflows/base-deploy.yaml around lines 105 to 111, the docker
build-push-action uses outputs with type=image and compression=zstd, but
compression=zstd only works with type=registry or oci. Change outputs to use
type=registry to enable Zstd compression. Also, remove load: true to avoid
unnecessary local loading and reduce runner I/O and cache size since the push
step is already handled.
slonopotamus
force-pushed
the
zstd
branch
3 times, most recently
from
6f6471e to
0a5c7fb
Compare
.github/workflows/addon-deploy.yaml
Outdated
| with: | ||
| load: true | ||
| outputs: | ||
| - type=image,oci-mediatypes=true,compression=zstd,push=true |
There was a problem hiding this comment.
I don't want to combine pushing.
The reason for this is caching. If a push fails, we still have the cache, hence it is a separate step right after cleanup.
Please revert that (also unrelated to this PR) change.
../Frenck
There was a problem hiding this comment.
Okay. I'll need to think how to avoid copy-pasting all args passed to build-push-action between build and push steps...
There was a problem hiding this comment.
It seems I can't because GitHub Actions does not support YAML anchors: actions/runner#1182
@frenck Do you want me to copy-paste the whole thing so the only difference will be load: for build step vs outputs: for push step?
There was a problem hiding this comment.
Do you want me to copy-paste the whole thing so the only difference will be load: for build step vs outputs: for push step?
Updated PR with these changes...
slonopotamus
force-pushed
the
zstd
branch
2 times, most recently
from
1a27f41 to
2471b3a
Compare
…eedup decompression
| # yamllint disable rule:line-length | ||
| run: | | ||
| docker push \ | ||
| "ghcr.io/${{ github.repository_owner }}/${{ needs.information.outputs.slug }}/${{ matrix.architecture }}:${{ needs.information.outputs.environment }}" | ||
| docker push \ | ||
| "ghcr.io/${{ github.repository_owner }}/${{ needs.information.outputs.slug }}/${{ matrix.architecture }}:${{ needs.information.outputs.version }}" |
There was a problem hiding this comment.
I'm not following why this part needs to go?
There was a problem hiding this comment.
Because push is done through outputs: parameter to docker/build-push-action step.
| uses: docker/build-push-action@v6.18.0 | ||
| with: | ||
| outputs: type=image,oci-mediatypes=true,compression=zstd,push=true | ||
| # yamllint disable rule:line-length | ||
| tags: | | ||
| ghcr.io/${{ github.repository_owner }}/${{ needs.information.outputs.slug }}/${{ matrix.architecture }}:${{ needs.information.outputs.environment }} | ||
| ghcr.io/${{ github.repository_owner }}/${{ needs.information.outputs.slug }}/${{ matrix.architecture }}:${{ needs.information.outputs.version }} | ||
| # yamllint enable rule:line-length | ||
| context: ${{ needs.information.outputs.target }} |
There was a problem hiding this comment.
We do not need to build it again, this step only pushes out the tags.
There was a problem hiding this comment.
Yes, but I need to invoke docker/build-push-action with outputs: flag. So I need to pass it list of tags, build context and etc. Otherwise, it will build and push something different.
There was a problem hiding this comment.
Maybe you want to revise your decision about splitting build and push? That would remove lots of copy-pasting. Is it really a common case when build succeeds but pushing fails and you still want to save the cache?
There was a problem hiding this comment.
Yes, but I need to invoke docker/build-push-action with outputs: flag.
We can use docker directly.
Maybe you want to revise your decision about splitting build and push?
Nope. I explained the reasoning earlier. I think think being able to really on cache is better than the speed bump added in this PR.
It is a nice improvement, but not something I think is important enough to drop other things for. In the end, if this really was the best and critical option, it would have been the default in Docker.
There was a problem hiding this comment.
We can use docker directly.
docker push doesn't allow configuring compression method, that's why I have to go the route I went.
There was a problem hiding this comment.
In that case, I am going to friendly decline this contribution. Honestly, the juice is not worth the squeeze imho.
Especially, since it doesn’t solve an actual issue we are experiencing.
Nevertheless, thanks for being willing to contribute.
../Frenck
2 participants
Proposed Changes
Zstd is a faster and more efficient compression algorithm than Gzip. This PR enables Zstd for all images created using
hassio-addons/workflows. Docker supports Zstd images for multiple years already.Related Issues
Summary by CodeRabbit