-
Notifications
You must be signed in to change notification settings - Fork 9.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
clean up remote.Cmd api #17609
clean up remote.Cmd api #17609
Conversation
Combine the ExitStatus and Err values from remote.Cmd into an error returned by Wait, better matching the behavior of the os/exec package. Non-zero exit codes are returned from Wait as a remote.ExitError. Communicator related errors are returned directly. Clean up all the error handling in the provisioners using a communicator. Also remove the extra copyOutput synchronization that was copied from package to package.
7767a49
to
3fbdee0
Compare
communicator/remote/command.go
Outdated
type ExitError int | ||
|
||
func (e ExitError) Error() string { | ||
return fmt.Sprintf("exit status: %d", e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the most common thing to do with this error is to re-package it in a more verbose message that includes the command name. Should we just make the error type include that itself already so that this error has a useful error message by default? (We could still recognize it and replace it with a more specific error in cases where there's something more interesting to say, of course.)
It looks like we can get to the command as c.Command
at the point where this error is generated, so should have everything we need to do it, at the expense of it needing to be a struct rather than just an integer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea. I was trying to keep it minimal, since users (though not in any cases here) often just want the integer exit code, and this matches the output of *exec.ExitError
. Though since nothing in the provisioners as actually checking the code, maybe we wrap it up with the command string and get rid of all the fmt.Errorf
s.
Since all use cases of ExitStatus are just putting it into fmt.Errorf, usually with the command string, have ExitStatus do that for the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
||
return c.exitStatus | ||
// ExitError is returned by Wait to indicate and error executing the remote |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExitError is returned by Wait to indicate an error executing the remote
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This reduces the surface area for the
remote.Cmd
API, as mentioned in #17596.Combine the
ExitStatus
andErr
values fromremote.Cmd
into an errorreturned by
Wait
, better matching the behavior of theos/exec
package.Non-zero exit codes are returned from Wait as a
remote.ExitError
.Communicator related errors are returned directly.
Clean up all the error handling in the provisioners using a
communicator. Also remove the extra copyOutput synchronization that was
copied from package to package.