Add warning while untagging an image podman-load#3257
Add warning while untagging an image podman-load#3257openshift-merge-robot merged 1 commit intocontainers:masterfrom
Conversation
|
Can one of the admins verify this patch?
|
Signed-off-by: Divyansh Kamboj <kambojdivyansh2000@gmail.com>
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, weirdwiz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
bot, test pull request |
| localImage, err := ir.NewFromLocal(dst) | ||
| imageID := strings.TrimSuffix(manifest[0].Config, ".json") | ||
| if err == nil && imageID != localImage.ID() { | ||
| logrus.Errorf("the image %s already exists, renaming the old one with ID %s to empty string", dst, localImage.ID()) |
There was a problem hiding this comment.
Suggest rewording - `"tag %s already exists, untagging but not deleting old image"
There was a problem hiding this comment.
Oh, but you'd want to keep the ID in there... maybe tag %s already exists as ID %s, untagging but not deleting old image?
There was a problem hiding this comment.
untagging would mean removing the tag, maybe I could reword it to
tag %s already exists as ID %s, tagging the old image as an empty string
There was a problem hiding this comment.
Back to my original point, unless I'm being oblivious, this code is not actually tagging the old image as an empty string is it? I think in this message you need to tell the user what action they need to do as we're not doing it for them.
There was a problem hiding this comment.
We're specifically retagging as empty string? I think internally that's considered an untag of the old image... @baude that sound right to you?
There was a problem hiding this comment.
Internally this set's the name of the image image.Names = stringSliceWithoutValue(image.Names, name) in the file https://github.com/containers/storage/blob/master/images.go .
The thing that is confusing me is that if I do podman rmi imagename then if the image exists with another tag then it simply gets untagged and is not visible when I run podman images, but in this case, the image is visible in podman images as <none> .
There was a problem hiding this comment.
...Could this be the dreaded literal tag returning... Let me poke around
| localImage, err := ir.NewFromLocal(dst) | ||
| imageID := strings.TrimSuffix(manifest[0].Config, ".json") | ||
| if err == nil && imageID != localImage.ID() { | ||
| logrus.Errorf("the image %s already exists, renaming the old one with ID %s to empty string", dst, localImage.ID()) |
There was a problem hiding this comment.
"renaming" implies the software is in the process of changing the images name which doesn't seem to be happening?
Perhaps:
"the image %s already exists, rename the image with ID %s to another value and retry"
There's probably a better way to say the above, but that's all I got after one cup of tea.
There was a problem hiding this comment.
Docker only gives this warning and continues to untag it, so I only added the warning
|
@mheon @TomSweeneyRedHat @baude Where do we stand on this PR? |
|
I'll give it another look, but we were just nitpicking the wording of the
warning. It's basically ready.
…On Sat, Jun 8, 2019, 06:50 Daniel J Walsh ***@***.***> wrote:
@mheon <https://github.com/mheon> @TomSweeneyRedHat
<https://github.com/TomSweeneyRedHat> @baude <https://github.com/baude>
Where do we stand on this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3257>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB3AOCCM4SFTWTTGWD2BVOLPZOFGZANCNFSM4HS3YEAQ>
.
|
|
The error message still needs to change IMO. The message implies that the code is changing something when AFAIKS, it's only reporting an issue. The code other than the message is fine. |
|
@TomSweeneyRedHat AFAIK the code does remove the name of the image with the same name but a different ID as the image loaded. I am unsure whether it should be called untagging, or renaming to something empty. |
|
If @mheon is happy with the messaging, then it's fine by me, But unless I'm missing the functionality of the code (very possible), I think it's a misleading message. |
|
/lgtm |
Adds a warning if another image with the same name exists and is going to be untagged
fixes #2821