Add --all to artifact rm #25355
Add --all to artifact rm #25355
Conversation
openshift-ci
bot
added
release-note
approved
| for _, art := range allArtifacts { | ||
| // Using the digest here instead of name to protect against | ||
| // an artifact that lacks a name | ||
| manifestDigest, err := art.GetDigest() |
There was a problem hiding this comment.
Should we ignore this and continue to catch TOCTOU?
There was a problem hiding this comment.
We don't need a multi error IMO, but what we have to do is ignore the no such artifact error silently and just continue. Otherwise --all can produce an error if it races against other concurrent removals.
That said none of the artifact store is race free, one can easily corrupt the store right now. I am fine to keep this particular problem for another time. I don't think we have proper types error either that we could match.
| } | ||
| } | ||
|
|
||
| if name != "" { |
There was a problem hiding this comment.
Should we detect if options.All was also specified here and error?
There was a problem hiding this comment.
my initial thought was ... we detect this on the client side already which covers local and remote-client use. So for the RESTFUL side, i would add to the tunnel side?
| artifactRemoveReport, err := registry.ImageEngine().ArtifactRm(registry.Context(), args[0], entities.ArtifactRemoveOptions{}) | ||
| var nameOrID string | ||
| if len(args) > 0 { | ||
| nameOrID = args[0] |
There was a problem hiding this comment.
What about multiple arguments being passed? Should we loop over args?
There was a problem hiding this comment.
checkAllAndArgs() checks that prior
There was a problem hiding this comment.
I don't see that check. It just verifies it's more than 0 - there's no check that it is exactly 1. As written, it seems arguments past 1 are discarded silently.
There was a problem hiding this comment.
@Luap99 made the same basic comment and I have it fixed locally
baude
force-pushed
the
artifactrmall
branch
2 times, most recently
from
3729f5c to
4684f7f
Compare
baude
force-pushed
the
artifactrmall
branch
2 times, most recently
from
6db00ad to
48b0b79
Compare
Add the ability to remove all artifacts with a --all|-a option in podman artifact rm. Fixes: https://issues.redhat.com/browse/RUN-2512 Signed-off-by: Brent Baude <bbaude@redhat.com>
| var ( | ||
| namesOrDigests []string | ||
| ) | ||
| artifactDigests := make([]*digest.Digest, 0, len(namesOrDigests)) |
There was a problem hiding this comment.
not blocking but this does not solve the pre allocate part because len(namesOrDigests) is 0 here. This must be moved directly above the artStore.Remove() loop.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, Luap99 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 |
Add the ability to remove all artifacts with a --all|-a option in podman artifact rm.
Fixes: https://issues.redhat.com/browse/RUN-2512
Does this PR introduce a user-facing change?