Fix issues with terraform execution.

Makefile

@@ -60,10 +60,10 @@ release: ## Create packages for a release
 fmt: ## Run goimports (which also formats)
 	goimports -w $$(find . -type f -name '*.go' ! -path "./vendor/*" ! -path "./server/static/bindata_assetfs.go" ! -path "**/mocks/*")
 
-lint: ## Run linter
+lint: ## Run linter locally
 	golangci-lint run
 
-check-lint:
+check-lint: ## Run linter in CI/CD. If running locally use 'lint'
 	curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b ./bin v1.13.2
 	./bin/golangci-lint run
 

server/events/terraform/terraform_client.go

@@ -23,8 +23,6 @@ import (
 	"regexp"
 	"strings"
 
-	"github.com/mitchellh/go-linereader"
-
 	"github.com/hashicorp/go-version"
 	"github.com/mitchellh/go-homedir"
 	"github.com/pkg/errors"
@@ -162,69 +160,17 @@ func (c *DefaultClient) RunCommandWithVersion(log *logging.SimpleLogger, path st
 
 	// append terraform executable name with args
 	tfCmd := fmt.Sprintf("%s %s", tfExecutable, strings.Join(args, " "))
-	out, err := c.crashSafeExec(tfCmd, path, envVars)
+	cmd := exec.Command("sh", "-c", tfCmd)
+	cmd.Dir = path
+	cmd.Env = envVars
+	out, err := cmd.CombinedOutput()
 	if err != nil {
 		err = fmt.Errorf("%s: running %q in %q", err, tfCmd, path)
 		log.Debug("error: %s", err)
-		return out, err
+		return string(out), err
 	}
 	log.Info("successfully ran %q in %q", tfCmd, path)
-	return out, err
-}
-
-// crashSafeExec executes tfCmd in dir with the env environment variables. It
-// returns any stderr and stdout output from the command as a combined string.
-// It is "crash safe" in that it handles an edge case related to:
-//    https://github.com/golang/go/issues/18874
-// where when terraform itself panics, it leaves file descriptors open which
-// cause golang to not know the process has terminated.
-// To handle this, we borrow code from
-//    https://github.com/hashicorp/terraform/blob/master/builtin/provisioners/local-exec/resource_provisioner.go#L92
-// and use an os.Pipe to collect the stderr and stdout. This allows golang to
-// know the command has exited and so the call to cmd.Wait() won't block
-// indefinitely.
-//
-// Unfortunately, this causes another issue where we never receive an EOF to
-// our pipe during a terraform panic and so again, we're left waiting
-// indefinitely. To handle this, I've hacked in detection of Terraform panic
-// output as a special case that causes us to exit the loop.
-func (c *DefaultClient) crashSafeExec(tfCmd string, dir string, env []string) (string, error) {
-	pr, pw, err := os.Pipe()
-	if err != nil {
-		return "", errors.Wrap(err, "failed to initialize pipe for output")
-	}
-
-	// We use 'sh -c' so that if extra_args have been specified with env vars,
-	// ex. -var-file=$WORKSPACE.tfvars, then they get substituted.
-	cmd := exec.Command("sh", "-c", tfCmd) // #nosec
-	cmd.Stdout = pw
-	cmd.Stderr = pw
-	cmd.Dir = dir
-	cmd.Env = env
-
-	err = cmd.Start()
-	if err == nil {
-		err = cmd.Wait()
-	}
-	pw.Close() // nolint: errcheck
-
-	lr := linereader.New(pr)
-	var outputLines []string
-	for line := range lr.Ch {
-		outputLines = append(outputLines, line)
-		// This checks if our output is a Terraform panic. If so, we break
-		// out of the loop because in this case, for some reason to do with
-		// terraform forking itself, we never receive an EOF and
-		// so this will block indefinitely.
-		if len(outputLines) >= 3 &&
-			strings.Join(
-				outputLines[len(outputLines)-3:], "\n") ==
-				tfCrashDelim {
-			break
-		}
-	}
-
-	return strings.Join(outputLines, "\n"), err
+	return string(out), nil
 }
 
 // MustConstraint will parse one or more constraints from the given
@@ -244,8 +190,3 @@ func MustConstraint(v string) version.Constraints {
 var rcFileContents = `credentials "app.terraform.io" {
   token = %q
 }`
-
-// tfCrashDelim is what the end of a terraform crash log looks like.
-var tfCrashDelim = `[1]: https://github.com/hashicorp/terraform/issues
-
-!!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!!`

server/events/terraform/terraform_client_internal_test.go

@@ -79,41 +79,3 @@ func TestGenerateRCFile_ErrIfCannotWrite(t *testing.T) {
 	actErr := generateRCFile("token", "/this/dir/does/not/exist")
 	ErrEquals(t, expErr, actErr)
 }
-
-// I couldn't find an easy way to test the edge case that this function exists
-// for (where terraform panics) so I'm just testing that it executes a normal
-// process as expected.
-func TestCrashSafeExec(t *testing.T) {
-	cases := []struct {
-		cmd    string
-		expErr string
-		expOut string
-	}{
-		{
-			"echo hi",
-			"",
-			"hi",
-		},
-		{
-			"echo yo && exit 1",
-			"exit status 1",
-			"yo",
-		},
-	}
-
-	client := DefaultClient{}
-	for _, c := range cases {
-		t.Run(c.cmd, func(t *testing.T) {
-			tmp, cleanup := TempDir(t)
-			defer cleanup()
-			out, err := client.crashSafeExec(c.cmd, tmp, nil)
-			if c.expErr != "" {
-				ErrEquals(t, c.expErr, err)
-				Equals(t, c.expOut, out)
-			} else {
-				Ok(t, err)
-				Equals(t, c.expOut, out)
-			}
-		})
-	}
-}

server/testfixtures/test-repos/automerge/exp-output-apply-dir1.txt

@@ -5,5 +5,6 @@ null_resource.automerge: Creating...
 null_resource.automerge: Creation complete after *s (ID: ******************)
 
 Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
+
 ```
 

server/testfixtures/test-repos/automerge/exp-output-apply-dir2.txt

@@ -5,5 +5,6 @@ null_resource.automerge: Creating...
 null_resource.automerge: Creation complete after *s (ID: ******************)
 
 Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
+
 ```
 

server/testfixtures/test-repos/automerge/exp-output-autoplan.txt

@@ -14,6 +14,7 @@ Terraform will perform the following actions:
 + null_resource.automerge
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
+
 ```
 
 * :arrow_forward: To **apply** this plan, comment:
@@ -35,6 +36,7 @@ Terraform will perform the following actions:
 + null_resource.automerge
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
+
 ```
 
 * :arrow_forward: To **apply** this plan, comment:

server/testfixtures/test-repos/modules-yaml/exp-output-apply-production.txt

@@ -9,5 +9,6 @@ Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
 Outputs:
 
 var = production
+
 ```
 

server/testfixtures/test-repos/modules-yaml/exp-output-apply-staging.txt

@@ -9,5 +9,6 @@ Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
 Outputs:
 
 var = staging
+
 ```
 

server/testfixtures/test-repos/modules-yaml/exp-output-autoplan.txt

@@ -14,6 +14,7 @@ Terraform will perform the following actions:
 + module.null.null_resource.this
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
+
 ```
 
 * :arrow_forward: To **apply** this plan, comment:
@@ -35,6 +36,7 @@ Terraform will perform the following actions:
 + module.null.null_resource.this
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
+
 ```
 
 * :arrow_forward: To **apply** this plan, comment:

server/testfixtures/test-repos/modules/exp-output-apply-production.txt

@@ -9,5 +9,6 @@ Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
 Outputs:
 
 var = production
+
 ```
 

server/testfixtures/test-repos/modules/exp-output-apply-staging.txt

@@ -9,5 +9,6 @@ Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
 Outputs:
 
 var = staging
+
 ```
 

server/testfixtures/test-repos/modules/exp-output-autoplan-only-staging.txt

@@ -11,6 +11,7 @@ Terraform will perform the following actions:
 + module.null.null_resource.this
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
+
 ```
 
 * :arrow_forward: To **apply** this plan, comment:

server/testfixtures/test-repos/modules/exp-output-plan-production.txt

@@ -11,6 +11,7 @@ Terraform will perform the following actions:
 + module.null.null_resource.this
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
+
 ```
 
 * :arrow_forward: To **apply** this plan, comment:

server/testfixtures/test-repos/modules/exp-output-plan-staging.txt

@@ -11,6 +11,7 @@ Terraform will perform the following actions:
 + module.null.null_resource.this
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
+
 ```
 
 * :arrow_forward: To **apply** this plan, comment:

server/testfixtures/test-repos/simple-yaml/exp-output-apply-all.txt

@@ -13,10 +13,13 @@ Outputs:
 
 var = fromconfig
 workspace = default
+
 ```
 
 ---
 ### 2. dir: `.` workspace: `staging`
+<details><summary>Show Output</summary>
+
 ```diff
 preapply
 
@@ -29,9 +32,11 @@ Outputs:
 
 var = fromfile
 workspace = staging
+
 postapply
 
 ```
+</details>
 
 ---
 

server/testfixtures/test-repos/simple-yaml/exp-output-apply-default.txt

@@ -10,5 +10,6 @@ Outputs:
 
 var = fromconfig
 workspace = default
+
 ```
 

server/testfixtures/test-repos/simple-yaml/exp-output-apply-staging.txt

@@ -1,5 +1,7 @@
 Ran Apply for dir: `.` workspace: `staging`
 
+<details><summary>Show Output</summary>
+
 ```diff
 preapply
 
@@ -12,7 +14,9 @@ Outputs:
 
 var = fromfile
 workspace = staging
+
 postapply
 
 ```
+</details>
 

server/testfixtures/test-repos/simple-yaml/exp-output-autoplan.txt

@@ -18,6 +18,7 @@ Terraform will perform the following actions:
 + null_resource.simple
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
+
 postplan
 
 ```
@@ -42,6 +43,7 @@ Terraform will perform the following actions:
 + null_resource.simple
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
+
 ```
 
 * :arrow_forward: To **apply** this plan, comment:

server/testfixtures/test-repos/simple/exp-output-apply-var-all.txt

@@ -3,6 +3,8 @@ Ran Apply for 2 projects:
 1. dir: `.` workspace: `new_workspace`
 
 ### 1. dir: `.` workspace: `default`
+<details><summary>Show Output</summary>
+
 ```diff
 null_resource.simple:
 null_resource.simple:
@@ -17,10 +19,14 @@ Outputs:
 
 var = default_workspace
 workspace = default
+
 ```
+</details>
 
 ---
 ### 2. dir: `.` workspace: `new_workspace`
+<details><summary>Show Output</summary>
+
 ```diff
 null_resource.simple:
 null_resource.simple:
@@ -35,7 +41,9 @@ Outputs:
 
 var = new_workspace
 workspace = new_workspace
+
 ```
+</details>
 
 ---
 

server/testfixtures/test-repos/simple/exp-output-apply-var-default-workspace.txt

@@ -1,5 +1,7 @@
 Ran Apply for dir: `.` workspace: `default`
 
+<details><summary>Show Output</summary>
+
 ```diff
 null_resource.simple:
 null_resource.simple:
@@ -14,5 +16,7 @@ Outputs:
 
 var = default_workspace
 workspace = default
+
 ```
+</details>