Fix driver return code handling#113
Conversation
When a box is used in a template, but isn't pusblished on vagrant cloud, trying to get it will fail. This is not necessarily a terminal condition, as the box may already exist locally, which is what the `skip_add' attribute is for. However, when the step_add_box step fails, no mention of the attribute was made, so we amend the error to suggest using it if it is already installed locally.
When executing a Vagrant command, the driver would never error, unless the call to `Start` actually errored. This is problematic, as when there's a problem with a Vagrant command, it doesn't get forwarded to the caller, and no error might be reported. This problem stems from a modification in commit b0c799c, which changed how the command was invoked, as previously it used `Run()` (a combination of Start and Wait), and now it split both calls in two so the outputs of the command can be processed before sending them back. However, that change did not account for errors in `Wait`, which is where the process actually returns an error if it ends with a non-zero error code, making all commands from `vagrant` not report their errors to the driver.
JenGoldstrich
left a comment
There was a problem hiding this comment.
Looks good to me Lucas, I left a nit and a small question
| Provider string `mapstructure:"provider" required:"false"` | ||
| // Options for the "vagrant init" command | ||
|
|
||
| // What vagrantfile to use |
There was a problem hiding this comment.
Nit: we should mention this in the PR name for the git history of removing this field
There was a problem hiding this comment.
In general we don't squash/merge, this is already in a separate commit, so I don't think this is an issue
| err = cmd.Wait() | ||
|
|
||
| if _, ok := err.(*exec.ExitError); ok { | ||
| err = fmt.Errorf("Vagrant error: %s", stderrString) |
There was a problem hiding this comment.
Does the Wait command always error when the Start command errors? If not we need to check the start command for ExitErrors too?
There was a problem hiding this comment.
Start errors if the command fails to be started, and we already check/exit if it does. The type of error doesn't matter here, and I would think this won't be an ExitError in any case, since the process won't even be created.
The problem here is if the command starts, and errors for any reason in the meantime, we didn't capture it, so even when vagrant returned non-zero, it would never report it.
TBF I'm not certain that checking the error type is relevant at all after Wait(), but I haven't dug into the complete history of the file, so it's hard to say, maybe there was a rationale for that type check. In the end for this commit I opted not to change this, but we can remove that check later if we don't think it impacts valid command executions.
When executing a Vagrant command, the driver would never error, unless the call to
Startactually errored.This is problematic, as when there's a problem with a Vagrant command, it doesn't get forwarded to the caller, and no error might be reported.
This problem stems from a modification in commit b0c799c, which changed how the command was invoked, as previously it used
Run()(a combination of Start and Wait), and now it split both calls in two so the outputs of the command can be processed before sending them back.However, that change did not account for errors in
Wait, which is where the process actually returns an error if it ends with a non-zero error code, making all commands fromvagrantnot report their errors to the driver.Related to: #110