auto-update: validate container image #15933
Conversation
openshift-ci
bot
added
release-note
approved
vrothberg
force-pushed
the
fix-15879
branch
from
43795c5 to
f630d4b
Compare
There was a problem hiding this comment.
Minor nits adding up.
Also... the first half of the test is actually applicable to podman-remote, but it is not run (all tests in this bats file are skipped on remote). Would there be any point to moving this test to another bats file? (My inclination is no, but I think it's worth an explicit decision)
|
Thanks for the review, @edsantiago! Wow, how much a phone call can distract me. |
vrothberg
force-pushed
the
fix-15879
branch
from
f630d4b to
433408c
Compare
I do not feel strongly either way. If you have a preference, I happily move the test around. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, 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:
Approvers can indicate their approval by writing |
|
Is there any conceivable situation in which this could happen? # podman create --label ... shortname:latest
Error: you can't do that
# podman-remote !*
[runs just fine]I'm guessing no. But if there is any possible code flow where that could pass, then it might be worth adding a regression test. |
No, there is none to my knowledge. There are various ways to create a container but all will be validated. That's why I moved it there. Initially, the check was in specgen which clearly broke at some point (also due to the missing tests). |
|
LGTM |
|
LGTM. Tests are red. |
vrothberg
force-pushed
the
fix-15879
branch
from
433408c to
ebed462
Compare
Fixed. There was a redundant test that I removed. @edsantiago, can you give your blessing for that? |
|
/lgtm |
openshift-ci
bot
added
the
do-not-merge/hold
|
OK hold on: is it intentional to revert #10063 then? Because it looks like that PR was deliberate. And if that will be reverted, it looks like we need many more reversions, such as to the man pages and the code? |
That is a fair point. It would break backwards compat as the "local" policy allows short names. In retrospect, I think that was a bad idea but that doesn't justify breaking things. Thank you for catching, Ed. |
vrothberg
force-pushed
the
fix-15879
branch
from
ebed462 to
9b8f2cb
Compare
Auto updates using the "registry" policy require container to be created with a fully-qualified image reference. Short names are not supported due the ambiguity of their source registry. Initially, container creation errored out for non FQN images but it seems that Podman has regressed. Fixes: containers#15879 Signed-off-by: Valentin Rothberg <[email protected]>
vrothberg
force-pushed
the
fix-15879
branch
from
9b8f2cb to
7bc3660
Compare
|
/lgtm |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
Auto updates require containers to be created with a fully-qualified image reference. Short names are not supported due the ambiguity of their source registry. Initially, container creation errored out for non FQN images but it seems that Podman has regressed.
Fixes: #15879
Signed-off-by: Valentin Rothberg [email protected]
Does this PR introduce a user-facing change?