Add a test for forcing compression and v2s2 format #24673
Conversation
openshift-ci
bot
added
do-not-merge/work-in-progress
Jamstah
force-pushed
the
allow-forced-gzip-on-v2s2
branch
2 times, most recently
from
ccf7160
to
71c9623
Compare
Jamstah
force-pushed
the
allow-forced-gzip-on-v2s2
branch
from
71c9623
to
d16d868
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Jamstah The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
test/e2e/push_test.go
Outdated
| skopeo.WaitWithDefaultTimeout() | ||
| Expect(skopeo).Should(ExitCleanly()) | ||
| output := skopeo.OutputToString() | ||
| Expect(output).ToNot(ContainSubstring("zstd")) |
There was a problem hiding this comment.
This should check for the proper gzip mime type I would assume? Checking for not zstd does not mean it cannot be any other incorrect value.
There was a problem hiding this comment.
Yes, this was copied from the --force-compression test, where actually its testing that without force it doesn't recompress existing layers.
Fixed to check for the presence of gzip instead.
|
|
||
| push = podmanTest.Podman([]string{"push", "-q", "--tls-verify=false", "--force-compression=true", "--compression-format", "zstd", "--format", "v2s2", "--remove-signatures", "imageone", "localhost:5005/image"}) | ||
| push.WaitWithDefaultTimeout() | ||
| Expect(push).Should(ExitWithError(125, "cannot use ForceCompressionFormat with undefined default compression format")) |
There was a problem hiding this comment.
This error seems extremely unhelpful. Why undefined default compression format? The command clearly specified zstd as format so this makes just no sense.
There was a problem hiding this comment.
Yes, that's just testing that the change to common doesn't mean that other invalid cases will still be attempted.
The fix to common is only to allow it to work in the gzip case, not to fix the misleading error completely.
There was a problem hiding this comment.
Yes, this is all a fallout of the c/common code being a short-term hack (where we didn’t do the long-term correct thing), see containers/common#1869 .
What we want is to differentiate between compression formats coming from containers.conf (which should be silently ignored for destinations which can’t support them, silently falling back to gzip) and compression formats coming from the command line (which should cause an error when the user asks for an impossible operation).
But the podman CLI (here
podman/cmd/podman/images/push.go
Line 140 in ceee7cb
containers.conf values as CLI option defaults, making it, from the very start, hard to tell the difference (although, IIRC, possible). And by the time the data gets to c/common, there is no longer any way to tell, in the current c/common API.
I do think that a comment here, in the test case, to the effect of “the command is asking for an impossible thing; per containers/common#1869, we should detect that and fail with a precise error, but that does not exist, so we end up reporting this misleading text” would be useful.
Jamstah
force-pushed
the
allow-forced-gzip-on-v2s2
branch
from
d16d868
to
6871b47
Compare
This highlights a bug in common where the compression format is reset if the format is v2s2, even if its a valid compression format. Signed-off-by: James Hewitt <[email protected]>
Signed-off-by: James Hewitt <[email protected]>
Jamstah
force-pushed
the
allow-forced-gzip-on-v2s2
branch
from
6871b47
to
5563a32
Compare
|
Cockpit tests failed for commit 6871b47. @martinpitt, @jelly, @mvollmer please check. |
|
Cockpit tests failed for commit 5563a32. @martinpitt, @jelly, @mvollmer please check. |
|
Wrt. failed cockpit rawhide test: I reported the criu/kernel regression to https://bugzilla.redhat.com/show_bug.cgi?id=2328985 and silenced it in our tests in cockpit-project/bots#7146 . From now on, this failure will be ignored, i.e. tests should go back to green. Please either retry or re-push. |
|
|
||
| push = podmanTest.Podman([]string{"push", "-q", "--tls-verify=false", "--force-compression=true", "--compression-format", "zstd", "--format", "v2s2", "--remove-signatures", "imageone", "localhost:5005/image"}) | ||
| push.WaitWithDefaultTimeout() | ||
| Expect(push).Should(ExitWithError(125, "cannot use ForceCompressionFormat with undefined default compression format")) |
There was a problem hiding this comment.
Yes, this is all a fallout of the c/common code being a short-term hack (where we didn’t do the long-term correct thing), see containers/common#1869 .
What we want is to differentiate between compression formats coming from containers.conf (which should be silently ignored for destinations which can’t support them, silently falling back to gzip) and compression formats coming from the command line (which should cause an error when the user asks for an impossible operation).
But the podman CLI (here
podman/cmd/podman/images/push.go
Line 140 in ceee7cb
containers.conf values as CLI option defaults, making it, from the very start, hard to tell the difference (although, IIRC, possible). And by the time the data gets to c/common, there is no longer any way to tell, in the current c/common API.
I do think that a comment here, in the test case, to the effect of “the command is asking for an impossible thing; per containers/common#1869, we should detect that and fail with a precise error, but that does not exist, so we end up reporting this misleading text” would be useful.
|
For the record, this PR exists to test containers/common#2255 . |
openshift-merge-robot
added
the
needs-rebase
|
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-sigs/prow repository. |
This highlights a bug in common where the compression format is reset if the format is v2s2, even if its a valid compression format.
Does this PR introduce a user-facing change?