Closed lawrencegripper closed 4 years ago
Yes that sounds like a good feature to add. Better error handling is always nice. I didnt add this before because errors happening arent necessarily a problem, but you are right that if the script exists with a nonzero condition then its probably worth throwing a hard error at that point.
Another thing that would be welcome is more and better tests. Could you also add at least one test for this new feature?
Looked pretty easy to get done so I've submitted a PR, didn't need to get the exact exit code as turns out the exec
package returns an error on non-zero codes.
Got some output back to the user too for debugging without TF_LOG=debug
Added a test to validate that an error is returned correctly too :)
Hi,
Would you be happy for me to put together a PR to track the exit codes coming back from the scripts?
The use case here is to prevent additional calls to scripts when a failure occurs. Currently I think what will happen (based on what I've seen) is that if
read
will exit with non-zero code and not output anystate
(say a network issue has prevented a curl call) the provider does a diff with the current state and observes a change, to rectify this it callsupdate
. My expectation would be to have the error cause theterraform apply
to terminate instead and let the user review the error then re-run as needed.This change would mean that scripts should always return a 0 exit code if they want their state output to be persisted or a non-zero code if they want to terminate and display the error.
An added bonus here is that we'd be able to take the cmd output from the error'd script and return it to Terraform so it'd displayed to the user without setting
TF_LOG=debug
(would only happen when the script error'd but still useful).I think I need to change this section:
https://github.com/scottwinkler/terraform-provider-shell/blob/a605c7b5edf970a7428c2f832686355782a7893e/shell/utility.go#L100-L108
To use a pattern like this:
https://stackoverflow.com/a/10385867/3437018
Sound like a useful change?