Skip to content

Feat artifact rm ignore #27142

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Feat artifact rm ignore #27142

wants to merge 1 commit into from

Conversation

nothiaki
Copy link
Contributor

@nothiaki nothiaki commented Sep 22, 2025 •
edited
Loading

Does this PR introduce a user-facing change?

Yes, --ignore flag.
Closes #27084

Added flag --ignore for "podman artifact rm" to ignoring errors when trying to remove artifacts that does not exists.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Sep 22, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nothiaki
Once this PR has been reviewed and has the lgtm label, please assign mheon for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@packit-as-a-service Packit-as-a-Service
Copy link

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

1 similar comment
@packit-as-a-service Packit-as-a-Service
Copy link

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

Honny1
Honny1 suggested changes Sep 22, 2025
View reviewed changes
Copy link
Member

@Honny1 Honny1 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, I've checked your changes and I think the --ignore flag should also be available for remote clients.

Also, please link the correct issue number.

Remote Client file
API file

@nothiaki
Copy link
Contributor Author

Thanks, I've checked your changes and I think the --ignore flag should also be available for remote clients.

Also, please link the correct issue number.

Remote Client file API file

Alright, I will do it!
and I changed the issue number!

@nothiaki nothiaki force-pushed the feat-artifact-rm-ignore branch from 0de706e to f0724e4 Compare September 23, 2025 01:36
@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Sep 23, 2025
@nothiaki
Copy link
Contributor Author

@Honny1 I think the PR is done, but the CI is failing.
The errors seems to be related to some setup warnings.
Im not sure how to fix these.

baude
baude reviewed Sep 23, 2025
View reviewed changes
docs/source/markdown/podman-artifact-rm.1.md Outdated
Deleted: 72875f8f6f78d5b8ba98b2dd2c0a6f395fde8f05ff63a1df580d7a88f5afa97b
```

Remove artifacts ignoring the errors if the artifact does not exists.
Copy link
Member

Choose a reason for hiding this comment

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

s/exists/exist

@baude
Copy link
Member

baude commented Sep 23, 2025

This looks good to me. I wonder if we should have a logrus.debug for when the artifact does not exist? it might help down the line, but I dont feel strongly about it. You have one grammatical/spelling mistake to fix at least. And thanks for your submission.

@nothiaki
Copy link
Contributor Author

onder if we should have a logrus.debug for when the artifact does not exist? it might help down the line, but I dont feel strongly about it. You have o

I fixed the grammatical mistake!
And added the logrus.Debug just in case.

Honny1
Honny1 reviewed Sep 24, 2025
View reviewed changes
Copy link
Member

@Honny1 Honny1 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM. Just one small request: could you please squash all your commits into one?

mheon
mheon reviewed Sep 24, 2025
View reviewed changes
pkg/api/handlers/libpod/artifacts.go
if err != nil {
if errors.Is(err, libartifact_types.ErrArtifactNotExist) {
if removeOptions.Ignore {
return
Copy link
Member

@mheon mheon Sep 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think you need to write a 200 header before you return here

mheon
mheon reviewed Sep 24, 2025
View reviewed changes
pkg/domain/infra/abi/artifact.go Outdated
artifactDigest, err := artStore.Remove(ctx, namesOrDigest)
if err != nil {
if opts.Ignore {
logrus.Debugf("Artifact does not exist, ignoring error (--ignore): %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe include namesOrDigest in the error somewhere so it's very clear what artifact wasn't there

@nothiaki nothiaki force-pushed the feat-artifact-rm-ignore branch from bdaba0f to f1464ef Compare September 24, 2025 17:03
@mheon mheon mentioned this pull request Sep 24, 2025
Closed
baude
baude reviewed Sep 24, 2025
View reviewed changes
docs/source/markdown/podman-artifact-rm.1.md Outdated

Remove artifacts ignoring the errors if the artifact does not exist.
```
$ podman artifact rm -i NonExistsId
Copy link
Member

Choose a reason for hiding this comment

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

i would prefer either a full or partial uuid OR a fully qualified name here so we dont confuse users about artifacts not honoring shortnames.

@nothiaki
cmd/podman: --ignore errors flag to artifact rm
208b9d3 
Signed-off-by: Celso Henrique Souza Silva <celsohenrique367@gmail.com>
@nothiaki nothiaki force-pushed the feat-artifact-rm-ignore branch from f1464ef to 208b9d3 Compare September 24, 2025 19:39
@TomSweeneyRedHat
Copy link
Member

LGTM
once @mheon's return concern is addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mheon mheon mheon left review comments

@baude baude baude left review comments

+1 more reviewer

@Honny1 Honny1 Honny1 requested changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
kind/api-change Change to remote API; merits scrutiny release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add --ignore option to podman artifact rm.
5 participants
@nothiaki @baude @TomSweeneyRedHat @mheon @Honny1