docker context rm: allow --force to ignore non-existing contexts #3791
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3791 +/- ##
==========================================
+ Coverage 59.28% 59.29% +0.01%
==========================================
Files 288 288
Lines 24632 24639 +7
==========================================
+ Hits 14602 14609 +7
Misses 9160 9160
Partials 870 870 |
cli/command/context/remove.go
Outdated
| @@ -64,5 +63,21 @@ func doRemove(dockerCli command.Cli, name string, isCurrent, force bool) error { | |||
| return err | |||
| } | |||
| } | |||
|
|
|||
| if err := checkContextExists(dockerCli, name); err != nil && !force { | |||
There was a problem hiding this comment.
nit: The result of checkContextExists doesn't matter when !force. We could avoid the call entirely by checking !force first.
| if err := checkContextExists(dockerCli, name); err != nil && !force { | |
| if !force { | |
| if err := checkContextExists(dockerCli, name); err != nil { | |
| return err | |
| } | |
| } |
It also makes the !force check a bit more noticeable IMO.
There was a problem hiding this comment.
LOL! I looked at my PR Yesterday evening and thought: "I should update that to only run if force is not enabled", and was about to make that change when I saw your comment 👍
There was a problem hiding this comment.
The reason I didn't have this in the first place was that some other commands still print the error on --force, but not return it as an error.
It felt a bit too verbose, so I decided not to do that, but then forgot to update this condition 😅
Either way; updated 👍
cli/command/context/remove.go
Outdated
| @@ -26,7 +28,7 @@ func newRemoveCommand(dockerCli command.Cli) *cobra.Command { | |||
| return RunRemove(dockerCli, opts, args) | |||
| }, | |||
| } | |||
| cmd.Flags().BoolVarP(&opts.Force, "force", "f", false, "Force the removal of a context in use") | |||
There was a problem hiding this comment.
I'll temporarily change this back to the old string; while it's not "fully" accurate, the docs need some improvements as well for this (no description for this flag), so I'll update the flag description together with improving the docs in a follow-up
Before this change, running `docker context rm --force` would fail if the context did not exist. This behavior was different from other commands, which allowed ignoring non-existing objects. For example; when trying to remove a non-existing volume, the command would fail without "force": ```bash docker volume rm nosuchvolume Error: No such volume: nosuchvolume echo $? 1 ``` But using the `-f` / `--force` option would make the command complete successfully (the error itself is still printed for informational purposes); ```bash docker volume rm -f nosuchvolume nosuchvolume echo $? 0 ``` With this patch, `docker context rm` behaves the same: ```bash docker context rm nosuchcontext context "nosuchcontext" does not exist echo $? 1 ``` ```bash docker context rm -f nosuchcontext nosuchcontext echo $? 0 ``` This patch also simplifies how we check if the context exists; previously we would try to read the context's metadata; this could fail if a context was corrupted, or if an empty directory was present. This patch now only checks if the directory exists, without first validating the context's data. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
context_force_remove
branch
from
b48d325 to
9a493b1
Compare
|
CI is still happy; let me bring this one in, and rebase #3790 |
Before this change, running
docker context rm --forcewould fail if the context did not exist. This behavior was different from other commands, which allowed ignoring non-existing objects.For example; when trying to remove a non-existing volume, the command would fail without "force":
But using the
-f/--forceoption would make the command complete successfully (the error itself is still printed for informational purposes);With this patch,
docker context rmbehaves the same:This patch also simplifies how we check if the context exists; previously we would try to read the context's metadata; this could fail if a context was corrupted, or if an empty directory was present. This patch now only checks if the directory exists, without first validating the context's data.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)