Repository: redhat-certification/chart-verifier (go)

Issue:
Improve reason output when chart-testing fails
For example, current reason when no OC cluster is available is: 

Error waiting for process: exit status 1

Need to make sure error message provides a good indication of the underlying problem.

Pull request:
Improve reason output when chart-testing fails
This change list improves in the Stderr and Stdout capture of helm and
kubectl/oc when errors are triggered not only by the absence of those
programs, but also when those programs are available but return
errors.

Closes #114

Patch:
diff --git a/hack/get-helm.sh b/hack/get-helm.sh
index 5060218..6943911 100755
--- a/hack/get-helm.sh
+++ b/hack/get-helm.sh
@@ -1,6 +1,13 @@
 #!/usr/bin/env bash
 
-curl -O https://mirror.openshift.com/pub/openshift-v4/clients/helm/3.5.0/helm-linux-amd64.tar.gz
-mkdir -p /usr/local/bin
-tar xfz helm-linux-amd64.tar.gz -C /usr/local/bin
-mv /usr/local/bin/helm-linux-amd64 /usr/local/bin/helm
+HELM_VERSION=${HELM_VERSION-3.5.0}
+DOWNLOAD_DIR=${DOWNLOAD_DIR-/tmp/chart-verifier}
+INSTALL_DIR=${INSTALL_DIR-/usr/local/bin}
+
+mkdir -p "${DOWNLOAD_DIR}"
+pushd "${DOWNLOAD_DIR}"
+curl -O "https://mirror.openshift.com/pub/openshift-v4/clients/helm/${HELM_VERSION}/helm-linux-amd64.tar.gz
+tar xfz helm-linux-amd64.tar.gz
+mkdir -p "${INSTALL_DIR}"
+mv "${DOWNLOAD_DIR}/helm-linux-amd64" "${INSTALL_DIR}/helm"
+popd
diff --git a/pkg/chartverifier/verifier.go b/pkg/chartverifier/verifier.go
index 7105c52..ff1fa1b 100644
--- a/pkg/chartverifier/verifier.go
+++ b/pkg/chartverifier/verifier.go
@@ -18,7 +18,6 @@ package chartverifier
 
 import (
 	"github.com/Masterminds/semver"
-	"github.com/helm/chart-testing/v3/pkg/exec"
 	"github.com/redhat-certification/chart-verifier/pkg/chartverifier/checks"
 	"github.com/redhat-certification/chart-verifier/pkg/tool"
 	"github.com/spf13/viper"
@@ -81,7 +80,7 @@ type version struct{}
 
 func (ver *version) getVersion(debug bool) (string, error) {
 
-	procExec := exec.NewProcessExecutor(debug)
+	procExec := tool.NewProcessExecutor(debug)
 	oc := tool.NewOc(procExec)
 
 	// oc.GetVersion() returns an error both in case the oc command can't be executed and
diff --git a/pkg/tool/exec.go b/pkg/tool/exec.go
new file mode 100644
index 0000000..5a67c15
--- /dev/null
+++ b/pkg/tool/exec.go
@@ -0,0 +1,54 @@
+package tool
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/helm/chart-testing/v3/pkg/exec"
+)
+
+type ProcessExecutor struct {
+	exec.ProcessExecutor
+}
+
+func NewProcessExecutor(debug bool) ProcessExecutor {
+	return ProcessExecutor{
+		exec.NewProcessExecutor(debug),
+	}
+}
+
+func (p ProcessExecutor) RunProcessAndCaptureOutput(executable string, execArgs ...interface{}) (string, error) {
+	return p.RunProcessInDirAndCaptureOutput("", executable, execArgs...)
+}
+
+// RunProcessInDirAndCaptureOutput overrides exec.ProcessExecutor's and inject the command line and any streamed content
+// to either Stdout or Stderr into the returned error, if any.
+func (p ProcessExecutor) RunProcessInDirAndCaptureOutput(
+	workingDirectory string,
+	executable string,
+	execArgs ...interface{},
+) (string, error) {
+	cmd, err := p.CreateProcess(executable, execArgs...)
+	if err != nil {
+		return "", err
+	}
+
+	cmd.Dir = workingDirectory
+	bytes, err := cmd.CombinedOutput()
+	capturedOutput := strings.TrimSpace(string(bytes))
+
+	execArgsCopy := toStringArray(execArgs)
+	execArgsStr := strings.Join(execArgsCopy, " ")
+
+	if err != nil {
+		if len(capturedOutput) == 0 {
+			return "", fmt.Errorf(
+				"Error running process: executing %s with args %q: %w",
+				executable, execArgsStr, err)
+		}
+		return capturedOutput, fmt.Errorf(
+			"Error running process: executing %s with args %q: %w\n---\n%s",
+			executable, execArgsStr, err, capturedOutput)
+	}
+	return capturedOutput, nil
+}
diff --git a/pkg/tool/helm.go b/pkg/tool/helm.go
index c9ab01f..19296f7 100644
--- a/pkg/tool/helm.go
+++ b/pkg/tool/helm.go
@@ -2,9 +2,7 @@ package tool
 
 import (
 	"fmt"
-	"strings"
 
-	"github.com/helm/chart-testing/v3/pkg/exec"
 	"github.com/helm/chart-testing/v3/pkg/tool"
 )
 
@@ -12,33 +10,48 @@ import (
 // to silence output being streamed to Stdout.
 type Helm struct {
 	tool.Helm
-	exec.ProcessExecutor
+	ProcessExecutor
 	extraArgs []string
 }
 
-func NewHelm(exec exec.ProcessExecutor, extraArgs []string) Helm {
+func NewHelm(exec ProcessExecutor, extraArgs []string) Helm {
 	return Helm{
-		tool.NewHelm(exec, extraArgs),
+		tool.NewHelm(exec.ProcessExecutor, extraArgs),
 		exec,
 		extraArgs,
 	}
 }
 
-func (h Helm) InstallWithValues(chart string, valuesFile string, namespace string, release string) error {
-	var values []string
-	if valuesFile != "" {
-		values = []string{"--values", valuesFile}
+func toStringArray(args []interface{}) []string {
+	copy := make([]string, len(args))
+	for i, a := range args {
+		copy[i] = fmt.Sprint(a)
 	}
+	return copy
+}
 
-	helmInstallArgs := []string{"install", release, chart, "--namespace", namespace, "--wait"}
-	helmInstallArgs = append(helmInstallArgs, values...)
-	helmInstallArgs = append(helmInstallArgs, h.extraArgs...)
+func toInterfaceArray(args []string) []interface{} {
+	copy := make([]interface{}, len(args))
+	for i, a := range args {
+		copy[i] = a
+	}
+	return copy
+}
 
-	if _, err := h.RunProcessAndCaptureOutput("helm", helmInstallArgs); err != nil {
-		return fmt.Errorf("executing helm with args %q: %w", strings.Join(helmInstallArgs, " "), err)
+// InstallWithValues overrides chart-testing's tool.Helm method to execute the modified RunProcessAndCaptureOutput
+// method.
+func (h Helm) InstallWithValues(chart string, valuesFile string, namespace string, release string) error {
+	var values []interface{}
+	if valuesFile != "" {
+		values = []interface{}{"--values", valuesFile}
 	}
 
-	return nil
+	helmArgs := []interface{}{"install", release, chart, "--namespace", namespace, "--wait"}
+	helmArgs = append(helmArgs, values...)
+	helmArgs = append(helmArgs, toInterfaceArray(h.extraArgs)...)
+
+	_, err := h.RunProcessAndCaptureOutput("helm", helmArgs...)
+	return err
 }
 
 func (h Helm) Test(namespace string, release string) error {
diff --git a/pkg/tool/kubectl.go b/pkg/tool/kubectl.go
index 841c7ea..7736699 100644
--- a/pkg/tool/kubectl.go
+++ b/pkg/tool/kubectl.go
@@ -4,7 +4,6 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/helm/chart-testing/v3/pkg/exec"
 	"github.com/helm/chart-testing/v3/pkg/tool"
 )
 
@@ -12,12 +11,12 @@ import (
 // chart-testing to silence output being streamed to Stdout.
 type Kubectl struct {
 	tool.Kubectl
-	exec.ProcessExecutor
+	ProcessExecutor
 }
 
-func NewKubectl(exec exec.ProcessExecutor) Kubectl {
+func NewKubectl(exec ProcessExecutor) Kubectl {
 	return Kubectl{
-		tool.NewKubectl(exec),
+		tool.NewKubectl(exec.ProcessExecutor),
 		exec,
 	}
 }
diff --git a/pkg/tool/oc.go b/pkg/tool/oc.go
index 0605b68..5616771 100644
--- a/pkg/tool/oc.go
+++ b/pkg/tool/oc.go
@@ -3,24 +3,23 @@ package tool
 import (
 	"fmt"
 
-	"github.com/helm/chart-testing/v3/pkg/exec"
 	"gopkg.in/yaml.v3"
 )
 
 type Oc struct {
-	exec exec.ProcessExecutor
+	ProcessExecutor
 }
 
-func NewOc(exec exec.ProcessExecutor) Oc {
+func NewOc(exec ProcessExecutor) Oc {
 	return Oc{
-		exec: exec,
+		ProcessExecutor: exec,
 	}
 }
 
 const osVersionKey = "openshiftVersion"
 
 func (o Oc) GetVersion() (string, error) {
-	rawOutput, err := o.exec.RunProcessAndCaptureOutput("oc", "version", "-o", "yaml")
+	rawOutput, err := o.RunProcessAndCaptureOutput("oc", "version", "-o", "yaml")
 	if err != nil {
 		return "", err
 	}


Test patch:
diff --git a/pkg/chartverifier/checks/charttesting.go b/pkg/chartverifier/checks/charttesting.go
index 034c5e1..f79d477 100644
--- a/pkg/chartverifier/checks/charttesting.go
+++ b/pkg/chartverifier/checks/charttesting.go
@@ -9,7 +9,6 @@ import (
 
 	"github.com/helm/chart-testing/v3/pkg/chart"
 	"github.com/helm/chart-testing/v3/pkg/config"
-	"github.com/helm/chart-testing/v3/pkg/exec"
 	"github.com/helm/chart-testing/v3/pkg/util"
 	"github.com/imdario/mergo"
 	"github.com/redhat-certification/chart-verifier/pkg/tool"
@@ -63,7 +62,7 @@ func buildChartTestingConfiguration(opts *CheckOptions) config.Configuration {
 func ChartTesting(opts *CheckOptions) (Result, error) {
 
 	cfg := buildChartTestingConfiguration(opts)
-	procExec := exec.NewProcessExecutor(cfg.Debug)
+	procExec := tool.NewProcessExecutor(cfg.Debug)
 	extraArgs := strings.Fields(cfg.HelmExtraArgs)
 	helm := tool.NewHelm(procExec, extraArgs)
 	kubectl := tool.NewKubectl(procExec)
diff --git a/pkg/tool/helm_test.go b/pkg/tool/helm_test.go
new file mode 100644
index 0000000..ddf791e
--- /dev/null
+++ b/pkg/tool/helm_test.go
@@ -0,0 +1,67 @@
+package tool
+
+import (
+	"fmt"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func temporarilyToggleEnv(name string) (untoggleFn func(), err error) {
+	originalValue := os.Getenv(name)
+	err = os.Unsetenv(name)
+	untoggleFn = func() {
+		os.Setenv(name, originalValue)
+	}
+	return
+}
+
+func TestInstallWithValues(t *testing.T) {
+	t.Run("helm not available in path", func(t *testing.T) {
+		untogglePathEnv, err := temporarilyToggleEnv("PATH")
+		require.NoError(t, err)
+		defer untogglePathEnv()
+
+		// arrange
+		valuesFile := ""
+		chrt := "non-existing-chart.tgz"
+		release := "non-existing-release"
+		namespace := "default"
+		expectedErrorMessage := fmt.Sprintf(
+			"Error running process: executing helm with args \"install %s %s --namespace %s --wait\": "+
+				"exec: \"helm\": executable file not found in $PATH",
+			release, chrt, namespace)
+		h := NewHelm(NewProcessExecutor(false), []string{})
+
+		// act
+		// for this test, none of the arguments matter si
+		err = h.InstallWithValues(chrt, valuesFile, namespace, release)
+
+		// assert
+		require.Error(t, err)
+		require.Equal(t, expectedErrorMessage, err.Error())
+	})
+
+	t.Run("helm failure should include content streamed to both Stderr and Stdout", func(t *testing.T) {
+		// arrange
+		chrt := "non-existing-chart.tgz"
+		release := "non-existing-release"
+		valuesFile := ""
+		namespace := "default"
+		expectedErrorMessage := fmt.Sprintf(
+			"Error running process: executing helm with args \"install %s %s --namespace %s --wait\": "+
+				"exit status 1\n---\nError: failed to download \"%s\" (hint: running `helm repo update` may help)",
+			release, chrt, namespace, chrt)
+		processExecutor := NewProcessExecutor(false)
+		extraArgs := []string{}
+		h := NewHelm(processExecutor, extraArgs)
+
+		// act
+		err := h.InstallWithValues(chrt, valuesFile, namespace, release)
+
+		// assert
+		require.Error(t, err)
+		require.Equal(t, expectedErrorMessage, err.Error())
+	})
+}