Repository: helm/helm
Pull request #8727 (2020-09-14 11:46:28)

fix(comp): Disable file comp for output formats
**What this PR does / why we need it**:

It does not make sense to suggest files to the user as output formats.
This PR fixed that.

Currently:
```
bash-5.0$ bin/helm status --output <TAB>
json   table  yaml
bash-5.0$ bin/helm status --output go.<TAB>
go.mod  go.sum
```
Notice that the `go.*` files are mistakenly suggested when the real completions don't match.
With this PR:
```
bash-5.0$ bin/helm status --output <TAB>
json   table  yaml
bash-5.0$ bin/helm status --output go.<TAB>
# No suggestions
```

Unit test has been corrected.


Patch:
diff --git a/Makefile b/Makefile
index 97f99fd86..a9eac8033 100644
--- a/Makefile
+++ b/Makefile
@@ -1,4 +1,5 @@
 BINDIR      := $(CURDIR)/bin
+INSTALL_PATH ?= /usr/local/bin
 DIST_DIRS   := find * -type d -exec
 TARGETS     := darwin/amd64 linux/amd64 linux/386 linux/arm linux/arm64 linux/ppc64le linux/s390x windows/amd64
 TARGET_OBJS ?= darwin-amd64.tar.gz darwin-amd64.tar.gz.sha256 darwin-amd64.tar.gz.sha256sum linux-amd64.tar.gz linux-amd64.tar.gz.sha256 linux-amd64.tar.gz.sha256sum linux-386.tar.gz linux-386.tar.gz.sha256 linux-386.tar.gz.sha256sum linux-arm.tar.gz linux-arm.tar.gz.sha256 linux-arm.tar.gz.sha256sum linux-arm64.tar.gz linux-arm64.tar.gz.sha256 linux-arm64.tar.gz.sha256sum linux-ppc64le.tar.gz linux-ppc64le.tar.gz.sha256 linux-ppc64le.tar.gz.sha256sum linux-s390x.tar.gz linux-s390x.tar.gz.sha256 linux-s390x.tar.gz.sha256sum windows-amd64.zip windows-amd64.zip.sha256 windows-amd64.zip.sha256sum
@@ -49,6 +50,7 @@ endif
 LDFLAGS += -X helm.sh/helm/v3/internal/version.metadata=${VERSION_METADATA}
 LDFLAGS += -X helm.sh/helm/v3/internal/version.gitCommit=${GIT_COMMIT}
 LDFLAGS += -X helm.sh/helm/v3/internal/version.gitTreeState=${GIT_DIRTY}
+LDFLAGS += $(EXT_LDFLAGS)
 
 .PHONY: all
 all: build
@@ -62,6 +64,13 @@ build: $(BINDIR)/$(BINNAME)
 $(BINDIR)/$(BINNAME): $(SRC)
 	GO111MODULE=on go build $(GOFLAGS) -tags '$(TAGS)' -ldflags '$(LDFLAGS)' -o '$(BINDIR)'/$(BINNAME) ./cmd/helm
 
+# ------------------------------------------------------------------------------
+#  install
+
+.PHONY: install
+install: build
+	@install "$(BINDIR)/$(BINNAME)" "$(INSTALL_PATH)/$(BINNAME)"
+
 # ------------------------------------------------------------------------------
 #  test
 
diff --git a/cmd/helm/flags.go b/cmd/helm/flags.go
index 544fb7608..d1329c279 100644
--- a/cmd/helm/flags.go
+++ b/cmd/helm/flags.go
@@ -69,7 +69,7 @@ func bindOutputFlag(cmd *cobra.Command, varRef *output.Format) {
 				formatNames = append(formatNames, format)
 			}
 		}
-		return formatNames, cobra.ShellCompDirectiveDefault
+		return formatNames, cobra.ShellCompDirectiveNoFileComp
 	})
 
 	if err != nil {
diff --git a/pkg/action/install.go b/pkg/action/install.go
index 00fb208b0..9bfecd915 100644
--- a/pkg/action/install.go
+++ b/pkg/action/install.go
@@ -654,8 +654,8 @@ func (c *ChartPathOptions) LocateChart(name string, settings *cli.EnvSettings) (
 		dl.Verify = downloader.VerifyAlways
 	}
 	if c.RepoURL != "" {
-		chartURL, err := repo.FindChartInAuthRepoURL(c.RepoURL, c.Username, c.Password, name, version,
-			c.CertFile, c.KeyFile, c.CaFile, getter.All(settings))
+		chartURL, err := repo.FindChartInAuthAndTLSRepoURL(c.RepoURL, c.Username, c.Password, name, version,
+			c.CertFile, c.KeyFile, c.CaFile, c.InsecureSkipTLSverify, getter.All(settings))
 		if err != nil {
 			return "", err
 		}
diff --git a/pkg/action/pull.go b/pkg/action/pull.go
index a46e98bae..220ca11b2 100644
--- a/pkg/action/pull.go
+++ b/pkg/action/pull.go
@@ -89,7 +89,7 @@ func (p *Pull) Run(chartRef string) (string, error) {
 	}
 
 	if p.RepoURL != "" {
-		chartURL, err := repo.FindChartInAuthRepoURL(p.RepoURL, p.Username, p.Password, chartRef, p.Version, p.CertFile, p.KeyFile, p.CaFile, getter.All(p.Settings))
+		chartURL, err := repo.FindChartInAuthAndTLSRepoURL(p.RepoURL, p.Username, p.Password, chartRef, p.Version, p.CertFile, p.KeyFile, p.CaFile, p.InsecureSkipTLSverify, getter.All(p.Settings))
 		if err != nil {
 			return out.String(), err
 		}
diff --git a/pkg/repo/chartrepo.go b/pkg/repo/chartrepo.go
index c2c366a1e..edb86eaeb 100644
--- a/pkg/repo/chartrepo.go
+++ b/pkg/repo/chartrepo.go
@@ -205,6 +205,14 @@ func FindChartInRepoURL(repoURL, chartName, chartVersion, certFile, keyFile, caF
 // without adding repo to repositories, like FindChartInRepoURL,
 // but it also receives credentials for the chart repository.
 func FindChartInAuthRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile string, getters getter.Providers) (string, error) {
+	return FindChartInAuthAndTLSRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile, false, getters)
+}
+
+// FindChartInAuthRepoURL finds chart in chart repository pointed by repoURL
+// without adding repo to repositories, like FindChartInRepoURL,
+// but it also receives credentials and TLS verify flag for the chart repository.
+// TODO Helm 4, FindChartInAuthAndTLSRepoURL should be integrated into FindChartInAuthRepoURL.
+func FindChartInAuthAndTLSRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile string, insecureSkipTLSverify bool, getters getter.Providers) (string, error) {
 
 	// Download and write the index file to a temporary location
 	buf := make([]byte, 20)
@@ -212,13 +220,14 @@ func FindChartInAuthRepoURL(repoURL, username, password, chartName, chartVersion
 	name := strings.ReplaceAll(base64.StdEncoding.EncodeToString(buf), "/", "-")
 
 	c := Entry{
-		URL:      repoURL,
-		Username: username,
-		Password: password,
-		CertFile: certFile,
-		KeyFile:  keyFile,
-		CAFile:   caFile,
-		Name:     name,
+		URL:                   repoURL,
+		Username:              username,
+		Password:              password,
+		CertFile:              certFile,
+		KeyFile:               keyFile,
+		CAFile:                caFile,
+		Name:                  name,
+		InsecureSkipTLSverify: insecureSkipTLSverify,
 	}
 	r, err := NewChartRepository(&c, getters)
 	if err != nil {


Test patch:
diff --git a/cmd/helm/dependency_build_test.go b/cmd/helm/dependency_build_test.go
index eeca12fa6..d6dfdabcb 100644
--- a/cmd/helm/dependency_build_test.go
+++ b/cmd/helm/dependency_build_test.go
@@ -28,7 +28,7 @@ import (
 )
 
 func TestDependencyBuildCmd(t *testing.T) {
-	srv, err := repotest.NewTempServer("testdata/testcharts/*.tgz")
+	srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testcharts/*.tgz")
 	defer srv.Stop()
 	if err != nil {
 		t.Fatal(err)
diff --git a/cmd/helm/dependency_update_test.go b/cmd/helm/dependency_update_test.go
index 1f9d55867..bf27c7b6c 100644
--- a/cmd/helm/dependency_update_test.go
+++ b/cmd/helm/dependency_update_test.go
@@ -33,7 +33,7 @@ import (
 )
 
 func TestDependencyUpdateCmd(t *testing.T) {
-	srv, err := repotest.NewTempServer("testdata/testcharts/*.tgz")
+	srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testcharts/*.tgz")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -121,7 +121,7 @@ func TestDependencyUpdateCmd_DontDeleteOldChartsOnError(t *testing.T) {
 	defer resetEnv()()
 	defer ensure.HelmHome(t)()
 
-	srv, err := repotest.NewTempServer("testdata/testcharts/*.tgz")
+	srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testcharts/*.tgz")
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/cmd/helm/pull_test.go b/cmd/helm/pull_test.go
index 3f769a1bc..1d439e873 100644
--- a/cmd/helm/pull_test.go
+++ b/cmd/helm/pull_test.go
@@ -26,7 +26,7 @@ import (
 )
 
 func TestPullCmd(t *testing.T) {
-	srv, err := repotest.NewTempServer("testdata/testcharts/*.tgz*")
+	srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testcharts/*.tgz*")
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/cmd/helm/repo_add_test.go b/cmd/helm/repo_add_test.go
index 9ef64390b..19281f3aa 100644
--- a/cmd/helm/repo_add_test.go
+++ b/cmd/helm/repo_add_test.go
@@ -34,7 +34,7 @@ import (
 )
 
 func TestRepoAddCmd(t *testing.T) {
-	srv, err := repotest.NewTempServer("testdata/testserver/*.*")
+	srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -53,7 +53,7 @@ func TestRepoAddCmd(t *testing.T) {
 }
 
 func TestRepoAdd(t *testing.T) {
-	ts, err := repotest.NewTempServer("testdata/testserver/*.*")
+	ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -118,7 +118,7 @@ func TestRepoAddConcurrentDirNotExist(t *testing.T) {
 }
 
 func repoAddConcurrent(t *testing.T, testName, repoFile string) {
-	ts, err := repotest.NewTempServer("testdata/testserver/*.*")
+	ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*")
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/cmd/helm/repo_remove_test.go b/cmd/helm/repo_remove_test.go
index 0ea1d63d2..cb5c6e9ab 100644
--- a/cmd/helm/repo_remove_test.go
+++ b/cmd/helm/repo_remove_test.go
@@ -30,7 +30,7 @@ import (
 )
 
 func TestRepoRemove(t *testing.T) {
-	ts, err := repotest.NewTempServer("testdata/testserver/*.*")
+	ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*")
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/cmd/helm/repo_update_test.go b/cmd/helm/repo_update_test.go
index e5e4eb337..4b16a1ea7 100644
--- a/cmd/helm/repo_update_test.go
+++ b/cmd/helm/repo_update_test.go
@@ -75,7 +75,7 @@ func TestUpdateCharts(t *testing.T) {
 	defer resetEnv()()
 	defer ensure.HelmHome(t)()
 
-	ts, err := repotest.NewTempServer("testdata/testserver/*.*")
+	ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*")
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/cmd/helm/show_test.go b/cmd/helm/show_test.go
index 2734faf5e..ac5294d3c 100644
--- a/cmd/helm/show_test.go
+++ b/cmd/helm/show_test.go
@@ -26,7 +26,7 @@ import (
 )
 
 func TestShowPreReleaseChart(t *testing.T) {
-	srv, err := repotest.NewTempServer("testdata/testcharts/*.tgz*")
+	srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testcharts/*.tgz*")
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/cmd/helm/testdata/output/output-comp.txt b/cmd/helm/testdata/output/output-comp.txt
index be574756b..e7799a56b 100644
--- a/cmd/helm/testdata/output/output-comp.txt
+++ b/cmd/helm/testdata/output/output-comp.txt
@@ -1,5 +1,5 @@
 table
 json
 yaml
-:0
-Completion ended with directive: ShellCompDirectiveDefault
+:4
+Completion ended with directive: ShellCompDirectiveNoFileComp
diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go
index 0cbdb162b..c05b4403d 100644
--- a/pkg/action/action_test.go
+++ b/pkg/action/action_test.go
@@ -20,6 +20,7 @@ import (
 	"flag"
 	"io/ioutil"
 	"net/http"
+	"os"
 	"path/filepath"
 	"testing"
 
@@ -56,6 +57,8 @@ func actionConfigFixture(t *testing.T) *Configuration {
 		t.Fatal(err)
 	}
 
+	t.Cleanup(func() { os.RemoveAll(tdir) })
+
 	cache, err := registry.NewCache(
 		registry.CacheOptDebug(true),
 		registry.CacheOptRoot(filepath.Join(tdir, registry.CacheRootDir)),
diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go
index abfb007ff..b456143a1 100644
--- a/pkg/downloader/chart_downloader_test.go
+++ b/pkg/downloader/chart_downloader_test.go
@@ -172,7 +172,7 @@ func TestIsTar(t *testing.T) {
 
 func TestDownloadTo(t *testing.T) {
 	// Set up a fake repo with basic auth enabled
-	srv, err := repotest.NewTempServer("testdata/*.tgz*")
+	srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*")
 	srv.Stop()
 	if err != nil {
 		t.Fatal(err)
@@ -229,7 +229,7 @@ func TestDownloadTo(t *testing.T) {
 
 func TestDownloadTo_TLS(t *testing.T) {
 	// Set up mock server w/ tls enabled
-	srv, err := repotest.NewTempServer("testdata/*.tgz*")
+	srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*")
 	srv.Stop()
 	if err != nil {
 		t.Fatal(err)
@@ -285,7 +285,7 @@ func TestDownloadTo_VerifyLater(t *testing.T) {
 	dest := ensure.TempDir(t)
 
 	// Set up a fake repo
-	srv, err := repotest.NewTempServer("testdata/*.tgz*")
+	srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*")
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go
index e60cf7624..9532cca7c 100644
--- a/pkg/downloader/manager_test.go
+++ b/pkg/downloader/manager_test.go
@@ -183,7 +183,7 @@ func TestGetRepoNames(t *testing.T) {
 
 func TestUpdateBeforeBuild(t *testing.T) {
 	// Set up a fake repo
-	srv, err := repotest.NewTempServer("testdata/*.tgz*")
+	srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -257,7 +257,7 @@ func TestUpdateBeforeBuild(t *testing.T) {
 // If each of these main fields (name, version, repository) is not supplied by dep param, default value will be used.
 func checkBuildWithOptionalFields(t *testing.T, chartName string, dep chart.Dependency) {
 	// Set up a fake repo
-	srv, err := repotest.NewTempServer("testdata/*.tgz*")
+	srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*")
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go
index 568afa094..de5358aee 100644
--- a/pkg/kube/client_test.go
+++ b/pkg/kube/client_test.go
@@ -91,9 +91,12 @@ func newResponse(code int, obj runtime.Object) (*http.Response, error) {
 	return &http.Response{StatusCode: code, Header: header, Body: body}, nil
 }
 
-func newTestClient() *Client {
+func newTestClient(t *testing.T) *Client {
+	testFactory := cmdtesting.NewTestFactory()
+	t.Cleanup(testFactory.Cleanup)
+
 	return &Client{
-		Factory: cmdtesting.NewTestFactory().WithNamespace("default"),
+		Factory: testFactory.WithNamespace("default"),
 		Log:     nopLogger,
 	}
 }
@@ -107,7 +110,7 @@ func TestUpdate(t *testing.T) {
 
 	var actions []string
 
-	c := newTestClient()
+	c := newTestClient(t)
 	c.Factory.(*cmdtesting.TestFactory).UnstructuredClient = &fake.RESTClient{
 		NegotiatedSerializer: unstructuredSerializer,
 		Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
@@ -232,7 +235,7 @@ func TestBuild(t *testing.T) {
 		},
 	}
 
-	c := newTestClient()
+	c := newTestClient(t)
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			// Test for an invalid manifest
@@ -279,7 +282,7 @@ func TestPerform(t *testing.T) {
 				return nil
 			}
 
-			c := newTestClient()
+			c := newTestClient(t)
 			infos, err := c.Build(tt.reader, false)
 			if err != nil && err.Error() != tt.errMessage {
 				t.Errorf("Error while building manifests: %v", err)
diff --git a/pkg/repo/chartrepo_test.go b/pkg/repo/chartrepo_test.go
index eceb3009e..800d9ae55 100644
--- a/pkg/repo/chartrepo_test.go
+++ b/pkg/repo/chartrepo_test.go
@@ -276,6 +276,44 @@ func startLocalServerForTests(handler http.Handler) (*httptest.Server, error) {
 	return httptest.NewServer(handler), nil
 }
 
+// startLocalTLSServerForTests Start the local helm server with TLS
+func startLocalTLSServerForTests(handler http.Handler) (*httptest.Server, error) {
+	if handler == nil {
+		fileBytes, err := ioutil.ReadFile("testdata/local-index.yaml")
+		if err != nil {
+			return nil, err
+		}
+		handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Write(fileBytes)
+		})
+	}
+
+	return httptest.NewTLSServer(handler), nil
+}
+
+func TestFindChartInAuthAndTLSRepoURL(t *testing.T) {
+	srv, err := startLocalTLSServerForTests(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer srv.Close()
+
+	chartURL, err := FindChartInAuthAndTLSRepoURL(srv.URL, "", "", "nginx", "", "", "", "", true, getter.All(&cli.EnvSettings{}))
+	if err != nil {
+		t.Fatalf("%v", err)
+	}
+	if chartURL != "https://kubernetes-charts.storage.googleapis.com/nginx-0.2.0.tgz" {
+		t.Errorf("%s is not the valid URL", chartURL)
+	}
+
+	// If the insecureSkipTLsverify is false, it will return an error that contains "x509: certificate signed by unknown authority".
+	_, err = FindChartInAuthAndTLSRepoURL(srv.URL, "", "", "nginx", "0.1.0", "", "", "", false, getter.All(&cli.EnvSettings{}))
+
+	if !strings.Contains(err.Error(), "x509: certificate signed by unknown authority") {
+		t.Errorf("Expected TLS error for function  FindChartInAuthAndTLSRepoURL not found, but got a different error (%v)", err)
+	}
+}
+
 func TestFindChartInRepoURL(t *testing.T) {
 	srv, err := startLocalServerForTests(nil)
 	if err != nil {
diff --git a/pkg/repo/repotest/server.go b/pkg/repo/repotest/server.go
index b18bce49c..270c8958a 100644
--- a/pkg/repo/repotest/server.go
+++ b/pkg/repo/repotest/server.go
@@ -21,6 +21,7 @@ import (
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"testing"
 
 	"helm.sh/helm/v3/internal/tlsutil"
 
@@ -29,6 +30,19 @@ import (
 	"helm.sh/helm/v3/pkg/repo"
 )
 
+// NewTempServerWithCleanup creates a server inside of a temp dir.
+//
+// If the passed in string is not "", it will be treated as a shell glob, and files
+// will be copied from that path to the server's docroot.
+//
+// The caller is responsible for stopping the server.
+// The temp dir will be removed by testing package automatically when test finished.
+func NewTempServerWithCleanup(t *testing.T, glob string) (*Server, error) {
+	srv, err := NewTempServer(glob)
+	t.Cleanup(func() { os.RemoveAll(srv.docroot) })
+	return srv, err
+}
+
 // NewTempServer creates a server inside of a temp dir.
 //
 // If the passed in string is not "", it will be treated as a shell glob, and files
@@ -36,6 +50,8 @@ import (
 //
 // The caller is responsible for destroying the temp directory as well as stopping
 // the server.
+//
+// Deprecated: use NewTempServerWithCleanup
 func NewTempServer(glob string) (*Server, error) {
 	tdir, err := ioutil.TempDir("", "helm-repotest-")
 	if err != nil {
diff --git a/pkg/repo/repotest/server_test.go b/pkg/repo/repotest/server_test.go
index ee62791af..6d71071da 100644
--- a/pkg/repo/repotest/server_test.go
+++ b/pkg/repo/repotest/server_test.go
@@ -99,7 +99,7 @@ func TestServer(t *testing.T) {
 func TestNewTempServer(t *testing.T) {
 	defer ensure.HelmHome(t)()
 
-	srv, err := NewTempServer("testdata/examplechart-0.1.0.tgz")
+	srv, err := NewTempServerWithCleanup(t, "testdata/examplechart-0.1.0.tgz")
 	if err != nil {
 		t.Fatal(err)
 	}