Skip to content
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

fix: remove image tag only not the image using id #3837

Merged
merged 5 commits into from Sep 20, 2023

Conversation

dgolovin
Copy link
Contributor

@dgolovin dgolovin commented Sep 8, 2023

What does this PR do?

  • switches to use name:tag to delete image so only specific tag is deleted not the image itself and all the tags
  • run delete commands as a chain to avoid conflicts when deleting different tags of the same image

Screenshot/screencast of this PR

n/a

What issues does this PR fix or reference?

Fix #2673

How to test this PR?

Create different tags for the same image and try to delete them using podman-desktop ui. The selected tag should be deleted, not all the tags of the same image.

@dgolovin dgolovin requested review from benoitf and a team as code owners September 8, 2023 07:05
@dgolovin dgolovin requested review from jeffmaury and cdrage and removed request for a team September 8, 2023 07:05
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's missing test where images have same id but different tags, etc

@dgolovin
Copy link
Contributor Author

dgolovin commented Sep 9, 2023

@benoitf I will do that, but that is rather an integration test that should be executed with real podman. I can mock it, but that would not be a correct validation, because I can mock it to work, but it does not mean it works with actual podman.

All new code seems to be covered with existing tests.

@dgolovin dgolovin force-pushed the i2673-delete-image-tag-only branch 2 times, most recently from 0ebfacf to b7621e5 Compare September 12, 2023 20:45
packages/renderer/src/lib/image/ImageDetails.svelte Outdated Show resolved Hide resolved
tests/src/model/pages/images-page.ts Outdated Show resolved Hide resolved
tests/src/utility/wait.ts Outdated Show resolved Hide resolved
tests/src/utility/wait.ts Outdated Show resolved Hide resolved
@dgolovin dgolovin force-pushed the i2673-delete-image-tag-only branch 2 times, most recently from 187ba03 to 1708563 Compare September 14, 2023 04:21
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work 👍

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I was testing again and noticed that there is one usecase where we have a regression

I have some leftover images and I was no longer able to remove these images

I've checked with the main branch and it's working so it's related to this PR

it's for images without a tag

for example do

podman pull quay.io/podman/hello
podman untag quay.io/podman/hello

then you see an image like
image

and trying to remove it leads to

image

then try to remove this image

@dgolovin
Copy link
Contributor Author

Good catch. I remember I've seen images like this, but did not pay enough attention to it :(.

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now it works as expected 👍

maybe add a test for the <none> usecase as it's not covered by a test, so we might forget about it later

This fix:
* switches to use name:tag to delete image
* run delete commands as a chain to avoid conflicts when
  deleting different tags of the same image

Fix containers#2673

Signed-off-by: Denis Golovin <dgolovin@redhat.com>
Signed-off-by: Denis Golovin <dgolovin@redhat.com>
Signed-off-by: Denis Golovin <dgolovin@redhat.com>
Signed-off-by: Denis Golovin <dgolovin@redhat.com>
Signed-off-by: Denis Golovin <dgolovin@redhat.com>
@dgolovin dgolovin merged commit 322c164 into containers:main Sep 20, 2023
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.5.0 milestone Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete Action deletes all tags with the same Image ID
3 participants