Passes 'docker load' error back to user. #334
Conversation
| // This error usually happens when docker is not able to connect to the daemon socket. | ||
| try (InputStreamReader stderr = | ||
| new InputStreamReader(dockerProcess.getErrorStream(), StandardCharsets.UTF_8)) { | ||
| String error = CharStreams.toString(stderr); |
There was a problem hiding this comment.
can 87-89 throw an IOException? If so, we might want to be careful about passing that through to the caller of load
There was a problem hiding this comment.
Yeah, this assumes IOException will be errors at the docker application level only (i.e., the docker process knows what the issue was and prints it to stderr). Technically speaking, writing to a process stdin may fail for reasons other than application level failures. For example, the docker process crashing for some unknown reason, docker command-not-found, no executable-bit set on the docker command binary, etc., and in such cases, dockerProcess.getErrorStream() may have nothing but the original IOException would better describe the situation.
There was a problem hiding this comment.
I reorganized the code a bit and passed the original IOException as a cause to the custom generated IOException so that it wouldn't be left discarded.
| try (OutputStream stdin = dockerProcess.getOutputStream()) { | ||
| imageTarballBlob.writeTo(stdin); | ||
| } | ||
| writeToStdin( |
There was a problem hiding this comment.
This new method feels like it adds a high cost to readability/code complexity without enough benefit. It feels like we're decoupling the exception handling and method calls in an unnatural way. I would prefer if we kept this simple here (was this for testing)
There was a problem hiding this comment.
Okay, I moved everything back to load.
Fixes #330
This exposes the
docker loaderror to the user so that they can take action to fix it.@chanseokoh