Repository: helm/helm
Pull request #6790 (2019-10-25 10:00:22)

fix(v3): fix regression on non-zero plugin exist status
Fixes the regression in helm-3.0.0-beta.5 that swallows and rounds any non-zero exit status greater than 1 from a helm plugin to `1`.

This, for example, breaks `helm-diff` which relies on helm able to return `2` when `helm-diff` returned `2`.

Closese #6788


Patch:
diff --git a/cmd/helm/helm.go b/cmd/helm/helm.go
index 0e935addb..b30d6b011 100644
--- a/cmd/helm/helm.go
+++ b/cmd/helm/helm.go
@@ -68,6 +68,11 @@ func main() {
 	cmd := newRootCmd(actionConfig, os.Stdout, os.Args[1:])
 
 	if err := actionConfig.Init(settings, false, os.Getenv("HELM_DRIVER"), debug); err != nil {
+		debug("%+v", err)
+		os.Exit(1)
+	}
+
+	if err := cmd.Execute(); err != nil {
 		debug("%+v", err)
 		switch e := err.(type) {
 		case pluginError:
@@ -76,11 +81,6 @@ func main() {
 			os.Exit(1)
 		}
 	}
-
-	if err := cmd.Execute(); err != nil {
-		debug("%+v", err)
-		os.Exit(1)
-	}
 }
 
 // wordSepNormalizeFunc changes all flags that contain "_" separators
diff --git a/go.mod b/go.mod
index 6744a7451..d986c4b77 100644
--- a/go.mod
+++ b/go.mod
@@ -10,7 +10,7 @@ require (
 	github.com/Masterminds/vcs v1.13.0
 	github.com/Microsoft/go-winio v0.4.12 // indirect
 	github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a
-	github.com/containerd/containerd v1.3.0
+	github.com/containerd/containerd v1.3.0-beta.2.0.20190823190603-4a2f61c4f2b4
 	github.com/cyphar/filepath-securejoin v0.2.2
 	github.com/deislabs/oras v0.7.0
 	github.com/docker/distribution v2.7.1+incompatible
diff --git a/go.sum b/go.sum
index c82eb87fe..de710b39a 100644
--- a/go.sum
+++ b/go.sum
@@ -71,8 +71,6 @@ github.com/chai2010/gettext-go v0.0.0-20160711120539-c6fed771bfd5/go.mod h1:/iP1
 github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
 github.com/containerd/containerd v1.3.0-beta.2.0.20190823190603-4a2f61c4f2b4 h1:aMyA5J7j6D07U7pf8BFEY67BKoDcz0zWleAbQj3zVng=
 github.com/containerd/containerd v1.3.0-beta.2.0.20190823190603-4a2f61c4f2b4/go.mod h1:bC6axHOhabU15QhwfG7w5PipXdVtMXFTttgp+kVtyUA=
-github.com/containerd/containerd v1.3.0 h1:xjvXQWABwS2uiv3TWgQt5Uth60Gu86LTGZXMJkjc7rY=
-github.com/containerd/containerd v1.3.0/go.mod h1:bC6axHOhabU15QhwfG7w5PipXdVtMXFTttgp+kVtyUA=
 github.com/containerd/continuity v0.0.0-20181203112020-004b46473808 h1:4BX8f882bXEDKfWIf0wa8HRvpnBoPszJJXL+TVbBw4M=
 github.com/containerd/continuity v0.0.0-20181203112020-004b46473808/go.mod h1:GL3xCUCBDV3CZiTSEKksMWbLE66hEyuu9qyDOOqM47Y=
 github.com/coreos/bbolt v1.3.1-coreos.6 h1:uTXKg9gY70s9jMAKdfljFQcuh4e/BXOM+V+d00KFj3A=
diff --git a/pkg/kube/wait.go b/pkg/kube/wait.go
index 7198917c4..f60ce05e9 100644
--- a/pkg/kube/wait.go
+++ b/pkg/kube/wait.go
@@ -27,6 +27,7 @@ import (
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
+	networkingv1beta1 "k8s.io/api/networking/v1beta1"
 	apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
@@ -125,6 +126,15 @@ func (w *waiter) waitForResources(created ResourceList) error {
 				if !w.statefulSetReady(sts) {
 					return false, nil
 				}
+			case *extensionsv1beta1.Ingress, *networkingv1beta1.Ingress:
+				ing, err := w.c.NetworkingV1beta1().Ingresses(v.Namespace).Get(v.Name, metav1.GetOptions{})
+				if err != nil {
+					return false, err
+				}
+				if !w.ingressReady(ing) {
+					return false, nil
+				}
+
 			case *corev1.ReplicationController, *extensionsv1beta1.ReplicaSet, *appsv1beta2.ReplicaSet, *appsv1.ReplicaSet:
 				ok, err = w.podsReadyForObject(v.Namespace, value)
 			}
@@ -288,6 +298,14 @@ func (w *waiter) statefulSetReady(sts *appsv1.StatefulSet) bool {
 	return true
 }
 
+func (w *waiter) ingressReady(ing *networkingv1beta1.Ingress) bool {
+	if len(ing.Status.LoadBalancer.Ingress) == 0 {
+		w.log("Ingress is not ready: %s/%s", ing.GetNamespace(), ing.GetName())
+		return false
+	}
+	return true
+}
+
 func getPods(client kubernetes.Interface, namespace, selector string) ([]corev1.Pod, error) {
 	list, err := client.CoreV1().Pods(namespace).List(metav1.ListOptions{
 		LabelSelector: selector,


Test patch:
diff --git a/cmd/helm/helm_test.go b/cmd/helm/helm_test.go
index 934f14f86..924e8e9d3 100644
--- a/cmd/helm/helm_test.go
+++ b/cmd/helm/helm_test.go
@@ -20,6 +20,8 @@ import (
 	"bytes"
 	"io/ioutil"
 	"os"
+	"os/exec"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -151,3 +153,59 @@ func testChdir(t *testing.T, dir string) func() {
 	}
 	return func() { os.Chdir(old) }
 }
+
+func TestPluginExitCode(t *testing.T) {
+	if os.Getenv("RUN_MAIN_FOR_TESTING") == "1" {
+		os.Args = []string{"helm", "exitwith", "2"}
+
+		// We DO call helm's main() here. So this looks like a normal `helm` process.
+		main()
+
+		// As main calls os.Exit, we never reach this line.
+		// But the test called this block of code catches and verifies the exit code.
+		return
+	}
+
+	// Currently, plugins assume a Linux subsystem. Skip the execution
+	// tests until this is fixed
+	if runtime.GOOS != "windows" {
+		// Do a second run of this specific test(TestPluginExitCode) with RUN_MAIN_FOR_TESTING=1 set,
+		// So that the second run is able to run main() and this first run can verify the exit status returned by that.
+		//
+		// This technique originates from https://talks.golang.org/2014/testing.slide#23.
+		cmd := exec.Command(os.Args[0], "-test.run=TestPluginExitCode")
+		cmd.Env = append(
+			os.Environ(),
+			"RUN_MAIN_FOR_TESTING=1",
+			// See pkg/cli/environment.go for which envvars can be used for configuring these passes
+			// and also see plugin_test.go for how a plugin env can be set up.
+			// We just does the same setup as plugin_test.go via envvars
+			"HELM_PLUGINS=testdata/helmhome/helm/plugins",
+			"HELM_REPOSITORY_CONFIG=testdata/helmhome/helm/repositories.yaml",
+			"HELM_REPOSITORY_CACHE=testdata/helmhome/helm/repository",
+		)
+		stdout := &bytes.Buffer{}
+		stderr := &bytes.Buffer{}
+		cmd.Stdout = stdout
+		cmd.Stderr = stderr
+		err := cmd.Run()
+		exiterr, ok := err.(*exec.ExitError)
+
+		if !ok {
+			t.Fatalf("Unexpected error returned by os.Exit: %T", err)
+		}
+
+		if stdout.String() != "" {
+			t.Errorf("Expected no write to stdout: Got %q", stdout.String())
+		}
+
+		expectedStderr := "Error: plugin \"exitwith\" exited with error\n"
+		if stderr.String() != expectedStderr {
+			t.Errorf("Expected %q written to stderr: Got %q", expectedStderr, stderr.String())
+		}
+
+		if exiterr.ExitCode() != 2 {
+			t.Errorf("Expected exit code 2: Got %d", exiterr.ExitCode())
+		}
+	}
+}
diff --git a/cmd/helm/plugin_test.go b/cmd/helm/plugin_test.go
index e545a5d4f..7bc8fe70c 100644
--- a/cmd/helm/plugin_test.go
+++ b/cmd/helm/plugin_test.go
@@ -86,11 +86,13 @@ func TestLoadPlugins(t *testing.T) {
 		long   string
 		expect string
 		args   []string
+		code   int
 	}{
-		{"args", "echo args", "This echos args", "-a -b -c\n", []string{"-a", "-b", "-c"}},
-		{"echo", "echo stuff", "This echos stuff", "hello\n", []string{}},
-		{"env", "env stuff", "show the env", "env\n", []string{}},
-		{"fullenv", "show env vars", "show all env vars", envs + "\n", []string{}},
+		{"args", "echo args", "This echos args", "-a -b -c\n", []string{"-a", "-b", "-c"}, 0},
+		{"echo", "echo stuff", "This echos stuff", "hello\n", []string{}, 0},
+		{"env", "env stuff", "show the env", "env\n", []string{}, 0},
+		{"exitwith", "exitwith code", "This exits with the specified exit code", "", []string{"2"}, 2},
+		{"fullenv", "show env vars", "show all env vars", envs + "\n", []string{}, 0},
 	}
 
 	plugins := cmd.Commands()
@@ -117,7 +119,17 @@ func TestLoadPlugins(t *testing.T) {
 		// tests until this is fixed
 		if runtime.GOOS != "windows" {
 			if err := pp.RunE(pp, tt.args); err != nil {
-				t.Errorf("Error running %s: %+v", tt.use, err)
+				if tt.code > 0 {
+					perr, ok := err.(pluginError)
+					if !ok {
+						t.Errorf("Expected %s to return pluginError: got %v(%T)", tt.use, err, err)
+					}
+					if perr.code != tt.code {
+						t.Errorf("Expected %s to return %d: got %d", tt.use, tt.code, perr.code)
+					}
+				} else {
+					t.Errorf("Error running %s: %+v", tt.use, err)
+				}
 			}
 			if out.String() != tt.expect {
 				t.Errorf("Expected %s to output:\n%s\ngot\n%s", tt.use, tt.expect, out.String())
diff --git a/cmd/helm/testdata/helmhome/helm/plugins/exitwith/exitwith.sh b/cmd/helm/testdata/helmhome/helm/plugins/exitwith/exitwith.sh
new file mode 100755
index 000000000..ec8469657
--- /dev/null
+++ b/cmd/helm/testdata/helmhome/helm/plugins/exitwith/exitwith.sh
@@ -0,0 +1,2 @@
+#!/bin/bash
+exit $*
diff --git a/cmd/helm/testdata/helmhome/helm/plugins/exitwith/plugin.yaml b/cmd/helm/testdata/helmhome/helm/plugins/exitwith/plugin.yaml
new file mode 100644
index 000000000..5691d1712
--- /dev/null
+++ b/cmd/helm/testdata/helmhome/helm/plugins/exitwith/plugin.yaml
@@ -0,0 +1,4 @@
+name: exitwith
+usage: "exitwith code"
+description: "This exits with the specified exit code"
+command: "$HELM_PLUGIN_DIR/exitwith.sh"