Improve the robustness of image cleanup #313
Conversation
That makes the name useless
nuclearsandwich
left a comment
There was a problem hiding this comment.
Raised a concern about squelching the error but nothing blocking.
| try: | ||
| docker_client.remove_image(image_id, force=force) | ||
| except docker.errors.APIError as ex: | ||
| ## removing the image can fail if there's child images |
There was a problem hiding this comment.
It seems a little weird not to print or log a message when catching this error. The end-user has no way of knowing whether the image was cleaned up or not, especially with the default behavior of not failing on error.
There was a problem hiding this comment.
This will return a True/False and then it warns the user based on the results:
Line 437 in 4a6e191
And the default is False so it's only suppressed if the developer asks for it to be suppressed.
src/rocker/core.py
Outdated
| def docker_remove_image( | ||
| image_id, | ||
| docker_client = None, | ||
| output_callback = None, |
There was a problem hiding this comment.
This is kind of drive-by but output_callback appears completely unused. Is it deprecated?
There was a problem hiding this comment.
Yeah, this doesn't have the streaming interface so removing it makes sense.
As flagged in the review
Don't crash if there's a dependent image already
And don't cleanup if the user has given it a name. The name's useless if we clear it.