cmd, push: expose --compression-level #18940

giuseppe · 2023-06-20T09:11:35Z

This patch adds the --compression-level option to the push command.

Does this PR introduce a user-facing change?

Now the push command supports the --compression-level option

vrothberg

LGTM

openshift-ci · 2023-06-20T09:13:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, 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~~ [giuseppe,vrothberg]

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

Luap99 · 2023-06-20T09:15:45Z

test/e2e/manifest_test.go

@@ -435,7 +435,12 @@ var _ = Describe("Podman manifest", func() {
 		Expect(output).To(ContainSubstring("Writing manifest to image destination"))
 		Expect(output).To(ContainSubstring("Storing signatures"))

-		push = podmanTest.Podman([]string{"manifest", "push", "--tls-verify=false", "--creds=podmantest:wrongpasswd", "foo", "localhost:" + registry.Port + "/credstest"})
+		// Invalid compression format specified for gzip, it must fail
+		push = podmanTest.Podman([]string{"push", "--compression-format=gzip", "--compression-level=20", "--tls-verify=false", "--creds=" + registry.User + ":" + registry.Password, "--format=v2s2", "localhost:" + registry.Port + "/alpine:latest"})


this is running push not manifest push
exit code checks are bad, you must check for the proper error message on stderr.

Luap99 · 2023-06-20T09:16:20Z

test/e2e/push_test.go

+		push = podmanTest.Podman([]string{"push", "--compression-format=gzip", "--compression-level=10", "--tls-verify=false", "--remove-signatures", ALPINE, "localhost:5000/my-alpine"})
+		push.WaitWithDefaultTimeout()
+		Expect(push).Should(Exit(125))
+		Expect(push.ErrorToString()).To(BeEmpty())


why should error be empty? please check for the actual expected error message.

flouthoc

Docs check is failing

edsantiago · 2023-06-20T13:55:47Z

test/e2e/manifest_test.go

@@ -434,7 +434,14 @@ var _ = Describe("Podman manifest", func() {
 		Expect(output).To(ContainSubstring("Writing manifest to image destination"))
 		Expect(output).To(ContainSubstring("Storing signatures"))

-		push = podmanTest.Podman([]string{"manifest", "push", "--tls-verify=false", "--creds=podmantest:wrongpasswd", "foo", "localhost:" + registry.Port + "/credstest"})
+		// Invalid compression format specified for gzip, it must fail


Is this comment accurate? It says format, but the error-message check (line 442) says level. Comment also says gzip, which seems inconsistent with zstd. Same applies to lines 102-103 in push_test.go below.

rhatdan · 2023-06-20T20:40:56Z

Should this be configurable in containers.conf? Stored with the compression type?

rhatdan · 2023-06-20T20:41:25Z

docs/source/markdown/options/compression-level.md

+####> are applicable to all of those.
+#### **--compression-level**=*level*
+
+Specifies the compression level to use.  The value is specific to the compression algorithm used, e.g. for zstd the accepted values are in the range 1-20 (inclusive), while for gzip it is 1-9 (inclusive).


What are the defaults?

@giuseppe this one's still one

thanks. I was waiting for the c/common change to merge before pushing the new version.

Should be addressed now

giuseppe · 2023-06-20T21:33:31Z

Should this be configurable in containers.conf? Stored with the compression type?

added compressionlevel to containers.conf here: containers/common#1518

vrothberg

LGTM

vrothberg · 2023-06-21T09:17:57Z

docs/source/markdown/options/compression-level.md

+####> are applicable to all of those.
+#### **--compression-level**=*level*
+
+Specifies the compression level to use.  The value is specific to the compression algorithm used, e.g. for zstd the accepted values are in the range 1-20 (inclusive), while for gzip it is 1-9 (inclusive).


@giuseppe this one's still one

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

This patch adds the --compression-level option to the push command. Closes: containers#18939 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2023-06-21T14:23:37Z

addressed all comment, and the CI is green

rhatdan · 2023-06-21T14:34:35Z

/lgtm

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 20, 2023

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Jun 20, 2023

giuseppe mentioned this pull request Jun 20, 2023

Expose buildah --compression-level on push command #18939

Closed

vrothberg approved these changes Jun 20, 2023

View reviewed changes

Luap99 reviewed Jun 20, 2023

View reviewed changes

giuseppe force-pushed the add-compression-level branch 4 times, most recently from 5879342 to e3685bc Compare June 20, 2023 11:18

flouthoc reviewed Jun 20, 2023

View reviewed changes

giuseppe force-pushed the add-compression-level branch 2 times, most recently from 9d68ccd to 66ce890 Compare June 20, 2023 13:33

edsantiago reviewed Jun 20, 2023

View reviewed changes

giuseppe force-pushed the add-compression-level branch from 66ce890 to 5fff082 Compare June 20, 2023 15:18

rhatdan reviewed Jun 20, 2023

View reviewed changes

giuseppe force-pushed the add-compression-level branch from 5fff082 to 92e4282 Compare June 20, 2023 21:14

giuseppe marked this pull request as draft June 20, 2023 22:22

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 20, 2023

vrothberg reviewed Jun 21, 2023

View reviewed changes

giuseppe force-pushed the add-compression-level branch from 92e4282 to d9ed414 Compare June 21, 2023 11:57

giuseppe marked this pull request as ready for review June 21, 2023 11:57

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 21, 2023

giuseppe force-pushed the add-compression-level branch from d9ed414 to c07883b Compare June 21, 2023 12:07

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2023

giuseppe force-pushed the add-compression-level branch from c07883b to 10a54ed Compare June 21, 2023 12:16

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 21, 2023

giuseppe added 2 commits June 21, 2023 14:28

vendor: bump containers/common

4592483

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

cmd, push: expose --compression-level

bb932cc

This patch adds the --compression-level option to the push command. Closes: containers#18939 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the add-compression-level branch from 10a54ed to bb932cc Compare June 21, 2023 12:28

openshift-ci bot assigned rhatdan Jun 21, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 21, 2023

openshift-merge-robot merged commit 71b0168 into containers:main Jun 21, 2023
87 checks passed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd, push: expose --compression-level #18940

cmd, push: expose --compression-level #18940

giuseppe commented Jun 20, 2023

vrothberg left a comment

openshift-ci bot commented Jun 20, 2023

Luap99 Jun 20, 2023

Luap99 Jun 20, 2023

flouthoc left a comment

edsantiago Jun 20, 2023

rhatdan commented Jun 20, 2023

rhatdan Jun 20, 2023

vrothberg Jun 21, 2023

giuseppe Jun 21, 2023

giuseppe commented Jun 20, 2023

vrothberg left a comment

vrothberg Jun 21, 2023

giuseppe commented Jun 21, 2023

rhatdan commented Jun 21, 2023

cmd, push: expose --compression-level #18940

cmd, push: expose --compression-level #18940

Conversation

giuseppe commented Jun 20, 2023

Does this PR introduce a user-facing change?

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 20, 2023

Luap99 Jun 20, 2023

Choose a reason for hiding this comment

Luap99 Jun 20, 2023

Choose a reason for hiding this comment

flouthoc left a comment

Choose a reason for hiding this comment

edsantiago Jun 20, 2023

Choose a reason for hiding this comment

rhatdan commented Jun 20, 2023

rhatdan Jun 20, 2023

Choose a reason for hiding this comment

vrothberg Jun 21, 2023

Choose a reason for hiding this comment

giuseppe Jun 21, 2023

Choose a reason for hiding this comment

giuseppe commented Jun 20, 2023

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg Jun 21, 2023

Choose a reason for hiding this comment

giuseppe commented Jun 21, 2023

rhatdan commented Jun 21, 2023