update c/{common,image,storage,buildah} to latest #18999
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 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 |
openshift-ci
bot
added
the
approved
| @@ -403,7 +403,6 @@ var _ = Describe("Podman images", func() { | |||
| Expect(output).To(ContainSubstring("Copying blob ")) | |||
| Expect(output).To(ContainSubstring("Copying config ")) | |||
| Expect(output).To(ContainSubstring("Writing manifest to image destination")) | |||
| Expect(output).To(ContainSubstring("Storing signatures")) | |||
There was a problem hiding this comment.
I don't know the reasoning behind this change, nor how important it is to preserve it for the future, but would it make sense to change at least some of these tests to NotTo(Contain() (<---- deliberate shortcut, not exact code) instead of removing the tests?
There was a problem hiding this comment.
I can do that if checking that makes sense, @mtrmac WDYT?
There was a problem hiding this comment.
Makes sense, that we should only see that line, when we are "Storing signatures"
There was a problem hiding this comment.
Is there a easy way for us in podman to make a test that actually stores signatures? Then I can have a test which checks for the string and make the other ones check for not the string.
There was a problem hiding this comment.
but would it make sense to change at least some of these tests to
NotTo(Contain()
I don’t have a strong opinion at all on the value of having tests for exact progress messages. Assuming that’s valuable, I’m tempted to say the tests should look for any unexpected lines, not just specifically the signature one. But that would be a larger change.
In this case (podman push), without --sign* options we really don’t expect “Storing signatures” to appear. (If it did appear on a push without --sign, we would be re-pushing signatures obtained during a pull, and that would almost certainly fail.)
There was a problem hiding this comment.
Is there a easy way for us in podman to make a test that actually stores signatures?
For pushes, a push with --sign*; e.g.
Line 140 in 3a013b6
For pulls, a pull of a signed image (with registries.d set up to read signatures). One instance where that is guaranteed to happen is also that test. (Alternatively, we could pull UBI or something like that, relying on the registry to be up.)
There was a problem hiding this comment.
Note In the interest of time I left this comment open, I don't like checking for something is not unless you have another test checking that it is. Given the release dance I like to get this PR in, if anyone thinks testing this is important I can follow-up with another PR.
|
@edsantiago looks like I must patch the bud tests as well. I assume it is easiest to do that in buildah (since it will break there as well after a c/image update) and then vendor buildah in here again? |
|
Ugh. Yes, it can only be fixed in buildah, but for now you can add a |
Luap99
changed the title
Signed-off-by: Paul Holzinger <pholzing@redhat.com>
After[1] c/image no longer prints "Storing signatures" so we should not check for it. [1] containers/image#2001 Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Signed-off-by: Paul Holzinger <pholzing@redhat.com>
This commit was automatically cherry-picked by buildah-vendor-treadmill v0.3 from the buildah vendor treadmill PR, containers#13808 Changes since 2023-05-01: - skip a new test, it fails in remote - skip encrypted-FROM test, broken by buildah PR 4746 Signed-off-by: Ed Santiago <santiago@redhat.com> Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com> Signed-off-by: Paul Holzinger <pholzing@redhat.com>
|
LGTM |
|
LGTM and CI is green |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
tests: fix "Storing signatures" check
After[1] c/image no longer prints "Storing signatures" so we should
not check for it.
[1] containers/image#2001
Does this PR introduce a user-facing change?