summon no longer buffers wrapped process stdout output [CONJ-4705] (#64)

* wip

* Dont wrap 'Convey' calls with stdout wrapper, introduces unwanted chars

* Ensure that err is nil in TestRunSubcommand test

* Bump version and changelog in prep for 0.6.6 release

* fix error message and assertion

* defer restoring stdout to ensure it happens always

CHANGELOG.md

@@ -1,21 +1,24 @@
 # unreleased
 
-#v0.6.5
+# v0.6.6
+- stdout is no longer buffered inside summon. This should greatly decrease the memory footprint of long-running processes wrapped by summon. Closes [#63](https://github.com/cyberark/summon/issues/63).
+
+# v0.6.5
 * Minor release, no functionality changes
   - Improved Jenkins CI pipeline.
   - Binaries are now built for more distributions (see `PLATFORMS` in [build.sh](build.sh)).
   - Simpler docker-compose development environment.
 
-#v0.6.4
+# v0.6.4
 * Don't rely on executable bit on the provider; instead provide descriptive error if it fails to run - [Issue #40](https://github.com/cyberark/summon/issues/40)
 
-#v0.6.3
+# v0.6.3
 * Summon now passes the child exit status to the caller - [PR #39](https://github.com/cyberark/summon/pull/39)
 
-#v0.6.2
+# v0.6.2
 * Added 'default' section support, this is an alias for 'common' - [PR #37](https://github.com/cyberark/summon/pull/37)
 
-#v0.6.1
+# v0.6.1
 * Support Boolean literals - [PR #35](https://github.com/cyberark/summon/pull/35)
 
 # v0.6.0

command/action.go

@@ -2,14 +2,15 @@ package command
 
 import (
 	"fmt"
-	"github.com/codegangsta/cli"
-	prov "github.com/cyberark/summon/provider"
-	"github.com/cyberark/summon/secretsyml"
 	"os"
 	"os/exec"
 	"strings"
 	"sync"
 	"syscall"
+
+	"github.com/codegangsta/cli"
+	prov "github.com/cyberark/summon/provider"
+	"github.com/cyberark/summon/secretsyml"
 )
 
 type ActionConfig struct {
@@ -36,7 +37,7 @@ var Action = func(c *cli.Context) {
 		os.Exit(127)
 	}
 
-	out, err := runAction(&ActionConfig{
+	err = runAction(&ActionConfig{
 		Args:        c.Args(),
 		Provider:    provider,
 		Environment: c.String("environment"),
@@ -49,15 +50,15 @@ var Action = func(c *cli.Context) {
 	code, err := returnStatusOfError(err)
 
 	if err != nil {
-		fmt.Println(out + ": " + err.Error())
+		fmt.Println(err.Error())
 		os.Exit(127)
 	}
 
 	os.Exit(code)
 }
 
 // runAction encapsulates the logic of Action without cli Context for easier testing
-func runAction(ac *ActionConfig) (string, error) {
+func runAction(ac *ActionConfig) error {
 	var (
 		secrets secretsyml.SecretsMap
 		err     error
@@ -71,7 +72,7 @@ func runAction(ac *ActionConfig) (string, error) {
 	}
 
 	if err != nil {
-		return "", err
+		return err
 	}
 
 	var env []string
@@ -121,7 +122,7 @@ EnvLoop:
 					continue EnvLoop
 				}
 			}
-			return "Error fetching variable " + envvar.string, envvar.error
+			return fmt.Errorf("Error fetching variable %v: %v", envvar.string, envvar.error.Error())
 		}
 	}
 

command/action_test.go

@@ -1,58 +1,75 @@
 package command
 
 import (
-	"github.com/cyberark/summon/secretsyml"
-	. "github.com/smartystreets/goconvey/convey"
-	_ "golang.org/x/net/context"
+	"bytes"
+	"errors"
+	"io"
 	"io/ioutil"
 	"os"
 	"os/exec"
 	"path"
 	"strings"
 	"testing"
-	"errors"
+
+	"github.com/cyberark/summon/secretsyml"
+	. "github.com/smartystreets/goconvey/convey"
+	_ "golang.org/x/net/context"
 )
 
 func TestRunAction(t *testing.T) {
 	Convey("Using a dummy provider that returns 'mysecret'", t, func() {
 		providerPath := path.Join(os.Getenv("PWD"), "testprovider.sh")
 
 		Convey("Passing in secrets.yml via --yaml", func() {
-			out, err := runAction(&ActionConfig{
-				Args:       []string{"printenv", "MYVAR"},
-				Provider:   providerPath,
-				Filepath:   "",
-				YamlInline: "MYVAR: !var somesecret/on/a/path",
-				Subs:       map[string]string{},
-				Ignores:    []string{},
+			var err error
+
+			output := captureStdout(func() {
+				err = runAction(&ActionConfig{
+					Args:       []string{"printenv", "MYVAR"},
+					Provider:   providerPath,
+					Filepath:   "",
+					YamlInline: "MYVAR: !var somesecret/on/a/path",
+					Subs:       map[string]string{},
+					Ignores:    []string{},
+				})
+
 			})
-			So(out, ShouldEqual, "mysecret\n")
+
 			So(err, ShouldBeNil)
+			So(output, ShouldEqual, "mysecret\n")
 		})
 
 		Convey("Errors when fetching keys return error", func() {
-			_, err := runAction(&ActionConfig{
+			err := runAction(&ActionConfig{
 				Args:       []string{"printenv", "MYVAR"}, // args
 				Provider:   providerPath,                  // provider
 				Filepath:   "",                            // filepath
 				YamlInline: "MYVAR: !var error",           // yaml inline
 				Subs:       map[string]string{},           // subs
 				Ignores:    []string{},                    // ignore
 			})
+
 			So(err, ShouldNotBeNil)
+			So(err.Error(), ShouldEqual, "Error fetching variable MYVAR: exit status 1")
 		})
 
 		Convey("Errors when fetching keys don't return error if ignored", func() {
-			out, err := runAction(&ActionConfig{
-				Args:       []string{"printenv", "MYVAR"},
-				Provider:   providerPath,
-				Filepath:   "",
-				YamlInline: "{MYVAR: !var test, ERR: !var error}",
-				Subs:       map[string]string{},
-				Ignores:    []string{"ERR"},
+			var err error
+
+			output := captureStdout(func() {
+				err = runAction(&ActionConfig{
+					Args:       []string{"printenv", "MYVAR"},
+					Provider:   providerPath,
+					Filepath:   "",
+					YamlInline: "{MYVAR: !var test, ERR: !var error}",
+					Subs:       map[string]string{},
+					Ignores:    []string{"ERR"},
+				})
+
 			})
+
 			So(err, ShouldBeNil)
-			So(out, ShouldEqual, "mysecret\n")
+			So(output, ShouldEqual, "mysecret\n")
 		})
 	})
 }
@@ -77,9 +94,14 @@ func TestConvertSubsToMap(t *testing.T) {
 
 func TestRunSubcommand(t *testing.T) {
 	Convey("The subcommand should have access to secrets injected into its environment", t, func() {
+		var err error
+
 		args := []string{"printenv", "MYVAR"}
 		env := []string{"MYVAR=myvalue"}
-		output, err := runSubcommand(args, env)
+
+		output := captureStdout(func() {
+			err = runSubcommand(args, env)
+		})
 		expected := "myvalue\n"
 
 		So(output, ShouldEqual, expected)
@@ -161,3 +183,21 @@ func TestReturnStatusOfError(t *testing.T) {
 		So(err, ShouldEqual, expected)
 	})
 }
+
+func captureStdout(f func()) string {
+	old := os.Stdout
+	defer func() { // deferred to ensure that stdout is restored no matter what
+		os.Stdout = old
+	}()
+
+	r, w, _ := os.Pipe()
+	os.Stdout = w
+
+	f()
+
+	w.Close()
+
+	var buf bytes.Buffer
+	io.Copy(&buf, r)
+	return buf.String()
+}

command/subcommand.go

@@ -1,23 +1,18 @@
 package command
 
 import (
-	"bytes"
-	"io"
 	"os"
 	"os/exec"
 )
 
 // runSubcommand executes a command with arguments in the context
 // of an environment populated with secret values.
-func runSubcommand(command []string, env []string) (string, error) {
-	var stdOut bytes.Buffer
-
+func runSubcommand(command []string, env []string) error {
 	runner := exec.Command(command[0], command[1:]...)
 	runner.Stdin = os.Stdin
-	runner.Stdout = io.MultiWriter(os.Stdout, &stdOut)
+	runner.Stdout = os.Stdout
 	runner.Stderr = os.Stderr
 	runner.Env = env
 
-	err := runner.Run()
-	return stdOut.String(), err
+	return runner.Run()
 }

version.go

@@ -1,3 +1,3 @@
 package main
 
-const VERSION = "0.6.5"
+const VERSION = "0.6.6"