-
Notifications
You must be signed in to change notification settings - Fork 781
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
manifest, push: implement --add-compression
to push with compressed variants.
#4912
Conversation
56bda6b
to
d9e34fa
Compare
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.
(Just a quick skim to see how the c/image feature is planned to be used.)
- Eventually this should include man pages.
- So a simple
buildah bud && buildah push
will not, with this PR, be able to do this. That’s fine, this PR is valuable as is — it just seems like an enhancement worth tracking.
vendor/github.com/containers/common/libimage/manifests/manifests.go
Outdated
Show resolved
Hide resolved
vendor/github.com/containers/common/libimage/manifests/manifests.go
Outdated
Show resolved
Hide resolved
@mtrmac I think a simple |
That creates a zstd-only image, not a dual image compatible with gzip consumers but allowing use of zstd. |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@flouthoc needs rebase |
@rhatdan Will rebase after this: containers/image#1987 |
--replicate
with compression while pushing--add-compression
with compression while pushing
--add-compression
with compression while pushing--add-compression
to push with compressed variants.
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.
Code LGTM.
@vrothberg @giuseppe PTAL |
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.
Code and test LGTM
@edsantiago can you take a look at the test?
|
||
**--tls-verify** *bool-value* | ||
|
||
Require HTTPS and verification of certificates when talking to container registries (defaults to true). TLS verification cannot be used when talking to an insecure registry. |
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.
Nit: the new flags are not tested in the commit adding them but the one after.
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.
I was not planning to add these flags but they were needed for the --add-compression
test, I can add independent tests for these but they will be just clone of --add-compression
test without the --add-compression
flag, I decided not to add because of duplication since these two flags are already getting tested in the test added for --add-compression
. But I could be wrong 😅
docs/buildah-manifest-push.1.md
Outdated
@@ -18,6 +18,11 @@ The list image's ID and the digest of the image's manifest. | |||
|
|||
## OPTIONS | |||
|
|||
**--add-compression** *compression* | |||
|
|||
If specified while pushing option makes sure that requested compression variant exists for each platform in the manifest list. |
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.
This sentence is a bit hard to brain parse. I'd drop while pushing
since we're already in the buildah-push
man page.
We should also elaborate how in interacts with --compression
and that the ones specified here are in addition to the default or --compression one.
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.
Could you check if it looks any better now.
c7b2e11
to
46ac671
Compare
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.
I'm going to assume that "testing for gzip" == "null annotation". I think that should be clearer in the comments.
There's a lot of unnecessary duplication in the test, it could maybe be refactored into a table. And, when possible, it's better to do exact matches instead of substring.
This is just a quick glance, sorry, gotta head out.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, flouthoc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@containers/buildah-maintainers PTAL |
cmd/buildah/manifest.go
Outdated
@@ -696,6 +702,9 @@ func manifestAnnotateCmd(c *cobra.Command, args []string, opts manifestAnnotateO | |||
} | |||
|
|||
func manifestInspectCmd(c *cobra.Command, args []string, opts manifestInspectOpts) error { | |||
if err := auth.CheckAuthFile(opts.authfile); err != nil { |
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.
Can you wrap this in a check to see if the user modified it?
LGTM other the check to make sure user set the authfile. |
Add flags for `manifest inspect` i.e `--tls-verify` and `--authfile` Signed-off-by: Aditya R <[email protected]>
Adds support for `--add-compression` which accepts multiple compression formats and when used it will add all instances in a manifest list with requested compression formats. Signed-off-by: Aditya R <[email protected]>
/lgtm |
/hold cancel |
@lsm5 Some |
@flouthoc yup, there was a second issue on c9s. I've filed bz and I have a temp fix for podman. I'll create one for buildah too. |
If I want to push |
Not as of now but I think we can create a field in
I think |
Allows users to set default value of `AddCompression` to Engine table so users can use containers/buildah#4912 by default. Closes: containers/buildah#4912 (comment) Signed-off-by: Aditya R <[email protected]>
No. In Fedora 39, the default for pushing to registries has to be "gzip,zstd:chunked" Then Podman can take advantage of zstd. And old docker will continue to work. |
I mean, we are not at that point at all yet. This is A simple |
Ok as long as I get my eventual goal, I am fine with this. |
Allows users to set default value of `AddCompression` to Engine table so users can use containers/buildah#4912 by default. Closes: containers/buildah#4912 (comment) Signed-off-by: Aditya R <[email protected]>
Allows users to set default value of `AddCompression` to Engine table so users can use containers/buildah#4912 by default. Closes: containers/buildah#4912 (comment) Signed-off-by: Aditya R <[email protected]>
This is a feature PR for
c/image
'sEnsureCompressionVariantExists
feature merged in below PR:TryReusingBlobWithOptions
considerRequiredCompression
if set image#2023instanceCopyClone
forzstd
compression image#1987ListUpdate
addimgspecv1.Platform
field image#2029#### Proof of concept to demonstrate the new API which are recently added inc/image
and some which still needs to be merged.Adds a new flag
--add-compression <compression>
tomanifest push
, which when added will push existing manifest on host while replicating the instances with requiredcompression
.Example usage
Expected pushed manifest list
What type of PR is this?
What this PR does / why we need it:
How to verify it
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?