Merge pull request #464 from runatlantis/fix-long-output

Fix issues with terraform execution.

Gopkg.lock

@@ -286,14 +286,6 @@
   pruneopts = "UT"
   revision = "b8bc1bf767474819792c23f32d8286a45736f1c6"
 
-[[projects]]
-  branch = "master"
-  digest = "1:2155cb970ad9dee55f4561a0a859a3f84fe20d7581409ed6e77b874a90709bfb"
-  name = "github.com/mitchellh/go-linereader"
-  packages = ["."]
-  pruneopts = "UT"
-  revision = "07bab5fdd9580500aea6ada0e09df4aa28e68abd"
-
 [[projects]]
   branch = "master"
   digest = "1:3d64942cc75c655215a64496e35a2ca1a30ee0007cd24f41b57631486f3d083c"
@@ -549,7 +541,6 @@
     "github.com/lkysow/go-gitlab",
     "github.com/mitchellh/colorstring",
     "github.com/mitchellh/go-homedir",
-    "github.com/mitchellh/go-linereader",
     "github.com/mohae/deepcopy",
     "github.com/nlopes/slack",
     "github.com/petergtz/pegomock",

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>
 

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

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

server/testfixtures/test-repos/simple/exp-output-apply-var.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 = overridden
 workspace = default
-```
+
+```
+</details>
 

server/testfixtures/test-repos/simple/exp-output-apply.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 = default
-```
+
+```
+</details>
 

server/testfixtures/test-repos/simple/exp-output-atlantis-plan-new-workspace.txt

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

server/testfixtures/test-repos/simple/exp-output-atlantis-plan-var-overridden.txt

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

server/testfixtures/test-repos/simple/exp-output-atlantis-plan.txt

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

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

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

server/testfixtures/test-repos/tfvars-yaml-no-autoplan/exp-output-apply-default.txt

@@ -19,6 +19,7 @@ Outputs:
 
 var = default
 workspace = default
+
 ```
 </details>
 

server/testfixtures/test-repos/tfvars-yaml-no-autoplan/exp-output-apply-staging.txt

@@ -19,6 +19,7 @@ Outputs:
 
 var = staging
 workspace = default
+
 ```
 </details>
 

server/testfixtures/test-repos/tfvars-yaml-no-autoplan/exp-output-plan-default.txt

@@ -11,6 +11,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/tfvars-yaml-no-autoplan/exp-output-plan-staging.txt

@@ -11,6 +11,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/tfvars-yaml/exp-output-apply-default.txt

@@ -19,6 +19,7 @@ Outputs:
 
 var = default
 workspace = default
+
 ```
 </details>
 

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

@@ -19,6 +19,7 @@ Outputs:
 
 var = staging
 workspace = default
+
 ```
 </details>
 

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

@@ -14,6 +14,7 @@ Terraform will perform the following actions:
 + null_resource.simple
       id: <computed>
 Plan: 1 to add, 0 to change, 0 to destroy.
+
 workspace=default
 
 ```
@@ -37,6 +38,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:

vendor/github.com/mitchellh/go-linereader/LICENSE.md

@@ -1,21 +0,0 @@
-The MIT License (MIT)
-
-Copyright (c) 2014 Mitchell Hashimoto
-
-Permission is hereby granted, free of charge, to any person obtaining a copy
-of this software and associated documentation files (the "Software"), to deal
-in the Software without restriction, including without limitation the rights
-to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
-copies of the Software, and to permit persons to whom the Software is
-furnished to do so, subject to the following conditions:
-
-The above copyright notice and this permission notice shall be included in
-all copies or substantial portions of the Software.
-
-THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
-THE SOFTWARE.

vendor/github.com/mitchellh/go-linereader/README.md

@@ -1,30 +0,0 @@
-# go-linereader
-
-`go-linereader` (Golang package: `linereader`) is a package for Go that
-breaks up the input from an io.Reader into multiple lines. It is
-a lot like `bufio.Scanner`, except you can specify timeouts that will push
-"lines" through after a certain amount of time. This lets you read lines,
-but return any data if a line isn't updated for some time.
-
-## Installation and Usage
-
-Install using `go get github.com/mitchellh/go-linereader`.
-
-Full documentation is available at
-http://godoc.org/github.com/mitchellh/go-linereader
-
-Below is an example of its usage ignoring errors:
-
-```go
-// Assume r is some set io.Reader. Perhaps a file, network, anything.
-var r io.Reader
-
-// Initialize the line reader
-lr := linereader.New(r)
-
-// Get all the lines
-for line := <-lr.Ch {
-	// Do something with the line. This line will have the line separator
-	// removed.
-}
-```

vendor/github.com/mitchellh/go-linereader/linereader.go

@@ -1,118 +0,0 @@
-package linereader
-
-import (
-	"io"
-	"bufio"
-	"sync/atomic"
-	"time"
-)
-
-// Reader takes an io.Reader and pushes the lines out onto the channel.
-type Reader struct {
-	Reader  io.Reader
-	Timeout time.Duration
-
-	// Ch is the output channel. This will be closed when there are no
-	// more lines (io.EOF).
-	Ch chan string
-
-	started uint32
-}
-
-// New creates a new Reader that reads lines from the io.Reader.
-//
-// The Reader is already started when returned, so it is unsafe to modify
-// any struct fields.
-func New(r io.Reader) *Reader {
-	result := &Reader{
-		Reader:  r,
-		Timeout: 100 * time.Millisecond,
-		Ch:      make(chan string),
-	}
-
-	go result.Run()
-	return result
-}
-
-// Run reads from the Reader and dispatches lines on the Ch channel.
-//
-// This blocks and is usually called with `go` prefixed to dispatch onto
-// a goroutine. It is safe to call this function multiple times; subsequent
-// calls to Run will exit without running.
-func (r *Reader) Run() {
-	if !atomic.CompareAndSwapUint32(&r.started, 0, 1) {
-		return
-	}
-
-	// When we're done, close the channel
-	defer close(r.Ch)
-
-	// Listen for bytes in a goroutine. We do this so that if we're blocking
-	// we can flush the bytes we have after some configured time. There is
-	// probably a way to make this a lot faster but this works for now.
-	//
-	// NOTE: This isn't particularly performant. I'm sure there is a better
-	// way to do this instead of sending single bytes on a channel, but it
-	// works fine.
-	buf := bufio.NewReader(r.Reader)
-	byteCh := make(chan byte)
-	doneCh := make(chan error)
-	go func() {
-		defer close(doneCh)
-		for {
-			b, err := buf.ReadByte()
-			if err != nil {
-				doneCh <- err
-				return
-			}
-
-			byteCh <- b
-		}
-	}()
-
-	lineBuf := make([]byte, 0, 80)
-	for {
-		var err error
-		line := lineBuf[0:0]
-		for {
-			brk := false
-
-			select {
-			case b := <-byteCh:
-				brk = b == '\n'
-				if !brk {
-					line = append(line, b)
-				}
-			case err = <-doneCh:
-				brk = true
-			case <-time.After(r.Timeout):
-				if len(line) > 0 {
-					brk = true
-				}
-			}
-
-			if brk {
-				break
-			}
-		}
-
-		// If an error occurred and its not an EOF, then report that
-		// error to all pipes and exit.
-		if err != nil && err != io.EOF {
-			break
-		}
-
-		// If we're at the end and the line is empty, then return.
-		if err == io.EOF && len(line) == 0 {
-			break
-		}
-
-		// Write out the line
-		r.Ch <- string(line)
-
-		// If we hit the end, we're done
-		if err == io.EOF {
-			break
-		}
-	}
-}