Various U/X improvements for helmfile apply (#586)

* Various U/X improvements for `helmfile apply` This improves the U/X of `helmfile apply`, by allowing you to selectively apply sub-helmfiles. When you have two or more sub-helmfiles processed, typing `n` to cancel the first doesn't automatically stop the whole helmfile execution. Instead, it proceeds by diffing the next sub-helmfile, and asks you to apply it, which should be what the user would expect. To support this workflow, I have suppressed useless exec logs, correct exit status when diff exists in sub-helmfiles but not in the parent helmfile, and made the final error message emitted by helmfile better. More concretely, this moves more output from `helm` to STDERR and the `debug` log-level. The overall output from `helmfile` should be a bit more cleaner especially for `apply`, `sync`, `diff` and perhaps other `helmfile` sub-commands, too. For example, when one of release failed, `helmfile`'s final error message now includes the error message from the failed `helm` execution, like seen in the last line: ``` List of updated releases : RELEASE CHART VERSION envoy stable/envoy 1.5.0 List of releases in error : RELEASE envoy2 in ./helmfile.yaml: in .helmfiles[0]: in /Users/c-ykuoka/helmfile/helmfile.1.yaml: failed processing release envoy2: helm exited with status 1: Error: UPGRADE FAILED: "envoy2" has no deployed releases ``` This way you can better understand what caused helmfile to finally fail. `helmfile` has been streaminig a lot of stdout and stderr contents from the `helm` commands regardless of the helmfile's log-level. It has been suppressed by default and moved to the `debug` log-level. You will see that it helps you focus on what was the cause of a failure. While working on the above, I found an another bug that made `--detailed-exitcode` useless in some case. That is, `helmfile diff --detailed-exitcode`, when any diff existed only in sub-helmfiles, has been returning an exit code of `1`. It should return `2` when any release had diff and no release had an error, regardless of the target is a sub-helmfile or a parent helmfile. Why? Because that's what `--detailed-exitcode` meant for! After this PR gets merged, `helmfile diff --detailed-exitcode` propery return exit code `2` in such cases. Fixes #543 Resolves #540
2026-10-09 05:00:44 +02:00 · 2019-05-12 16:26:11 +09:00
parent bae842f234
commit 4f83e69bf6
11 changed files with 257 additions and 87 deletions
@@ -6,7 +6,6 @@ import (
 	"io/ioutil"
 	"os"
 	"strings"
-
 	"sync"

 	"go.uber.org/zap"
@@ -183,7 +182,26 @@ func (helm *execer) DiffRelease(context HelmContext, name, chart string, flags .
 	preArgs := context.GetTillerlessArgs(helm.helmBinary)
 	env := context.getTillerlessEnv()
 	out, err := helm.exec(append(append(preArgs, "diff", "upgrade", "--allow-unreleased", name, chart), flags...), env)
-	helm.write(out)
+	// Do our best to write STDOUT only when diff existed
+	// Unfortunately, this works only when you run helmfile with `--detailed-exitcode`
+	detailedExitcodeEnabled := false
+	for _, f := range flags {
+		if strings.Contains(f, "detailed-exitcode") {
+			detailedExitcodeEnabled = true
+			break
+		}
+	}
+	if detailedExitcodeEnabled {
+		switch e := err.(type) {
+		case ExitError:
+			if e.ExitStatus() == 2 {
+				helm.write(out)
+				return err
+			}
+		}
+	} else {
+		helm.write(out)
+	}
 	return err
 }

@@ -227,8 +245,11 @@ func (helm *execer) exec(args []string, env map[string]string) ([]byte, error) {
 	if helm.kubeContext != "" {
 		cmdargs = append(cmdargs, "--kube-context", helm.kubeContext)
 	}
-	helm.logger.Debugf("exec: %s %s", helm.helmBinary, strings.Join(cmdargs, " "))
-	return helm.runner.Execute(helm.helmBinary, cmdargs, env)
+	cmd := fmt.Sprintf("exec: %s %s", helm.helmBinary, strings.Join(cmdargs, " "))
+	helm.logger.Debug(cmd)
+	bytes, err := helm.runner.Execute(helm.helmBinary, cmdargs, env)
+	helm.logger.Debugf("%s: %s", cmd, bytes)
+	return bytes, err
 }

 func (helm *execer) info(out []byte) {
@@ -78,6 +78,7 @@ func Test_AddRepo(t *testing.T) {
 	helm.AddRepo("myRepo", "https://repo.example.com/", "cert.pem", "key.pem", "", "")
 	expected := `Adding repo myRepo https://repo.example.com/
 exec: helm repo add myRepo https://repo.example.com/ --cert-file cert.pem --key-file key.pem --kube-context dev
+exec: helm repo add myRepo https://repo.example.com/ --cert-file cert.pem --key-file key.pem --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.AddRepo()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -87,6 +88,7 @@ exec: helm repo add myRepo https://repo.example.com/ --cert-file cert.pem --key-
 	helm.AddRepo("myRepo", "https://repo.example.com/", "", "", "", "")
 	expected = `Adding repo myRepo https://repo.example.com/
 exec: helm repo add myRepo https://repo.example.com/ --kube-context dev
+exec: helm repo add myRepo https://repo.example.com/ --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.AddRepo()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -96,6 +98,7 @@ exec: helm repo add myRepo https://repo.example.com/ --kube-context dev
 	helm.AddRepo("myRepo", "https://repo.example.com/", "", "", "example_user", "example_password")
 	expected = `Adding repo myRepo https://repo.example.com/
 exec: helm repo add myRepo https://repo.example.com/ --username example_user --password example_password --kube-context dev
+exec: helm repo add myRepo https://repo.example.com/ --username example_user --password example_password --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.AddRepo()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -109,6 +112,7 @@ func Test_UpdateRepo(t *testing.T) {
 	helm.UpdateRepo()
 	expected := `Updating repo
 exec: helm repo update --kube-context dev
+exec: helm repo update --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.UpdateRepo()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -122,6 +126,7 @@ func Test_SyncRelease(t *testing.T) {
 	helm.SyncRelease(HelmContext{}, "release", "chart", "--timeout 10", "--wait")
 	expected := `Upgrading chart
 exec: helm upgrade --install --reset-values release chart --timeout 10 --wait --kube-context dev
+exec: helm upgrade --install --reset-values release chart --timeout 10 --wait --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.SyncRelease()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -131,6 +136,7 @@ exec: helm upgrade --install --reset-values release chart --timeout 10 --wait --
 	helm.SyncRelease(HelmContext{}, "release", "chart")
 	expected = `Upgrading chart
 exec: helm upgrade --install --reset-values release chart --kube-context dev
+exec: helm upgrade --install --reset-values release chart --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.SyncRelease()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -145,6 +151,7 @@ func Test_SyncReleaseTillerless(t *testing.T) {
 		"--timeout 10", "--wait")
 	expected := `Upgrading chart
 exec: helm tiller run foo -- helm upgrade --install --reset-values release chart --timeout 10 --wait --kube-context dev
+exec: helm tiller run foo -- helm upgrade --install --reset-values release chart --timeout 10 --wait --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.SyncRelease()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -158,6 +165,7 @@ func Test_UpdateDeps(t *testing.T) {
 	helm.UpdateDeps("./chart/foo")
 	expected := `Updating dependency ./chart/foo
 exec: helm dependency update ./chart/foo --kube-context dev
+exec: helm dependency update ./chart/foo --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.UpdateDeps()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -168,6 +176,7 @@ exec: helm dependency update ./chart/foo --kube-context dev
 	helm.UpdateDeps("./chart/foo")
 	expected = `Updating dependency ./chart/foo
 exec: helm dependency update ./chart/foo --verify --kube-context dev
+exec: helm dependency update ./chart/foo --verify --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.AddRepo()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -181,6 +190,7 @@ func Test_BuildDeps(t *testing.T) {
 	helm.BuildDeps("./chart/foo")
 	expected := `Building dependency ./chart/foo
 exec: helm dependency build ./chart/foo --kube-context dev
+exec: helm dependency build ./chart/foo --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.BuildDeps()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -191,6 +201,7 @@ exec: helm dependency build ./chart/foo --kube-context dev
 	helm.BuildDeps("./chart/foo")
 	expected = `Building dependency ./chart/foo
 exec: helm dependency build ./chart/foo --verify --kube-context dev
+exec: helm dependency build ./chart/foo --verify --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.BuildDeps()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -204,6 +215,7 @@ func Test_DecryptSecret(t *testing.T) {
 	helm.DecryptSecret(HelmContext{}, "secretName")
 	expected := `Decrypting secret secretName
 exec: helm secrets dec secretName --kube-context dev
+exec: helm secrets dec secretName --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.DecryptSecret()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -217,6 +229,7 @@ func Test_DiffRelease(t *testing.T) {
 	helm.DiffRelease(HelmContext{}, "release", "chart", "--timeout 10", "--wait")
 	expected := `Comparing release chart
 exec: helm diff upgrade --allow-unreleased release chart --timeout 10 --wait --kube-context dev
+exec: helm diff upgrade --allow-unreleased release chart --timeout 10 --wait --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.DiffRelease()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -226,6 +239,7 @@ exec: helm diff upgrade --allow-unreleased release chart --timeout 10 --wait --k
 	helm.DiffRelease(HelmContext{}, "release", "chart")
 	expected = `Comparing release chart
 exec: helm diff upgrade --allow-unreleased release chart --kube-context dev
+exec: helm diff upgrade --allow-unreleased release chart --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.DiffRelease()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -239,6 +253,7 @@ func Test_DiffReleaseTillerless(t *testing.T) {
 	helm.DiffRelease(HelmContext{Tillerless: true}, "release", "chart", "--timeout 10", "--wait")
 	expected := `Comparing release chart
 exec: helm tiller run -- helm diff upgrade --allow-unreleased release chart --timeout 10 --wait --kube-context dev
+exec: helm tiller run -- helm diff upgrade --allow-unreleased release chart --timeout 10 --wait --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.DiffRelease()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -252,6 +267,7 @@ func Test_DeleteRelease(t *testing.T) {
 	helm.DeleteRelease(HelmContext{}, "release")
 	expected := `Deleting release
 exec: helm delete release --kube-context dev
+exec: helm delete release --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.DeleteRelease()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -264,6 +280,7 @@ func Test_DeleteRelease_Flags(t *testing.T) {
 	helm.DeleteRelease(HelmContext{}, "release", "--purge")
 	expected := `Deleting release
 exec: helm delete release --purge --kube-context dev
+exec: helm delete release --purge --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.DeleteRelease()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -277,6 +294,7 @@ func Test_TestRelease(t *testing.T) {
 	helm.TestRelease(HelmContext{}, "release")
 	expected := `Testing release
 exec: helm test release --kube-context dev
+exec: helm test release --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.TestRelease()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -289,6 +307,7 @@ func Test_TestRelease_Flags(t *testing.T) {
 	helm.TestRelease(HelmContext{}, "release", "--cleanup", "--timeout", "60")
 	expected := `Testing release
 exec: helm test release --cleanup --timeout 60 --kube-context dev
+exec: helm test release --cleanup --timeout 60 --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.TestRelease()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -302,6 +321,7 @@ func Test_ReleaseStatus(t *testing.T) {
 	helm.ReleaseStatus(HelmContext{}, "myRelease")
 	expected := `Getting status myRelease
 exec: helm status myRelease --kube-context dev
+exec: helm status myRelease --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.ReleaseStatus()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -314,7 +334,9 @@ func Test_exec(t *testing.T) {
 	helm := MockExecer(logger, "")
 	env := map[string]string{}
 	helm.exec([]string{"version"}, env)
-	expected := "exec: helm version\n"
+	expected := `exec: helm version
+exec: helm version: 
+`
 	if buffer.String() != expected {
 		t.Errorf("helmexec.exec()\nactual = %v\nexpect = %v", buffer.String(), expected)
 	}
@@ -328,14 +350,18 @@ func Test_exec(t *testing.T) {
 	buffer.Reset()
 	helm = MockExecer(logger, "dev")
 	helm.exec([]string{"diff", "release", "chart", "--timeout 10", "--wait"}, env)
-	expected = "exec: helm diff release chart --timeout 10 --wait --kube-context dev\n"
+	expected = `exec: helm diff release chart --timeout 10 --wait --kube-context dev
+exec: helm diff release chart --timeout 10 --wait --kube-context dev: 
+`
 	if buffer.String() != expected {
 		t.Errorf("helmexec.exec()\nactual = %v\nexpect = %v", buffer.String(), expected)
 	}

 	buffer.Reset()
 	helm.exec([]string{"version"}, env)
-	expected = "exec: helm version --kube-context dev\n"
+	expected = `exec: helm version --kube-context dev
+exec: helm version --kube-context dev: 
+`
 	if buffer.String() != expected {
 		t.Errorf("helmexec.exec()\nactual = %v\nexpect = %v", buffer.String(), expected)
 	}
@@ -343,7 +369,9 @@ func Test_exec(t *testing.T) {
 	buffer.Reset()
 	helm.SetExtraArgs("foo")
 	helm.exec([]string{"version"}, env)
-	expected = "exec: helm version foo --kube-context dev\n"
+	expected = `exec: helm version foo --kube-context dev
+exec: helm version foo --kube-context dev: 
+`
 	if buffer.String() != expected {
 		t.Errorf("helmexec.exec()\nactual = %v\nexpect = %v", buffer.String(), expected)
 	}
@@ -352,7 +380,9 @@ func Test_exec(t *testing.T) {
 	helm = MockExecer(logger, "")
 	helm.SetHelmBinary("overwritten")
 	helm.exec([]string{"version"}, env)
-	expected = "exec: overwritten version\n"
+	expected = `exec: overwritten version
+exec: overwritten version: 
+`
 	if buffer.String() != expected {
 		t.Errorf("helmexec.exec()\nactual = %v\nexpect = %v", buffer.String(), expected)
 	}
@@ -365,6 +395,7 @@ func Test_Lint(t *testing.T) {
 	helm.Lint("path/to/chart", "--values", "file.yml")
 	expected := `Linting path/to/chart
 exec: helm lint path/to/chart --values file.yml --kube-context dev
+exec: helm lint path/to/chart --values file.yml --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.Lint()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -378,6 +409,7 @@ func Test_Fetch(t *testing.T) {
 	helm.Fetch("chart", "--version", "1.2.3", "--untar", "--untardir", "/tmp/dir")
 	expected := `Fetching chart
 exec: helm fetch chart --version 1.2.3 --untar --untardir /tmp/dir --kube-context dev
+exec: helm fetch chart --version 1.2.3 --untar --untardir /tmp/dir --kube-context dev: 
 `
 	if buffer.String() != expected {
 		t.Errorf("helmexec.Lint()\nactual = %v\nexpect = %v", buffer.String(), expected)
@@ -387,6 +419,7 @@ exec: helm fetch chart --version 1.2.3 --untar --untardir /tmp/dir --kube-contex
 var logLevelTests = map[string]string{
 	"debug": `Adding repo myRepo https://repo.example.com/
 exec: helm repo add myRepo https://repo.example.com/ --username example_user --password example_password
+exec: helm repo add myRepo https://repo.example.com/ --username example_user --password example_password: 
 `,
 	"info": `Adding repo myRepo https://repo.example.com/
 `,
@@ -0,0 +1,36 @@
+package helmexec
+
+import (
+	"fmt"
+	"path/filepath"
+	"strings"
+)
+
+func newExitError(helmCmdPath string, exitStatus int, errorMessage string) ExitError {
+	return ExitError{
+		msg:        fmt.Sprintf("%s exited with status %d:\n%s", filepath.Base(helmCmdPath), exitStatus, indent(strings.TrimSpace(errorMessage))),
+		exitStatus: exitStatus,
+	}
+}
+
+func indent(text string) string {
+	lines := strings.Split(text, "\n")
+	for i := range lines {
+		lines[i] = "  " + lines[i]
+	}
+	return strings.Join(lines, "\n")
+}
+
+// ExitError is created whenever your shell command exits with a non-zero exit status
+type ExitError struct {
+	msg        string
+	exitStatus int
+}
+
+func (e ExitError) Error() string {
+	return e.msg
+}
+
+func (e ExitError) ExitStatus() int {
+	return e.exitStatus
+}
@@ -8,6 +8,7 @@ import (
 	"os"
 	"os/exec"
 	"strings"
+	"syscall"
 )

 const (
@@ -51,17 +52,22 @@ func combinedOutput(c *exec.Cmd, logger *zap.SugaredLogger) ([]byte, error) {
 	o := stdout.Bytes()
 	e := stderr.Bytes()

-	if len(e) > 0 {
-		logger.Debugf("%s\n", e)
-	}
-
 	if err != nil {
 		// TrimSpace is necessary, because otherwise helmfile prints the redundant new-lines after each error like:
 		//
 		//   err: release "envoy2" in "helmfile.yaml" failed: exit status 1: Error: could not find a ready tiller pod
 		//   <redundant new line!>
 		//   err: release "envoy" in "helmfile.yaml" failed: exit status 1: Error: could not find a ready tiller pod
-		err = fmt.Errorf("%v: %s", err, strings.TrimSpace(string(e)))
+		switch ee := err.(type) {
+		case *exec.ExitError:
+			// Propagate any non-zero exit status from the external command, rather than throwing it away,
+			// so that helmfile could return its own exit code accordingly
+			waitStatus := ee.Sys().(syscall.WaitStatus)
+			exitStatus := waitStatus.ExitStatus()
+			err = newExitError(c.Path, exitStatus, string(e))
+		default:
+			panic(fmt.Sprintf("unexpected error: %v", err))
+		}
 	}

 	return o, err