Add a --force flag to force deletion #523
Conversation
Codecov Report
@@ Coverage Diff @@
## master #523 +/- ##
=======================================
Coverage 69.35% 69.35%
=======================================
Files 55 55
Lines 2973 2973
=======================================
Hits 2062 2062
Misses 627 627
Partials 284 284Continue to review full report at Codecov.
|
silvin-lubecki
force-pushed
the
force-deletion
branch
from
18dd557
to
557830c
Compare
|
PTAL @ijc |
| if err := installationStore.Delete(installationName); err != nil { | ||
| fmt.Fprintf(os.Stderr, "failed to force deletion of installation %q: %s\n", installationName, err) | ||
| } | ||
| fmt.Fprintf(os.Stderr, "deletion forced for installation %q\n", installationName) |
There was a problem hiding this comment.
I think the ordering of these two messages will be backwards in the case where they are both printed:
failed to force deletion of installation "foo": something went wrong
deletion forced for installation "foo"
I think you should move this second print up so the output would be:
deletion forced for installation "foo"
failed to force deletion of installation "foo": something went wrong
Should we also log mainErr or does that already happen e.g. in the caller?
There was a problem hiding this comment.
If we hit this path should we also print something along the lines "some additional manual cleanup may be required"?
There was a problem hiding this comment.
in the case where they are both printed:
You are right about the 2 messages, but I guess is case of failure I should just return and never print "deletion forced...".
Should we also log mainErr or does that already happen e.g. in the caller?
mainErr is handled by cobra afterwards, as we are running a RunE 👍
If we hit this path should we also print something along the lines "some additional manual cleanup may be required"?
I don't know about that, because it may be a privilege issue, so maybe the user needs to be root and retry. I let the user figure why it fails, as we print the error too.
There was a problem hiding this comment.
Can we e2e test this somehow? Do we have a way to provoke a normal uninstall failure?
I'll do that in a followup 👍
There was a problem hiding this comment.
You are right about the 2 messages, but I guess is case of failure I should just return and never print "deletion forced...".
SGTM.
I'll do that in a followup
Ack
… when the uninstall action repeatedly fails. Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
silvin-lubecki
force-pushed
the
force-deletion
branch
from
557830c
to
3f98389
Compare
- What I did
When the uninstall action will always fail, the new
--forceflag will delete the installation in the installation store.- A picture of a cute animal (not mandatory but encouraged)