docker image rm -f should exit with 1 if the image was not removed#418
docker image rm -f should exit with 1 if the image was not removed#418adshmh wants to merge 1 commit intodocker:masterfrom
Conversation
|
I edited the files under vendor directory so all changes are seen together in the PR. If the changes are approved, I will submit a PR against docker/docker (with the addition of tests for new type of error and the API client). |
| if err != nil { | ||
| if apiclient.IsErrImageRemoveConflict(err) { | ||
| removeConflict = true | ||
| } |
There was a problem hiding this comment.
I think we should invert this. Any NotFound can be skipped in the force case, but any other errors should still be reported as errors.
There was a problem hiding this comment.
Thank you for the review. I just updated the PR. Two of the unit tests also needed to be changed, as with the new logic, the -f option can only skip imageNotFound errors.
449d66a to
2df56fa
Compare
Codecov Report
@@ Coverage Diff @@
## master #418 +/- ##
==========================================
+ Coverage 46.28% 46.28% +<.01%
==========================================
Files 194 194
Lines 16134 16138 +4
==========================================
+ Hits 7467 7470 +3
- Misses 8278 8279 +1
Partials 389 389 |
dnephin
left a comment
There was a problem hiding this comment.
Thanks, this is looking good, just a few minor comments
| return []types.ImageDeleteResponseItem{{Deleted: image}}, nil | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
I guess we should still have a case here "Image not found with force option"
There was a problem hiding this comment.
Thank you for the review. I added a test case.
Would it be a good idea for the docker/docker/client package to export the notFound* errors to make testing easier? I think in addition to the new test, stack/deploy_composefile_test.go also implements a type of notFound error for testing.
There was a problem hiding this comment.
Possibly. The problem with exporting them is that other packages could start to use them to report errors, which is not really the intent.
cli/command/image/remove.go
Outdated
| for _, img := range images { | ||
| dels, err := client.ImageRemove(ctx, img, options) | ||
| if err != nil { | ||
| if !apiclient.IsErrImageNotFound(err) { |
There was a problem hiding this comment.
minor, but these object specific error checkers should be deprecated. Please use IsErrNotFound() instead. IsErrImageNotfound() just calls that directly anyway.
cli/command/image/remove.go
Outdated
| } | ||
|
|
||
| var errs []string | ||
| var unresolvableErr = false |
There was a problem hiding this comment.
minor: I'm not sure about the term "resolvable" here. Maybe "unrecoverable" or just "fatal" ?
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
2df56fa to
020703e
Compare
|
Thanks, looks good! Please ping me on the moby/moby PR to apply this fix to |
|
@dnephin thank you for the reminder, it seems the changes are now in vendor/. I will rebase. |
|
ping @adshmh could you rebase? I think docker 17.11-rc1 is scheduled to be cut off today; would be nice to get this in 👍 |
|
I'll carry this |
|
Yes, carrying this will also close #567 |
docker image rm -fshould exit with error code 1 if the image was not removed. Fixes #394Signed-off-by: Arash Deshmeh adeshmeh@ca.ibm.com
- What I did![]()
' exit with 1 instead of 0 when an image cannot be removed (e.g. because running containers are using the image)
Added logic that makes 'docker image rm -f
- How I did it
- How to verify it
- Description for the changelog
docker image rm -fexits with 1 if the image is not removed.- A picture of a cute animal (not mandatory but encouraged)