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.

Sign up for GitHub

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

Jump to bottom

manifest: Make sure manifest rm only removes manifests not referenced images. #3492

Merged
merged 2 commits into from
Sep 4, 2021
Merged

manifest: Make sure manifest rm only removes manifests not referenced images. #3492

merged 2 commits into from
Sep 4, 2021

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Sep 1, 2021

Following PR makes sure that buildah manifest rm <list> only removes
the manifest list not referenced images.

Depends on: containers/common#753

@flouthoc
Copy link
Collaborator Author

flouthoc commented Sep 1, 2021 •
edited
Loading

I need to change flouthoc/common to c/common in go.mod as soon as containers/common#753 is merged.

@flouthoc flouthoc mentioned this pull request Sep 1, 2021
Closed
@flouthoc flouthoc linked an issue Sep 1, 2021 that may be closed by this pull request
Buildah rmi untagged/removed the wrong image/manifest-list #3490
Closed
@giuseppe
Copy link
Member

giuseppe commented Sep 1, 2021

could you split the common upgrade to a different commit?

@flouthoc
Copy link
Collaborator Author

flouthoc commented Sep 1, 2021

Sure.

@flouthoc flouthoc force-pushed the manifest-rm-only-manifest branch from 76f4b55 to 4f50ba4 Compare September 2, 2021 10:44
vrothberg
vrothberg reviewed Sep 2, 2021
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

One addition to the test, other than that, LGTM

Nice work!

tests/rm.bats Show resolved Hide resolved
@flouthoc flouthoc force-pushed the manifest-rm-only-manifest branch 2 times, most recently from 3dd07a6 to c84c568 Compare September 2, 2021 13:01
@flouthoc
Copy link
Collaborator Author

flouthoc commented Sep 2, 2021

@vrothberg @giuseppe PTAL

@vrothberg
Copy link
Member

@nalind @ashley-cui PTAL

nalind
nalind reviewed Sep 2, 2021
View reviewed changes
tests/rm.bats
run_buildah manifest rm manifestsample
# Since actual list is getting removed it will also print the image id of list
# So check for substring instead of exact match
expect_output --substring "untagged: localhost/manifestsample:latest"
Copy link
Member

Choose a reason for hiding this comment

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

Unless this is covered by another test, verifying that the image we added to the list (busybox) is still present would at least make me feel better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nalind That's a very fair request. Similar check was added in podman for this fix adding it here as well. :)

@flouthoc
manifest: rm should remove only manifests not referenced images.
f037ce4 
Following commit makes sure that `buildah manifest rm <list>` only removes
the manifest list not referenced images.

Signed-off-by: Aditya Rajan <[email protected]>
@flouthoc flouthoc force-pushed the manifest-rm-only-manifest branch from c84c568 to f037ce4 Compare September 3, 2021 01:15
@flouthoc flouthoc requested a review from nalind September 3, 2021 06:39
@flouthoc
Copy link
Collaborator Author

flouthoc commented Sep 3, 2021

@nalind @ashley-cui @giuseppe PTAL

@rhatdan
Copy link
Member

rhatdan commented Sep 4, 2021

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 4, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 4, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Sep 4, 2021
@openshift-merge-robot openshift-merge-robot merged commit b6c883d into containers:main Sep 4, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vrothberg vrothberg vrothberg left review comments

@giuseppe giuseppe Awaiting requested review from giuseppe

@nalind nalind Awaiting requested review from nalind

Assignees

@rhatdan rhatdan

Labels
approved lgtm locked - please file new issue/PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Buildah rmi untagged/removed the wrong image/manifest-list
6 participants
@flouthoc @giuseppe @vrothberg @rhatdan @nalind @openshift-merge-robot