-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add --replace option to podman artifact add command #27153
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
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rhatdan 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 |
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
||
Print usage statement. | ||
|
||
#### **--replace** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicated, you can use @@option replace
and do a small edit in options/replace.md
to add artifacts in there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only works with pods and containers, and would take a lot more hacking to get it to work with artifacts.
// If replace is true, try to remove existing artifact (ignore errors if it doesn't exist) | ||
if opts.Replace { | ||
_, _ = artStore.Remove(ctx, name) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my preference would be to examine the error here, and if it is a notexists error, move on; otherwise, blow up OR at the least, drop some debug out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
test/e2e/artifact_test.go
Outdated
Expect(listSession.OutputToStringArray()).To(HaveLen(2)) // header + 1 artifact | ||
}) | ||
|
||
It("podman artifact add --replace nonexistent artifact", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is just the AI not thinking about test times. I will make the change.
thanks for the submission @rhatdan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AI has missed registering this new option: https://github.com/rhatdan/podman/blob/0bbf989fceb05f3c0f037550890bc2af0a0ef9d1/pkg/api/server/register_artifacts.go#L149-L195
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and I guess subsequently an API test for it as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I told cursor:
You missed registering the new --replace option with @https://github.com/rhatdan/podman/blob/0bbf989fceb05f3c0f037550890bc2af0a0ef9d1/pkg/api/server/register_artifacts.go#L149-L195 and we need an API test as well
Will see how it does.
ad1abd3
to
a93f41c
Compare
This commit implements the --replace functionality for the artifact add command, allowing users to replace existing artifacts without having to manually remove them first. Changes made: - Add Replace field to ArtifactAddOptions entity types - Add --replace CLI flag with validation to prevent conflicts with --append - Implement replace logic in ABI backend to remove existing artifacts before adding - Update API handlers and tunnel implementation for podman-remote support - Add comprehensive documentation and examples to man page - Add e2e and system BATS tests for --replace functionality - Fix code formatting in pkg/bindings/artifacts/types_pull_options.go: * Reorder imports with proper spacing * Fix function declaration spacing * Convert spaces to proper tab indentation * Remove extraneous blank lines The --replace option follows the same pattern as other podman replace options like 'podman container create --replace' and 'podman pod create --replace'. It gracefully handles cases where no existing artifact exists (no error thrown). Usage examples: podman artifact add --replace quay.io/myimage/artifact:latest /path/to/file podman artifact add --replace localhost/test/artifact /tmp/newfile.txt Fixes: Implements requested --replace functionality for artifact add command Signed-off-by: Daniel J Walsh <[email protected]>
This commit implements the --replace functionality for the artifact add command, allowing users to replace existing artifacts without having to manually remove them first.
Changes made:
The --replace option follows the same pattern as other podman replace options like 'podman container create --replace' and 'podman pod create --replace'. It gracefully handles cases where no existing artifact exists (no error thrown).
Usage examples:
podman artifact add --replace quay.io/myimage/artifact:latest /path/to/file
podman artifact add --replace localhost/test/artifact /tmp/newfile.txt
Fixes: Implements requested --replace functionality for artifact add command
Fixes: #27082
Does this PR introduce a user-facing change?