Repository: helm/helm
Pull request #8779 (2020-09-21 15:32:33)

use warning function
This ensures warning messages are displayed on stderr rather than stdout.

relates to an issue discovered in #8776.

Signed-off-by: Matthew Fisher <matt.fisher@microsoft.com>


Patch:
diff --git a/cmd/helm/registry_login.go b/cmd/helm/registry_login.go
index e3435bf9d..43228f90a 100644
--- a/cmd/helm/registry_login.go
+++ b/cmd/helm/registry_login.go
@@ -104,7 +104,7 @@ func getUsernamePassword(usernameOpt string, passwordOpt string, passwordFromStd
 			}
 		}
 	} else {
-		fmt.Fprintln(os.Stderr, "WARNING! Using --password via the CLI is insecure. Use --password-stdin.")
+		warning("Using --password via the CLI is insecure. Use --password-stdin.")
 	}
 
 	return username, password, nil
diff --git a/cmd/helm/repo_add.go b/cmd/helm/repo_add.go
index f79c213c0..1c2162bfa 100644
--- a/cmd/helm/repo_add.go
+++ b/cmd/helm/repo_add.go
@@ -116,6 +116,11 @@ func (o *repoAddOptions) run(out io.Writer) error {
 		return err
 	}
 
+	// If the repo exists and --force-update was not specified, error out.
+	if !o.forceUpdate && f.Has(o.name) {
+		return errors.Errorf("repository name (%s) already exists, please specify a different name", o.name)
+	}
+
 	if o.username != "" && o.password == "" {
 		fd := int(os.Stdin.Fd())
 		fmt.Fprint(out, "Password: ")
@@ -138,23 +143,6 @@ func (o *repoAddOptions) run(out io.Writer) error {
 		InsecureSkipTLSverify: o.insecureSkipTLSverify,
 	}
 
-	// If the repo exists do one of two things:
-	// 1. If the configuration for the name is the same continue without error
-	// 2. When the config is different require --force-update
-	if !o.forceUpdate && f.Has(o.name) {
-		existing := f.Get(o.name)
-		if c != *existing {
-
-			// The input coming in for the name is different from what is already
-			// configured. Return an error.
-			return errors.Errorf("repository name (%s) already exists, please specify a different name", o.name)
-		}
-
-		// The add is idempotent so do nothing
-		fmt.Fprintf(out, "%q already exists with the same configuration, skipping\n", o.name)
-		return nil
-	}
-
 	r, err := repo.NewChartRepository(&c, getter.All(settings))
 	if err != nil {
 		return err
diff --git a/cmd/helm/root.go b/cmd/helm/root.go
index 91542bb7e..0a2b1be8f 100644
--- a/cmd/helm/root.go
+++ b/cmd/helm/root.go
@@ -205,7 +205,7 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string
 	loadPlugins(cmd, out)
 
 	// Check permissions on critical files
-	checkPerms(out)
+	checkPerms()
 
 	return cmd, nil
 }
diff --git a/cmd/helm/root_unix.go b/cmd/helm/root_unix.go
index 4eb0b442b..b1b0f3896 100644
--- a/cmd/helm/root_unix.go
+++ b/cmd/helm/root_unix.go
@@ -19,14 +19,12 @@ limitations under the License.
 package main
 
 import (
-	"fmt"
-	"io"
 	"os"
 	"os/user"
 	"path/filepath"
 )
 
-func checkPerms(out io.Writer) {
+func checkPerms() {
 	// This function MUST NOT FAIL, as it is just a check for a common permissions problem.
 	// If for some reason the function hits a stopping condition, it may panic. But only if
 	// we can be sure that it is panicing because Helm cannot proceed.
@@ -52,9 +50,9 @@ func checkPerms(out io.Writer) {
 
 	perm := fi.Mode().Perm()
 	if perm&0040 > 0 {
-		fmt.Fprintf(out, "WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: %s\n", kc)
+		warning("Kubernetes configuration file is group-readable. This is insecure. Location: %s", kc)
 	}
 	if perm&0004 > 0 {
-		fmt.Fprintf(out, "WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: %s\n", kc)
+		warning("Kubernetes configuration file is world-readable. This is insecure. Location: %s", kc)
 	}
 }
diff --git a/cmd/helm/root_windows.go b/cmd/helm/root_windows.go
index 243780d40..0b390f16c 100644
--- a/cmd/helm/root_windows.go
+++ b/cmd/helm/root_windows.go
@@ -18,7 +18,7 @@ package main
 
 import "io"
 
-func checkPerms(out io.Writer) {
+func checkPerms() {
 	// Not yet implemented on Windows. If you know how to do a comprehensive perms
 	// check on Windows, contributions welcomed!
 }
diff --git a/cmd/helm/search_repo.go b/cmd/helm/search_repo.go
index a7f27f179..bf82a6051 100644
--- a/cmd/helm/search_repo.go
+++ b/cmd/helm/search_repo.go
@@ -22,7 +22,6 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
-	"os"
 	"path/filepath"
 	"strings"
 
@@ -184,7 +183,7 @@ func (o *searchRepoOptions) buildIndex() (*search.Index, error) {
 		f := filepath.Join(o.repoCacheDir, helmpath.CacheIndexFile(n))
 		ind, err := repo.LoadIndexFile(f)
 		if err != nil {
-			fmt.Fprintf(os.Stderr, "WARNING: Repo %q is corrupt or missing. Try 'helm repo update'.", n)
+			warning("Repo %q is corrupt or missing. Try 'helm repo update'.", n)
 			continue
 		}
 


Test patch:
diff --git a/cmd/helm/repo_add_test.go b/cmd/helm/repo_add_test.go
index f3bc54985..d358ad970 100644
--- a/cmd/helm/repo_add_test.go
+++ b/cmd/helm/repo_add_test.go
@@ -40,38 +40,14 @@ func TestRepoAddCmd(t *testing.T) {
 	}
 	defer srv.Stop()
 
-	// A second test server is setup to verify URL changing
-	srv2, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer srv2.Stop()
-
 	tmpdir := ensure.TempDir(t)
 	repoFile := filepath.Join(tmpdir, "repositories.yaml")
 
-	tests := []cmdTestCase{
-		{
-			name:   "add a repository",
-			cmd:    fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s", srv.URL(), repoFile, tmpdir),
-			golden: "output/repo-add.txt",
-		},
-		{
-			name:   "add repository second time",
-			cmd:    fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s", srv.URL(), repoFile, tmpdir),
-			golden: "output/repo-add2.txt",
-		},
-		{
-			name:      "add repository different url",
-			cmd:       fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s", srv2.URL(), repoFile, tmpdir),
-			wantError: true,
-		},
-		{
-			name:   "add repository second time",
-			cmd:    fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s --force-update", srv2.URL(), repoFile, tmpdir),
-			golden: "output/repo-add.txt",
-		},
-	}
+	tests := []cmdTestCase{{
+		name:   "add a repository",
+		cmd:    fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s", srv.URL(), repoFile, tmpdir),
+		golden: "output/repo-add.txt",
+	}}
 
 	runTestCmd(t, tests)
 }
diff --git a/cmd/helm/root_unix_test.go b/cmd/helm/root_unix_test.go
index b1fcfbc66..89c3c1eea 100644
--- a/cmd/helm/root_unix_test.go
+++ b/cmd/helm/root_unix_test.go
@@ -19,7 +19,7 @@ limitations under the License.
 package main
 
 import (
-	"bytes"
+	"bufio"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -28,6 +28,14 @@ import (
 )
 
 func TestCheckPerms(t *testing.T) {
+	// NOTE(bacongobbler): have to open a new file handler here as the default os.Sterr cannot be read from
+	stderr, err := os.Open("/dev/stderr")
+	if err != nil {
+		t.Fatalf("could not open /dev/stderr for reading: %s", err)
+	}
+	defer stderr.Close()
+	reader := bufio.NewReader(stderr)
+
 	tdir, err := ioutil.TempDir("", "helmtest")
 	if err != nil {
 		t.Fatal(err)
@@ -43,21 +51,26 @@ func TestCheckPerms(t *testing.T) {
 	settings.KubeConfig = tfile
 	defer func() { settings.KubeConfig = tconfig }()
 
-	var b bytes.Buffer
-	checkPerms(&b)
+	checkPerms()
+	text, err := reader.ReadString('\n')
+	if err != nil {
+		t.Fatalf("could not read from stderr: %s", err)
+	}
 	expectPrefix := "WARNING: Kubernetes configuration file is group-readable. This is insecure. Location:"
-	if !strings.HasPrefix(b.String(), expectPrefix) {
-		t.Errorf("Expected to get a warning for group perms. Got %q", b.String())
+	if !strings.HasPrefix(text, expectPrefix) {
+		t.Errorf("Expected to get a warning for group perms. Got %q", text)
 	}
 
 	if err := fh.Chmod(0404); err != nil {
 		t.Errorf("Could not change mode on file: %s", err)
 	}
-	b.Reset()
-	checkPerms(&b)
+	checkPerms()
+	text, err = reader.ReadString('\n')
+	if err != nil {
+		t.Fatalf("could not read from stderr: %s", err)
+	}
 	expectPrefix = "WARNING: Kubernetes configuration file is world-readable. This is insecure. Location:"
-	if !strings.HasPrefix(b.String(), expectPrefix) {
-		t.Errorf("Expected to get a warning for world perms. Got %q", b.String())
+	if !strings.HasPrefix(text, expectPrefix) {
+		t.Errorf("Expected to get a warning for world perms. Got %q", text)
 	}
-
 }
diff --git a/cmd/helm/testdata/output/repo-add2.txt b/cmd/helm/testdata/output/repo-add2.txt
deleted file mode 100644
index 263ffa9e4..000000000
--- a/cmd/helm/testdata/output/repo-add2.txt
+++ /dev/null
@@ -1,1 +0,0 @@
-"test-name" already exists with the same configuration, skipping