-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove the ability to use [name:tag] in podman load command #8877
Conversation
b3c1bc0
to
637d630
Compare
Changes LGTM, But, you remove a few tests then the test run times out before it completes? I've restarted. |
@QiWang19 @vrothberg @mtrmac PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM assuming happy tests
if t, ok := ref.(reference.Tagged); ok { | ||
loadOpts.Tag = t.Tag() | ||
} else { | ||
loadOpts.Tag = "latest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn’t the loadOpts.Tag
field, and similarly all the way down (other than fixed APIs), be removed then, as well? Otherwise we won’t get rid of the ambiguity throughout the codebase.
f0c32d6
to
911cca4
Compare
PR containers#8851 broke CI: it included "/var/run" strings that, per containers#8771, should have been just "/run". Signed-off-by: Ed Santiago <[email protected]>
Docker does not support this, and it is confusing what to do if the image has more then one tag. We are dropping support for this in podman 3.0 Fixes: containers#7387 Signed-off-by: Daniel J Walsh <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, rhatdan 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 |
We _usually_ have only one image in store, $IMAGE, but it's perfectly fine to also have $SYSTEMD_IMAGE also. Fix a few tests so they can handle that condition. And, cleanup: - remove a no-longer-useful test ("podman load NEWNAME", functionality that was removed 2+ years ago in containers#8877) - reorder some tests in the image-mount test, to make them safer and easier to understand - use no-such-image, not no-such-container, in image-mount test. Computer don't care, but this human felt confused for a sec. Signed-off-by: Ed Santiago <[email protected]>
We _usually_ have only one image in store, $IMAGE, but it's perfectly fine to also have $SYSTEMD_IMAGE also. Fix a few tests so they can handle that condition. And, cleanup: - remove a no-longer-useful test ("podman load NEWNAME", functionality that was removed 2+ years ago in containers#8877) - reorder some tests in the image-mount test, to make them safer and easier to understand - use no-such-image, not no-such-container, in image-mount test. Computer don't care, but this human felt confused for a sec. Signed-off-by: Ed Santiago <[email protected]>
Docker does not support this, and it is confusing what to do if
the image has more then one tag. We are dropping support for this
in podman 3.0
Fixes: #7387
Signed-off-by: Daniel J Walsh [email protected]