-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
system: Adds support for removing all named destination via --all
#12213
system: Adds support for removing all named destination via --all
#12213
Conversation
@edsantiago PTAL |
31527af
to
9c87157
Compare
|
||
if len(args) < 1 { | ||
return errors.New("accepts 1 arg(s), received 0") | ||
} |
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.
Also need to check for >1 args
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 fixed
LGTM once the more-than-one-arg bug is fixed. Friendly suggestion: diff --git a/test/system/272-system-connection.bats b/test/system/272-system-connection.bats
index 5a90d9398..14c4f6664 100644
--- a/test/system/272-system-connection.bats
+++ b/test/system/272-system-connection.bats
@@ -34,10 +34,7 @@ function teardown() {
| xargs -l1 --no-run-if-empty umount
# Remove all system connections
- run_podman system connection ls --format json
- while read name; do
- run_podman system connection rm "$name"
- done < <(jq -r '.[].Name' <<<"$output")
+ run_podman system connection rm --all
basic_teardown
} |
9c87157
to
b1ad026
Compare
cfg, err := config.ReadCustomConfig() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if cmd.Flags().Changed("all") { |
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.
No reason to check Changed. If All is set we should do it, admittedly this would be a strange default to set.
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.
@rhatdan ah yes not needed. Removed in last commit :)
Adds support of dropping all named destination from system connections via `--all`. Closes: containers#12018 Signed-off-by: Aditya Rajan <arajan@redhat.com>
b1ad026
to
338eb9d
Compare
LGTM once tests go green |
@mheon @edsantiago @rhatdan PTAL |
/approve |
[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 |
Adds support of dropping all named destination from system connections via
--all
.Closes: #12018