Skip to content

Add --force option to network rm subcommand #3547

Merged
thaJeztah merged 1 commit intodocker:masterfrom
cavcrosby:2382-add-force-flag
Apr 30, 2022
Merged

Add --force option to network rm subcommand #3547
thaJeztah merged 1 commit intodocker:masterfrom
cavcrosby:2382-add-force-flag

Conversation

@cavcrosby
Copy link
Copy Markdown
Contributor

@cavcrosby cavcrosby commented Apr 9, 2022

- What I did

Added a -f / --force option to the network's rm subcommand. What this option allows is for a user to remove a configured Docker network but return a zero status code even if said network does not exist (stdout will also still display the network name). Similar to Unix's rm --force command. This closes #2382.

- How I did it

I was inspired by the volume rm subcommand's --force option (at least on the client-side) so hence my implementation is similar to it. However, like #2678, my changes only affect the client and are currently not implemented for the daemon.

- How to verify it

docker network rm --force foo

- Description for the changelog

Add --force option to network rm subcommand.

- A picture of a cute animal (not mandatory but encouraged)

image

@cavcrosby cavcrosby requested a review from thaJeztah as a code owner April 9, 2022 03:16
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 9, 2022 •
edited
Loading

Codecov Report

Merging #3547 (1cc09e2) into master (c780f7c) will increase coverage by 1.79%.
The diff coverage is 100.00%.

❗ Current head 1cc09e2 differs from pull request most recent head 33cba73. Consider uploading reports for the commit 33cba73 to get more accurate results

@@            Coverage Diff             @@
##           master    #3547      +/-   ##
==========================================
+ Coverage   57.33%   59.12%   +1.79%     
==========================================
  Files         304      284      -20     
  Lines       26379    23823    -2556     
==========================================
- Hits        15124    14086    -1038     
+ Misses      10329     8881    -1448     
+ Partials      926      856      -70

@cavcrosby
Copy link
Copy Markdown
Contributor Author

cavcrosby commented Apr 9, 2022 •
edited
Loading

That's no bueno (or good, and in regards to the checks), err, can I get a maintainer's thoughts?

At least in concerns to the automated checks performed:

cli/command/network/remove_test.go:5:2: 'io/ioutil' is in the blacklist (depguard)

  • I can discard where the test uses the ioutil package, but many tests do set (*Command).outWriter to 'ioutil.discard'. So not entirely sure why this is an issue?
Run go test -coverprofile=/tmp/coverage.txt $(go list ./... | grep -vE '/vendor/|/e2e/')
ok  	github.com/docker/cli/cli	0.206s	coverage: 25.4% of statements
ok  	github.com/docker/cli/cli/command	0.270s	coverage: 48.0% of statements
?   	github.com/docker/cli/cli/command/builder	[no test files]
ok  	github.com/docker/cli/cli/command/checkpoint	0.214s	coverage: 90.9% of statements
?   	github.com/docker/cli/cli/command/commands	[no test files]
ok  	github.com/docker/cli/cli/command/config	0.224s	coverage: 90.1% of statements
Error: something went wrong
Error: You cannot attach to a stopped container, start it first
Error: You cannot attach to a paused container, unpause it first
Error: You cannot attach to a restarting container, wait until it is running
Error: client is offline
Error: Error: remote trust data does not exist for docker.io/library/image:  does not have trust data for 
Error: No valid trust data for tag
Error: something went wrong
Error: failed to export container: invalid output path: "/dev/random" must be a directory or a regular file: got a device
Error: failed to parse template: template: :1: function "invalid" not defined
Error: failed to execute template: template: :1:2: executing "" at <join>: wrong number of args for join: want 2 got 0
Error: error listing containers
Error: Error: no such container: nosuchcontainer
Error: Status: , Code: 125
Error: Status: , Code: 125
Error: Status: , Code: 125
--- FAIL: TestInitTtySizeErrors (0.13s)
  • Err, has this been seen before (see the 'test / host (macos-latest) (pull_request)' check)?

thaJeztah
thaJeztah requested changes Apr 14, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I left some suggestions inline, but was writing some of the changes while reviewing, so I'll open a pull request against your branch with those

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Apr 14, 2022

I opened https://github.com/cavcrosby/docker-cli/pull/1 (which, once merged, should appear in this PR); perhaps you can also squash the commits (no need to preserve my commit as a separate commit); happy to help doing so if you want!

@thaJeztah thaJeztah added this to the 22.04.0 milestone Apr 14, 2022
@cavcrosby cavcrosby requested a review from albers as a code owner April 22, 2022 01:51
@cavcrosby
Copy link
Copy Markdown
Contributor Author

cavcrosby commented Apr 22, 2022 •
edited
Loading

Squashed your commit, and closed out the PR on cavcrosby#1. Should be good to go but let me know what you think.

albers
albers approved these changes Apr 25, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@albers albers left a comment

Choose a reason for hiding this comment

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

Completion LGTM, thanks.

@cavcrosby @thaJeztah
Add --force option to network rm subcommand
0ea587b 
The code is similar to that used by the volume rm subcommand, however,
one difference I noticed was VolumeRemove takes the force flag/option
was a parameter. This isn't the case for NetworkRemove.

To get NetworkRemove to take a similar parameter, this would require
modifying the Docker daemon. For now this isn't a route I wish to take
when the code can be arrange to mimic the same behavior.

Co-authored-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Conner Crosby <conner@cavcrosby.tech>
thaJeztah
thaJeztah reviewed Apr 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

changes LGTM, thanks!

I think we can combine the 5 commits into one, as they're all related (and some of the later ones touch up previous ones); let me do a quick squash and push to the PR branch

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Apr 30, 2022

All green; let's get this one in

Thanks!

@thaJeztah thaJeztah merged commit b3ab7c9 into docker:master Apr 30, 2022
@cavcrosby cavcrosby deleted the 2382-add-force-flag branch May 29, 2022 20:00
matt9ucci added a commit to matt9ucci/DockerCompletion that referenced this pull request Feb 9, 2023
@matt9ucci
CLI 23.0.0: add --force to network rm
66e5dcf 
See docker/cli#3547
matt9ucci added a commit to matt9ucci/DockerCompletion that referenced this pull request Feb 9, 2023
@matt9ucci
CLI 23.0.0: add --force to network rm
8e729cb 
See docker/cli#3547
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@albers albers albers approved these changes

Assignees

No one assigned

Labels

impact/changelog kind/enhancement status/2-code-review

Projects

None yet

Milestone

23.0.0

Development

Successfully merging this pull request may close these issues.

Add a 'docker network rm --force' flag

4 participants

@cavcrosby @codecov-commenter @thaJeztah @albers