Added error info on non 0 and unknown exit code #867
Conversation
When a command is executed with toolbox run and it returns a non zero exit code, if that exit code is not handled it is just ignored. This prevents users to identify errors when executing commands in toolbox. With this fix, the unhandled exit codes are returned as errors to give the user a better error info.
|
Build failed.
|
There was a problem hiding this comment.
Thanks for converting the discussion with @fmuellner into a pull request, @olivergs !
| if err != nil { | ||
| newerr = fmt.Errorf("failed to invoke command %s in container %s (Exit code: %d): %s", command[0], container, exitCode, err) | ||
| } else { | ||
| newerr = fmt.Errorf("failed to invoke command %s in container %s (Exit code: %d)", command[0], container, exitCode) |
There was a problem hiding this comment.
Wouldn't it be better if enter and/or run forwarded the exit code of the requested command? eg., toolbox run false would have an exit code of 1 and toolbox run true would have 0.
I suppose, most people use the exit code programmatically because they can already see the standard error and output streams as a human. In that case, not forwarding the exit code breaks the programmability of it.
But it's hard to say without knowing the exact use-case.
Part of the complication in forwarding the exit code is that Cobra, the Go framework we use for our command line handling, only lets us return an error. See Execute in src/cmd/root.go. So, I think we will need to provide our own custom error implementation that can encapsulate an exit code.
There was a problem hiding this comment.
Part of the complication in forwarding the exit code is that Cobra, the Go framework we use for our command line handling, only lets us return an error. See Execute in src/cmd/root.go. So, I think we will need to provide our own custom error implementation that can encapsulate an exit code.
We could simply stop relying on that part of Cobra and exit from directly from the commands.
There was a problem hiding this comment.
It's not good to have multiple exit points spread all over the code.
Having a single consolidated exit point means that we have a well-defined place to perform clean-ups, and put breakpoints or logs for debugging.
HarryMichal
added
7. Needs tests
When a command is executed with toolbox run and it returns a non-zero exit code, it is just ignored if that exit code is not handled. This prevents users to identify errors when executing commands in toolbox. With this fix, the exit codes of the invoked command are propagated and returned by 'toolbox run'. This includes even exit codes returned by Podman on error. containers#867
|
I propose an alternative implementation of this which fully propagates the return codes of the invoked commands (including Podman): https://github.com/HarryMichal/toolbox/tree/cmd/run/return-code-propagation |
When a command is executed with toolbox run and it returns a non-zero exit code, it is just ignored if that exit code is not handled. This prevents users to identify errors when executing commands in toolbox. With this fix, the exit codes of the invoked command are propagated and returned by 'toolbox run'. This includes even exit codes returned by Podman on error. containers#867
|
Created #1013 with the alternative implementation. |
|
Closing in favor of #1013 |
When a command is executed with toolbox run and it returns a non zero exit code, if that exit code is not handled it is just ignored.
This prevents users to identify errors when executing commands in toolbox.
With this fix, the unhandled exit codes are returned as errors to give the user a better error info.