Repository: helm/helm
Pull request #8559 (2020-08-05 14:12:04)

Improve the console output for resource policy keep to align with helm2.
Signed-off-by: Du Zheng <zsuzhengdu@gmail.com>

**What this PR does / why we need it**:

Improve the console output for resources with annotation `"helm.sh/resource-policy": keep` to align with helm2 [outputs](https://github.com/helm/helm/blob/release-2.16/pkg/tiller/resource_policy.go#L57).

In helm3, following is print out when uninstall or delete a release with `kind: Secret`:
```bash
manifest-1
manifest-2
manifest-3
manifest-4
manifest-6
manifest-7
manifest-8
```

Improved:
```bash
These resources were kept due to the resource policy:
[Secret] Metadata.Name
[Secret] Metadata.Name
[Secret] Metadata.Name
[Secret] Metadata.Name
[Secret] Metadata.Name
[Secret] Metadata.Name
[Secret] Metadata.Name
```


bacongobbler: ping.
zsuzhengdu: Hey @bacongobbler Sorry for getting back to you late!

Added unit test to check the log message is formatted as expected. Please help to take a look and thank you very much!

Patch:
diff --git a/pkg/action/uninstall.go b/pkg/action/uninstall.go
index c466c6ee2..c762159cb 100644
--- a/pkg/action/uninstall.go
+++ b/pkg/action/uninstall.go
@@ -111,6 +111,10 @@ func (u *Uninstall) Run(name string) (*release.UninstallReleaseResponse, error)
 	}
 
 	kept, errs := u.deleteRelease(rel)
+
+	if kept != "" {
+		kept = "These resources were kept due to the resource policy:\n" + kept
+	}
 	res.Info = kept
 
 	if !u.DisableHooks {
@@ -189,7 +193,7 @@ func (u *Uninstall) deleteRelease(rel *release.Release) (string, []error) {
 	filesToKeep, filesToDelete := filterManifestsToKeep(files)
 	var kept string
 	for _, f := range filesToKeep {
-		kept += f.Name + "\n"
+		kept += "[" + f.Head.Kind + "] " + f.Head.Metadata.Name + "\n"
 	}
 
 	var builder strings.Builder
diff --git a/pkg/chart/loader/load.go b/pkg/chart/loader/load.go
index 6ed3b4187..c9d57234e 100644
--- a/pkg/chart/loader/load.go
+++ b/pkg/chart/loader/load.go
@@ -123,9 +123,6 @@ func LoadFiles(files []*BufferedFile) (*chart.Chart, error) {
 			if err := yaml.Unmarshal(f.Data, &c.Lock); err != nil {
 				return c, errors.Wrap(err, "cannot load requirements.lock")
 			}
-			if c.Metadata == nil {
-				c.Metadata = new(chart.Metadata)
-			}
 			if c.Metadata.APIVersion == chart.APIVersionV1 {
 				c.Files = append(c.Files, &chart.File{Name: f.Name, Data: f.Data})
 			}
diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go
index 145244082..0efac37bc 100644
--- a/pkg/downloader/manager.go
+++ b/pkg/downloader/manager.go
@@ -453,12 +453,6 @@ func (m *Manager) ensureMissingRepos(repoNames map[string]string, deps []*chart.
 
 	for _, dd := range deps {
 
-		// If the chart is in the local charts directory no repository needs
-		// to be specified.
-		if dd.Repository == "" {
-			continue
-		}
-
 		// When the repoName for a dependency is known we can skip ensuring
 		if _, ok := repoNames[dd.Name]; ok {
 			continue
diff --git a/pkg/kube/client.go b/pkg/kube/client.go
index 6fd3336c9..83bebf51f 100644
--- a/pkg/kube/client.go
+++ b/pkg/kube/client.go
@@ -43,7 +43,6 @@ import (
 	"k8s.io/apimachinery/pkg/watch"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 	"k8s.io/cli-runtime/pkg/resource"
-	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/scheme"
 	cachetools "k8s.io/client-go/tools/cache"
 	watchtools "k8s.io/client-go/tools/watch"
@@ -61,8 +60,6 @@ type Client struct {
 	Log     func(string, ...interface{})
 	// Namespace allows to bypass the kubeconfig file for the choice of the namespace
 	Namespace string
-
-	kubeClient *kubernetes.Clientset
 }
 
 var addToScheme sync.Once
@@ -90,19 +87,9 @@ func New(getter genericclioptions.RESTClientGetter) *Client {
 
 var nopLogger = func(_ string, _ ...interface{}) {}
 
-// getKubeClient get or create a new KubernetesClientSet
-func (c *Client) getKubeClient() (*kubernetes.Clientset, error) {
-	var err error
-	if c.kubeClient == nil {
-		c.kubeClient, err = c.Factory.KubernetesClientSet()
-	}
-
-	return c.kubeClient, err
-}
-
 // IsReachable tests connectivity to the cluster
 func (c *Client) IsReachable() error {
-	client, err := c.getKubeClient()
+	client, err := c.Factory.KubernetesClientSet()
 	if err == genericclioptions.ErrEmptyConfig {
 		// re-replace kubernetes ErrEmptyConfig error with a friendy error
 		// moar workarounds for Kubernetes API breaking.
@@ -128,7 +115,7 @@ func (c *Client) Create(resources ResourceList) (*Result, error) {
 
 // Wait up to the given timeout for the specified resources to be ready
 func (c *Client) Wait(resources ResourceList, timeout time.Duration) error {
-	cs, err := c.getKubeClient()
+	cs, err := c.Factory.KubernetesClientSet()
 	if err != nil {
 		return err
 	}
@@ -585,7 +572,7 @@ func scrubValidationError(err error) error {
 // WaitAndGetCompletedPodPhase waits up to a timeout until a pod enters a completed phase
 // and returns said phase (PodSucceeded or PodFailed qualify).
 func (c *Client) WaitAndGetCompletedPodPhase(name string, timeout time.Duration) (v1.PodPhase, error) {
-	client, err := c.getKubeClient()
+	client, err := c.Factory.KubernetesClientSet()
 	if err != nil {
 		return v1.PodUnknown, err
 	}


Test patch:
diff --git a/pkg/action/uninstall_test.go b/pkg/action/uninstall_test.go
new file mode 100644
index 000000000..53c3bf8f9
--- /dev/null
+++ b/pkg/action/uninstall_test.go
@@ -0,0 +1,62 @@
+/*
+Copyright The Helm Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package action
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func uninstallAction(t *testing.T) *Uninstall {
+	config := actionConfigFixture(t)
+	unAction := NewUninstall(config)
+	return unAction
+}
+
+func TestUninstallRelease_deleteRelease(t *testing.T) {
+	is := assert.New(t)
+
+	unAction := uninstallAction(t)
+	unAction.DisableHooks = true
+	unAction.DryRun = false
+	unAction.KeepHistory = true
+
+	rel := releaseStub()
+	rel.Name = "keep-secret"
+	rel.Manifest = `{
+		"apiVersion": "v1",
+		"kind": "Secret",
+		"metadata": {
+		  "name": "secret",
+		  "annotations": {
+			"helm.sh/resource-policy": "keep"
+		  }
+		},
+		"type": "Opaque",
+		"data": {
+		  "password": "password"
+		}
+	}`
+	unAction.cfg.Releases.Create(rel)
+	res, err := unAction.Run(rel.Name)
+	is.NoError(err)
+	expected := `These resources were kept due to the resource policy:
+[Secret] secret
+`
+	is.Contains(res.Info, expected)
+}
diff --git a/pkg/chart/loader/load_test.go b/pkg/chart/loader/load_test.go
index 3819161d2..f3456b9f6 100644
--- a/pkg/chart/loader/load_test.go
+++ b/pkg/chart/loader/load_test.go
@@ -206,32 +206,6 @@ func TestLoadFile(t *testing.T) {
 	verifyDependencies(t, c)
 }
 
-func TestLoadFiles_BadCases(t *testing.T) {
-	for _, tt := range []struct {
-		name          string
-		bufferedFiles []*BufferedFile
-		expectError   string
-	}{
-		{
-			name: "These files contain only requirements.lock",
-			bufferedFiles: []*BufferedFile{
-				{
-					Name: "requirements.lock",
-					Data: []byte(""),
-				},
-			},
-			expectError: "validation: chart.metadata.apiVersion is required"},
-	} {
-		_, err := LoadFiles(tt.bufferedFiles)
-		if err == nil {
-			t.Fatal("expected error when load illegal files")
-		}
-		if !strings.Contains(err.Error(), tt.expectError) {
-			t.Errorf("Expected error to contain %q, got %q for %s", tt.expectError, err.Error(), tt.name)
-		}
-	}
-}
-
 func TestLoadFiles(t *testing.T) {
 	goodFiles := []*BufferedFile{
 		{
diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go
index fc8d9abb2..849cf185a 100644
--- a/pkg/downloader/manager_test.go
+++ b/pkg/downloader/manager_test.go
@@ -249,76 +249,6 @@ func TestUpdateBeforeBuild(t *testing.T) {
 	}
 }
 
-// TestUpdateWithNoRepo is for the case of a dependency that has no repo listed.
-// This happens when the dependency is in the charts directory and does not need
-// to be fetched.
-func TestUpdateWithNoRepo(t *testing.T) {
-	// Set up a fake repo
-	srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer srv.Stop()
-	if err := srv.LinkIndices(); err != nil {
-		t.Fatal(err)
-	}
-	dir := func(p ...string) string {
-		return filepath.Join(append([]string{srv.Root()}, p...)...)
-	}
-
-	// Setup the dependent chart
-	d := &chart.Chart{
-		Metadata: &chart.Metadata{
-			Name:       "dep-chart",
-			Version:    "0.1.0",
-			APIVersion: "v1",
-		},
-	}
-
-	// Save a chart with the dependency
-	c := &chart.Chart{
-		Metadata: &chart.Metadata{
-			Name:       "with-dependency",
-			Version:    "0.1.0",
-			APIVersion: "v2",
-			Dependencies: []*chart.Dependency{{
-				Name:    d.Metadata.Name,
-				Version: "0.1.0",
-			}},
-		},
-	}
-	if err := chartutil.SaveDir(c, dir()); err != nil {
-		t.Fatal(err)
-	}
-
-	// Save dependent chart into the parents charts directory. If the chart is
-	// not in the charts directory Helm will return an error that it is not
-	// found.
-	if err := chartutil.SaveDir(d, dir(c.Metadata.Name, "charts")); err != nil {
-		t.Fatal(err)
-	}
-
-	// Set-up a manager
-	b := bytes.NewBuffer(nil)
-	g := getter.Providers{getter.Provider{
-		Schemes: []string{"http", "https"},
-		New:     getter.NewHTTPGetter,
-	}}
-	m := &Manager{
-		ChartPath:        dir(c.Metadata.Name),
-		Out:              b,
-		Getters:          g,
-		RepositoryConfig: dir("repositories.yaml"),
-		RepositoryCache:  dir(),
-	}
-
-	// Test the update
-	err = m.Update()
-	if err != nil {
-		t.Fatal(err)
-	}
-}
-
 // This function is the skeleton test code of failing tests for #6416 and #6871 and bugs due to #5874.
 //
 // This function is used by below tests that ensures success of build operation