Merge pull request #534 from dgageot/improve-commands

Improve commands output

integration/run_test.go

@@ -50,8 +50,8 @@ func TestMain(m *testing.M) {
 	flag.Parse()
 	if *remote {
 		cmd := exec.Command("gcloud", "container", "clusters", "get-credentials", *gkeClusterName, "--zone", *gkeZone, "--project", *gcpProject)
-		if stdout, stderr, err := util.RunCommand(cmd, nil); err != nil {
-			logrus.Fatalf("Error authenticating to GKE cluster stdout: %s, stderr: %s, err: %s", stdout, stderr, err)
+		if err := util.RunCmd(cmd); err != nil {
+			logrus.Fatalf("Error authenticating to GKE cluster stdout: %v", err)
 		}
 	}
 
@@ -179,9 +179,9 @@ func TestRun(t *testing.T) {
 			}
 			cmd.Env = env
 			cmd.Dir = testCase.dir
-			out, outerr, err := util.RunCommand(cmd, nil)
+			err := util.RunCmd(cmd)
 			if err != nil {
-				t.Fatalf("skaffold run: \nstdout: %s\nstderr: %s\nerror: %s", out, outerr, err)
+				t.Fatalf("skaffold run: %v", err)
 			}
 
 			for _, p := range testCase.pods {
@@ -212,9 +212,9 @@ func setupNamespace(t *testing.T) (*v1.Namespace, func()) {
 	}
 
 	kubectlCmd := exec.Command("kubectl", "config", "set-context", context.Cluster, "--namespace", ns.Name)
-	out, outerr, err := util.RunCommand(kubectlCmd, nil)
+	err := util.RunCmd(kubectlCmd)
 	if err != nil {
-		t.Fatalf("kubectl config set-context --namespace: %s\nstderr: %s\nerror: %s", out, outerr, err)
+		t.Fatalf("kubectl config set-context --namespace: %v", err)
 	}
 
 	return ns, func() { client.CoreV1().Namespaces().Delete(ns.Name, &meta_v1.DeleteOptions{}); return }
@@ -225,14 +225,16 @@ func TestFix(t *testing.T) {
 
 	fixCmd := exec.Command("skaffold", "fix", "-f", "skaffold.yaml")
 	fixCmd.Dir = "testdata/old-config"
-	out, _, err := util.RunCommand(fixCmd, nil)
+	out, err := util.RunCmdOut(fixCmd)
 	if err != nil {
-		t.Fatalf("testing error: %s", err.Error())
+		t.Fatalf("testing error: %v", err)
 	}
+
 	runCmd := exec.Command("skaffold", "run", "-f", "-")
 	runCmd.Dir = "testdata/old-config"
-	_, _, err = util.RunCommand(runCmd, bytes.NewReader(out))
+	runCmd.Stdin = bytes.NewReader(out)
+	err = util.RunCmd(runCmd)
 	if err != nil {
-		t.Fatalf("testing error: %s", err.Error())
+		t.Fatalf("testing error: %v", err)
 	}
 }

pkg/skaffold/bazel/bazel.go

@@ -34,9 +34,9 @@ const sourceQuery = "kind('source file', deps('%s'))"
 func (*BazelDependencyResolver) GetDependencies(a *v1alpha2.Artifact) ([]string, error) {
 	cmd := exec.Command("bazel", "query", fmt.Sprintf(sourceQuery, a.BazelArtifact.BuildTarget), "--noimplicit_deps", "--order_output=no")
 	cmd.Dir = a.Workspace
-	stdout, stderr, err := util.RunCommand(cmd, nil)
+	stdout, err := util.RunCmdOut(cmd)
 	if err != nil {
-		return nil, errors.Wrapf(err, "stdout: %s stderr: %s", stdout, stderr)
+		return nil, errors.Wrap(err, "getting bazel dependencies")
 	}
 
 	labels := strings.Split(string(stdout), "\n")

pkg/skaffold/deploy/helm.go

@@ -74,11 +74,11 @@ func (h *HelmDeployer) args(moreArgs ...string) []string {
 func (h *HelmDeployer) deployRelease(out io.Writer, r v1alpha2.HelmRelease, b *build.BuildResult) error {
 	isInstalled := true
 	getCmd := exec.Command("helm", h.args("get", r.Name)...)
-	if stdout, stderr, err := util.RunCommand(getCmd, nil); err != nil {
-		logrus.Debugf("Error getting release %s: %s stdout: %s stderr: %s", r.Name, err, string(stdout), string(stderr))
+	if _, err := util.RunCmdOut(getCmd); err != nil {
 		fmt.Fprintf(out, "Helm release %s not installed. Installing...\n", r.Name)
 		isInstalled = false
 	}
+
 	params, err := JoinTagsToBuildResult(b.Builds, r.Values)
 	if err != nil {
 		return errors.Wrap(err, "matching build results to chart values")
@@ -93,11 +93,9 @@ func (h *HelmDeployer) deployRelease(out io.Writer, r v1alpha2.HelmRelease, b *b
 	// First build dependencies.
 	logrus.Infof("Building helm dependencies...")
 	depCmd := exec.Command("helm", h.args("dep", "build", r.ChartPath)...)
-	stdout, stderr, err := util.RunCommand(depCmd, nil)
-	if err != nil {
-		return errors.Wrapf(err, "helm dep build stdout: %s, stderr: %s", string(stdout), string(stderr))
+	if err := util.RunCmd(depCmd); err != nil {
+		return errors.Wrap(err, "building helm dependencies")
 	}
-	out.Write(stdout)
 
 	args := h.args()
 	if !isInstalled {
@@ -125,22 +123,16 @@ func (h *HelmDeployer) deployRelease(out io.Writer, r v1alpha2.HelmRelease, b *b
 	args = append(args, setOpts...)
 
 	execCmd := exec.Command("helm", args...)
-	stdout, stderr, err = util.RunCommand(execCmd, nil)
-	if err != nil {
-		return errors.Wrapf(err, "helm updater stdout: %s, stderr: %s", string(stdout), string(stderr))
-	}
-
-	out.Write(stdout)
-	return nil
+	return util.RunCmd(execCmd)
 }
 
 func (h *HelmDeployer) deleteRelease(out io.Writer, r v1alpha2.HelmRelease) error {
-	getCmd := exec.Command("helm", h.args("delete", r.Name, "--purge")...)
-	stdout, stderr, err := util.RunCommand(getCmd, nil)
+	deleteCmd := exec.Command("helm", h.args("delete", r.Name, "--purge")...)
+
+	err := util.RunCmd(deleteCmd)
 	if err != nil {
-		logrus.Debugf("running helm delete %s: %v stdout: %s stderr: %s", r.Name, err, string(stdout), string(stderr))
+		logrus.Debugf("deleting release %s: %v\n", r.Name, err)
 	}
 
-	out.Write(stdout)
 	return nil
 }

pkg/skaffold/deploy/helm_test.go

@@ -20,7 +20,6 @@ import (
 	"bytes"
 	"context"
 	"fmt"
-	"io"
 	"os/exec"
 	"testing"
 
@@ -100,8 +99,8 @@ func TestHelmDeploy(t *testing.T) {
 			description: "get failure should install not upgrade",
 			cmd: &MockHelm{
 				t:             t,
-				getResult:     cmdOutput{"", "", fmt.Errorf("not found")},
-				upgradeResult: cmdOutput{"", "", fmt.Errorf("should not have called upgrade")},
+				getResult:     cmdOutput{"", fmt.Errorf("not found")},
+				upgradeResult: cmdOutput{"", fmt.Errorf("should not have called upgrade")},
 			},
 			deployer:    NewHelmDeployer(testDeployConfig, testKubeContext),
 			buildResult: testBuildResult,
@@ -110,7 +109,7 @@ func TestHelmDeploy(t *testing.T) {
 			description: "get success should upgrade not install",
 			cmd: &MockHelm{
 				t:             t,
-				installResult: cmdOutput{"", "", fmt.Errorf("should not have called install")},
+				installResult: cmdOutput{"", fmt.Errorf("should not have called install")},
 			},
 			deployer:    NewHelmDeployer(testDeployConfig, testKubeContext),
 			buildResult: testBuildResult,
@@ -119,7 +118,7 @@ func TestHelmDeploy(t *testing.T) {
 			description: "deploy error",
 			cmd: &MockHelm{
 				t:             t,
-				upgradeResult: cmdOutput{"", "", fmt.Errorf("unexpected error")},
+				upgradeResult: cmdOutput{"", fmt.Errorf("unexpected error")},
 			},
 			shouldErr:   true,
 			deployer:    NewHelmDeployer(testDeployConfig, testKubeContext),
@@ -129,7 +128,7 @@ func TestHelmDeploy(t *testing.T) {
 			description: "dep build error",
 			cmd: &MockHelm{
 				t:         t,
-				depResult: cmdOutput{"", "", fmt.Errorf("unexpected error")},
+				depResult: cmdOutput{"", fmt.Errorf("unexpected error")},
 			},
 			shouldErr:   true,
 			deployer:    NewHelmDeployer(testDeployConfig, testKubeContext),
@@ -160,15 +159,14 @@ type MockHelm struct {
 
 type cmdOutput struct {
 	stdout string
-	stderr string
 	err    error
 }
 
-func (c cmdOutput) out() ([]byte, []byte, error) {
-	return []byte(c.stdout), []byte(c.stderr), c.err
+func (c cmdOutput) out() ([]byte, error) {
+	return []byte(c.stdout), c.err
 }
 
-func (m *MockHelm) RunCommand(c *exec.Cmd, _ io.Reader) ([]byte, []byte, error) {
+func (m *MockHelm) RunCmdOut(c *exec.Cmd) ([]byte, error) {
 	if len(c.Args) < 3 {
 		m.t.Errorf("Not enough args in command %v", c)
 	}
@@ -189,5 +187,10 @@ func (m *MockHelm) RunCommand(c *exec.Cmd, _ io.Reader) ([]byte, []byte, error)
 	}
 
 	m.t.Errorf("Unknown helm command: %+v", c)
-	return nil, nil, nil
+	return nil, nil
+}
+
+func (m *MockHelm) RunCmd(c *exec.Cmd) error {
+	_, err := m.RunCmdOut(c)
+	return err
 }

pkg/skaffold/deploy/kubectl.go

@@ -149,13 +149,10 @@ func (k *KubectlDeployer) kubectl(in io.Reader, out io.Writer, arg ...string) er
 	args := append([]string{"--context", k.kubeContext}, arg...)
 
 	cmd := exec.Command("kubectl", args...)
-	stdout, stderr, err := util.RunCommand(cmd, in)
-	if err != nil {
-		return errors.Wrapf(err, "running kubectl: stdout: %s, stderr: %s, err: %v", stdout, stderr, err)
-	}
+	cmd.Stdin = in
+	cmd.Stdout = out
 
-	out.Write(stdout)
-	return nil
+	return util.RunCmd(cmd)
 }
 
 func manifestFiles(manifests []string) ([]string, error) {

pkg/skaffold/deploy/kubectl_test.go

@@ -111,7 +111,7 @@ func TestKubectlDeploy(t *testing.T) {
 					},
 				},
 			},
-			command: testutil.NewFakeRunCommand("kubectl --context kubecontext apply -f -", "", "", nil),
+			command: testutil.NewFakeCmd("kubectl --context kubecontext apply -f -", nil),
 			b: &build.BuildResult{
 				Builds: []build.Build{
 					{
@@ -132,7 +132,7 @@ func TestKubectlDeploy(t *testing.T) {
 					},
 				},
 			},
-			command: testutil.NewFakeRunCommand("kubectl --context kubecontext apply -f -", "", "", fmt.Errorf("")),
+			command: testutil.NewFakeCmd("kubectl --context kubecontext apply -f -", fmt.Errorf("")),
 			b: &build.BuildResult{
 				Builds: []build.Build{
 					{
@@ -181,7 +181,7 @@ func TestKubectlCleanup(t *testing.T) {
 					},
 				},
 			},
-			command: testutil.NewFakeRunCommand("kubectl --context kubecontext delete -f -", "", "", nil),
+			command: testutil.NewFakeCmd("kubectl --context kubecontext delete -f -", nil),
 		},
 		{
 			description: "cleanup error",
@@ -192,7 +192,7 @@ func TestKubectlCleanup(t *testing.T) {
 					},
 				},
 			},
-			command:   testutil.NewFakeRunCommand("kubectl --context kubecontext delete -f -", "", "", errors.New("BUG")),
+			command:   testutil.NewFakeCmd("kubectl --context kubecontext delete -f -", errors.New("BUG")),
 			shouldErr: true,
 		},
 	}
@@ -220,11 +220,11 @@ func TestKubectlCleanup(t *testing.T) {
 
 func TestReplaceImages(t *testing.T) {
 	var tests = []struct {
-		description       string
-		builds            []build.Build
-		manifests         manifestList
-		expectedManifests manifestList
-		shouldErr         bool
+		description string
+		builds      []build.Build
+		manifests   manifestList
+		expected    manifestList
+		shouldErr   bool
 	}{
 		{
 			description: "pod",
@@ -240,7 +240,7 @@ spec:
   containers:
   - image: gcr.io/k8s-skaffold/skaffold-example
     name: getting-started`)},
-			expectedManifests: manifestList{[]byte(`apiVersion: v1
+			expected: manifestList{[]byte(`apiVersion: v1
 kind: Pod
 metadata:
   name: getting-started
@@ -250,9 +250,9 @@ spec:
     name: getting-started`)},
 		},
 		{
-			description:       "empty",
-			manifests:         manifestList{[]byte(""), []byte("  ")},
-			expectedManifests: manifestList{},
+			description: "empty",
+			manifests:   manifestList{[]byte(""), []byte("  ")},
+			expected:    manifestList{},
 		},
 		{
 			description: "ignore tag",
@@ -268,7 +268,7 @@ spec:
   containers:
   - image: gcr.io/k8s-skaffold/skaffold-example:IGNORED
     name: getting-started`)},
-			expectedManifests: manifestList{[]byte(`apiVersion: v1
+			expected: manifestList{[]byte(`apiVersion: v1
 kind: Pod
 metadata:
   name: getting-started
@@ -313,7 +313,7 @@ spec:
   - port: 50051
   selector:
     app: leeroy-app`)},
-			expectedManifests: manifestList{
+			expected: manifestList{
 				[]byte(`apiVersion: apps/v1beta2
 kind: Deployment
 metadata:
@@ -352,7 +352,7 @@ spec:
 
 			resultManifest, err := manifests.replaceImages(test.builds)
 
-			testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expectedManifests.String(), resultManifest.String())
+			testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected.String(), resultManifest.String())
 		})
 	}
 }

pkg/skaffold/docker/client.go

@@ -127,10 +127,11 @@ func newMinikubeDockerAPIClient() (DockerAPIClient, error) {
 
 func getMinikubeDockerEnv() (map[string]string, error) {
 	cmd := exec.Command("minikube", "docker-env", "--shell", "none")
-	out, stderr, err := util.RunCommand(cmd, nil)
+	out, err := util.RunCmdOut(cmd)
 	if err != nil {
-		return nil, errors.Wrapf(err, "getting minikube docker-env stdout: %s, stdin: %s, err: %s", out, stderr, err)
+		return nil, errors.Wrap(err, "getting minikube env")
 	}
+
 	env := map[string]string{}
 	for _, line := range strings.Split(string(out), "\n") {
 		if line == "" {

pkg/skaffold/docker/client_test.go

@@ -66,45 +66,45 @@ func TestNewMinikubeImageAPIClient(t *testing.T) {
 	}{
 		{
 			description: "correct client",
-			cmd: testutil.NewFakeRunCommand("minikube docker-env --shell none", `DOCKER_TLS_VERIFY=1
+			cmd: testutil.NewFakeCmdOut("minikube docker-env --shell none", `DOCKER_TLS_VERIFY=1
 DOCKER_HOST=http://127.0.0.1:8080
 DOCKER_CERT_PATH=testdata
-DOCKER_API_VERSION=1.23`, "", nil),
+DOCKER_API_VERSION=1.23`, nil),
 		},
 		{
 			description: "correct client",
-			cmd: testutil.NewFakeRunCommand("minikube docker-env --shell none", `DOCKER_TLS_VERIFY=1
+			cmd: testutil.NewFakeCmdOut("minikube docker-env --shell none", `DOCKER_TLS_VERIFY=1
 DOCKER_HOST=http://127.0.0.1:8080
 DOCKER_CERT_PATH=bad/cert/path
-DOCKER_API_VERSION=1.23`, "", nil),
+DOCKER_API_VERSION=1.23`, nil),
 			shouldErr: true,
 		},
 		{
 			description: "missing host env, no error",
-			cmd: testutil.NewFakeRunCommand("minikube docker-env --shell none", `DOCKER_TLS_VERIFY=1
+			cmd: testutil.NewFakeCmdOut("minikube docker-env --shell none", `DOCKER_TLS_VERIFY=1
 DOCKER_CERT_PATH=testdata
-DOCKER_API_VERSION=1.23`, "", nil),
+DOCKER_API_VERSION=1.23`, nil),
 		},
 		{
 			description: "missing version env, no error",
-			cmd: testutil.NewFakeRunCommand("minikube docker-env --shell none", `DOCKER_TLS_VERIFY=1
+			cmd: testutil.NewFakeCmdOut("minikube docker-env --shell none", `DOCKER_TLS_VERIFY=1
 DOCKER_HOST=http://127.0.0.1:8080
-DOCKER_CERT_PATH=testdata`, "", nil),
+DOCKER_CERT_PATH=testdata`, nil),
 		},
 		{
 			description: "missing version env, no error",
-			cmd: testutil.NewFakeRunCommand("minikube docker-env --shell none", `DOCKER_TLS_VERIFY=1
+			cmd: testutil.NewFakeCmdOut("minikube docker-env --shell none", `DOCKER_TLS_VERIFY=1
 DOCKER_HOST=badurl
 DOCKER_CERT_PATH=testdata
-DOCKER_API_VERSION=1.23`, "", nil),
+DOCKER_API_VERSION=1.23`, nil),
 			shouldErr: true,
 		},
 		{
 			description: "bad env output, should fallback to host docker",
-			cmd: testutil.NewFakeRunCommand("minikube docker-env --shell none", `DOCKER_TLS_VERIFY=1
+			cmd: testutil.NewFakeCmdOut("minikube docker-env --shell none", `DOCKER_TLS_VERIFY=1
 DOCKER_HOST=http://127.0.0.1:8080=toomanyvalues
 DOCKER_CERT_PATH=testdata
-DOCKER_API_VERSION=1.23`, "", nil),
+DOCKER_API_VERSION=1.23`, nil),
 		},
 	}