manifests,push: add support for `AddCompression` #1585

flouthoc · 2023-07-26T08:33:21Z

c/image recently added support for EnsureCompressionVariantsExist, following PR exposes the feature to c/common manifests so actual users can use it.

Needs: containers/image#1987

flouthoc · 2023-07-26T08:33:43Z

We need vendor of: containers/image#1987 first.

mtrmac

The idea and general approach LGTM
I don’t feel strongly about the “replicate” name… it’s fairly imprecise (we are not replicating anything when asking for zstd and the image already is zstd), OTOH it’s nicely short
There are several reasons why this doesn’t even compile, so it apparently hasn’t been tried at all…
(Vendoring a c/image commit in this PR is fine… now that the c/image PR was merged.)

libimage/manifests/manifests_test.go

flouthoc · 2023-07-26T14:57:22Z

“replicate” name… it’s fairly imprecise

@mtrmac Yes we need a better name since a similar flag is going to be in CLI as well, do have any names in mind ? I have few EnsureCompression, CloneCompression, CopyWithCompression but not sure if they are good.

@giuseppe @vrothberg @rhatdan Any naming ideas other than replicate

vrothberg · 2023-07-26T15:00:44Z

libimage/manifests/manifests.go

@@ -70,6 +71,7 @@ type PushOptions struct {
 	RemoveSignatures                 bool                  // true to discard signatures in images
 	ManifestType                     string                // the format to use when saving the list - possible options are oci, v2s1, and v2s2
 	SourceFilter                     LookupReferenceFunc   // filter the list source
+	ReplicateWithCompression         []string              // replicate required imges with requested compression algorithms


CompressionVariants would be ~ consistent with c/image I think.

do have any names in mind

I can’t think of anything clearly better. Something broadly like “ensure has compression”? But using a simpler and shorter word than “ensure” would be nice.

(BTW until c/image tags a release, we can rename the c/image options if we find a good name.)

c/image has option as EnsureCompressionVariantsExist we can name this as same and later CLI can have --ensure-compression

--ensure-compression sounds more like a validation check to me.

Signed-off-by: Aditya R <arajan@redhat.com>

mtrmac

Code LGTM.

I don’t know whether we can improve on the naming. One very reasonable answer is that c/common explicitly does not have a stable API, so we can change our mind at any time in the future, and it is not worth worrying about much in this repo.

OTOH we do need to settle on a name we are happy with for the CLI options, which are presumably going to come immediately after this PR.

libimage/manifests/manifests.go

flouthoc · 2023-07-27T05:07:32Z

OTOH we do need to settle on a name we are happy with for the CLI options, which are presumably going to come immediately after this PR.

Yes I agree, lets decide it on the CLI PR, where I am going to add this i.e buildah and later skopeo we can decide it there.

@vrothberg @giuseppe @rhatdan PTAL

vrothberg · 2023-07-27T07:08:11Z

Yes I agree, lets decide it on the CLI PR, where I am going to add this i.e buildah and later skopeo we can decide it there.

I think we should decide here, otherwise we're just going to reopen the very same conversation.

--ensure-compression sounds more like a validation check to me. I also do not like ReplicateWithCompression in that context as I want a consistent terminology. It's not nice to have three different terms for the same thing across c/image, c/common and the CLI - although I am OK to embed a more descriptive action in the CLI.

A "replica" to me is a 1:1 copy of an original which is not 100 accurate here I think. I like the term "variant" or "instance".

flouthoc · 2023-07-27T07:15:29Z

--ensure-compression sounds more like a validation check to me

Ah my idea was that flag will ensure that manifest list contains the compression, if it is not there it will create one. How about --compression-variant ?

vrothberg · 2023-07-27T07:36:48Z

--ensure-compression sounds more like a validation check to me

Ah my idea was that flag will ensure that manifest list contains the compression, if it is not there it will create one. How about --compression-variant ?

Hm ... there is already a podman push --compress XXX flag. We could allow specifying it more than once such that a --compress gzip --compress zstd would work?

flouthoc · 2023-07-27T07:46:42Z

--ensure-compression sounds more like a validation check to me

Ah my idea was that flag will ensure that manifest list contains the compression, if it is not there it will create one. How about --compression-variant ?

Hm ... there is already a podman push --compress XXX flag. We could allow specifying it more than once such that a --compress gzip --compress zstd would work?

@vrothberg Technically --compress is a different function, it means replacing existing instance with a new compression, it does not means keeping the old compression and adding new in manifest list ( it works with single instance as well not only manifest list ). Changing behavior of --compress sounds like a breaking change. I'd prefer keeping this different from legacy --compression and adding a new command instead.

flouthoc · 2023-07-27T08:01:11Z

I found a few, does any one of these appeal ?

(--copy-compression, --append-compression , --extend-compression, --add-compression)

vrothberg · 2023-07-27T08:02:51Z

Very fair point, @flouthoc. In that case, I really like --add-compression

flouthoc · 2023-07-27T08:04:15Z

--add-compression is fairly simple and communicates what operation is doing.

@giuseppe @mtrmac @rhatdan WDYT ?

giuseppe · 2023-07-27T08:07:05Z

I like --add-compression

`c/image` recently added support for EnsureCompressionVariantsExist, following PR exposes the feature to `c/common` manifests so actual users can use it. Signed-off-by: Aditya R <arajan@redhat.com>

vrothberg

LGTM

openshift-ci · 2023-07-27T08:26:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, vrothberg

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:

~~OWNERS~~ [vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2023-07-27T09:54:36Z

/lgtm

flouthoc · 2023-07-27T10:12:34Z

@mtrmac Following PR is merged, do we need to change option name in c/image as well ?

mtrmac · 2023-07-27T14:21:17Z

My first instinct is that --add-compression is a good choice for the CLI, a good compromise between the competing requirements (brevity is quite important) but in the API, with IDE autocompletion, using a more precise name is a bit more valuable.

I don’t feel too strongly about that, though.

flouthoc · 2023-07-27T15:02:55Z

Fair enough leaving c/image as-is.

Podman uses different API for pushing manifest list, add `AddCompression` to ManifestPushOptions, which is implemented here: containers#1585 [NO NEW TESTS NEEDED] Tests are added here: containers#1585 Signed-off-by: Aditya R <arajan@redhat.com>

mtrmac reviewed Jul 26, 2023

View reviewed changes

libimage/manifests/manifests_test.go Outdated Show resolved Hide resolved

vrothberg reviewed Jul 26, 2023

View reviewed changes

flouthoc force-pushed the implement-ensure-compression-exsts branch from 53146f9 to fa665ac Compare July 26, 2023 15:28

vendor: bump c/image to v5.26.1-0.20230726142307-8c387a14f4ac

09a8785

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc force-pushed the implement-ensure-compression-exsts branch 2 times, most recently from 1330fd1 to 7816121 Compare July 26, 2023 15:33

mtrmac reviewed Jul 26, 2023

View reviewed changes

libimage/manifests/manifests.go Outdated Show resolved Hide resolved

flouthoc force-pushed the implement-ensure-compression-exsts branch from 7816121 to 93f7f8d Compare July 27, 2023 05:06

manifests,push: add support for AddCompression

44dacb8

`c/image` recently added support for EnsureCompressionVariantsExist, following PR exposes the feature to `c/common` manifests so actual users can use it. Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc force-pushed the implement-ensure-compression-exsts branch from 93f7f8d to 44dacb8 Compare July 27, 2023 08:21

flouthoc changed the title ~~manifests,push: add support for ReplicateWithCompression~~ manifests,push: add support for AddCompression Jul 27, 2023

flouthoc requested review from mtrmac and vrothberg July 27, 2023 08:24

vrothberg approved these changes Jul 27, 2023

View reviewed changes

openshift-ci bot added the approved label Jul 27, 2023

openshift-ci bot assigned rhatdan Jul 27, 2023

openshift-ci bot added the lgtm label Jul 27, 2023

openshift-merge-robot merged commit 647ed1d into containers:main Jul 27, 2023
7 checks passed

flouthoc mentioned this pull request Jul 31, 2023

libimage/ManifestPushOptions: add AddCompression for Manifest.PushOptions #1590

Merged

flouthoc mentioned this pull request Aug 2, 2023

manifest, push: implement --add-compression to push with compressed variants. containers/podman#19468

Merged

flouthoc mentioned this pull request Aug 4, 2023

[release-v0.55] manifests,push: add support for AddCompression #1594

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

manifests,push: add support for `AddCompression` #1585

manifests,push: add support for `AddCompression` #1585

flouthoc commented Jul 26, 2023

flouthoc commented Jul 26, 2023

mtrmac left a comment

flouthoc commented Jul 26, 2023

vrothberg Jul 26, 2023

mtrmac Jul 26, 2023

flouthoc Jul 26, 2023

vrothberg Jul 27, 2023

mtrmac left a comment

flouthoc commented Jul 27, 2023

vrothberg commented Jul 27, 2023

flouthoc commented Jul 27, 2023 •

edited

vrothberg commented Jul 27, 2023

flouthoc commented Jul 27, 2023 •

edited

flouthoc commented Jul 27, 2023

vrothberg commented Jul 27, 2023

flouthoc commented Jul 27, 2023

giuseppe commented Jul 27, 2023

vrothberg left a comment

openshift-ci bot commented Jul 27, 2023

rhatdan commented Jul 27, 2023

flouthoc commented Jul 27, 2023 •

edited

mtrmac commented Jul 27, 2023

flouthoc commented Jul 27, 2023

manifests,push: add support for AddCompression #1585

manifests,push: add support for AddCompression #1585

Conversation

flouthoc commented Jul 26, 2023

flouthoc commented Jul 26, 2023

mtrmac left a comment

Choose a reason for hiding this comment

flouthoc commented Jul 26, 2023

vrothberg Jul 26, 2023

Choose a reason for hiding this comment

mtrmac Jul 26, 2023

Choose a reason for hiding this comment

flouthoc Jul 26, 2023

Choose a reason for hiding this comment

vrothberg Jul 27, 2023

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

flouthoc commented Jul 27, 2023

vrothberg commented Jul 27, 2023

flouthoc commented Jul 27, 2023 • edited

vrothberg commented Jul 27, 2023

flouthoc commented Jul 27, 2023 • edited

flouthoc commented Jul 27, 2023

vrothberg commented Jul 27, 2023

flouthoc commented Jul 27, 2023

giuseppe commented Jul 27, 2023

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 27, 2023

rhatdan commented Jul 27, 2023

flouthoc commented Jul 27, 2023 • edited

mtrmac commented Jul 27, 2023

flouthoc commented Jul 27, 2023

manifests,push: add support for `AddCompression` #1585

manifests,push: add support for `AddCompression` #1585

flouthoc commented Jul 27, 2023 •

edited

flouthoc commented Jul 27, 2023 •

edited

flouthoc commented Jul 27, 2023 •

edited