Skip to content
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

Add --force option to network rm subcommand #3547

Merged
merged 1 commit into from
Apr 30, 2022

Conversation

cavcrosby
Copy link
Contributor

- 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

codecov-commenter commented Apr 9, 2022

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
Contributor Author

cavcrosby commented Apr 9, 2022

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)?

Copy link
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

cli/command/network/remove.go Outdated Show resolved Hide resolved
cli/command/network/remove.go Outdated Show resolved Hide resolved
cli/command/network/remove.go Outdated Show resolved Hide resolved
cli/command/network/remove_test.go Outdated Show resolved Hide resolved
cli/command/network/remove_test.go Outdated Show resolved Hide resolved
@thaJeztah
Copy link
Member

I opened cavcrosby#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 cavcrosby force-pushed the 2382-add-force-flag branch from ec52be9 to 33cba73 Compare April 22, 2022 01:56
@cavcrosby
Copy link
Contributor Author

cavcrosby commented Apr 22, 2022

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

Copy link
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.

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 <[email protected]>
Signed-off-by: Conner Crosby <[email protected]>
Copy link
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 thaJeztah force-pushed the 2382-add-force-flag branch from 33cba73 to 0ea587b Compare April 29, 2022 11:57
@thaJeztah
Copy link
Member

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 added a commit to matt9ucci/DockerCompletion that referenced this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a 'docker network rm --force' flag
4 participants