Repository: helm/helm
Pull request #8532 (2020-07-29 20:31:57)

Adding filtering of helm releases for configmap and secret backends during list command
<!--  Thanks for sending a pull request!  Here are some tips for you:
1. Make sure to read the Contributing Guide before submitting your PR: https://github.com/helm/helm/blob/master/CONTRIBUTING.md
2. If this PR closes another issue, add 'closes #<issue number>' somewhere in the PR summary. GitHub will automatically close that issue when this PR gets merged. Alternatively, adding 'refs #<issue number>' will not close the issue, but help provide the reviewer more context.-->

**What this PR does / why we need it**:
Adding labels filtering to list command.
Filtering behaves like k8s does.
For now only secrets and configmaps backends supported. Other backends not supported.
/resolves https://github.com/helm/helm/issues/4639

**Special notes for your reviewer**:
I would like some help with unit testing if that's possible.
Also I need some consultation if current behaviour would be fine for sql and in-memory storage backends.
**If applicable**:
- [ ] this PR contains documentation
- [x] this PR contains unit tests
- [x] this PR has been tested for backwards compatibility


dm3ch: > Also, do you mind adding/updating the unit tests to test for this?

I like tests but I'm not a good test writer, so I'll try but I'm not sure if I would succeed.

What types of tests does helm have only unit or you also have some sort of end2end tests?
bacongobbler: Both, though for this pull request we're asking for unit tests.

Have a look at the tests that exist under `pkg/storage` and `pkg/storage/driver` for some inspiration.
dm3ch: Added selector specific tests for list action.

For configmap and secret drivers I don't see any proper way to test changes, cause currently code that allows labeling via helm itself is not supported, so labels getting check is hard.

End to end test for cms/list.go can't be done because we currently needs some side methods of manipulating labels depends on backend.

Patch:
diff --git a/cmd/helm/list.go b/cmd/helm/list.go
index 18e5ffe45..2f40707b3 100644
--- a/cmd/helm/list.go
+++ b/cmd/helm/list.go
@@ -126,6 +126,7 @@ func newListCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 	f.IntVarP(&client.Limit, "max", "m", 256, "maximum number of releases to fetch")
 	f.IntVar(&client.Offset, "offset", 0, "next release name in the list, used to offset from start value")
 	f.StringVarP(&client.Filter, "filter", "f", "", "a regular expression (Perl compatible). Any releases that match the expression will be included in the results")
+	f.StringVarP(&client.Selector, "selector", "l", "", "Selector (label query) to filter on, supports '=', '==', and '!='.(e.g. -l key1=value1,key2=value2). Works only for secret(default) and configmap storage backends.")
 	bindOutputFlag(cmd, &outfmt)
 
 	return cmd
diff --git a/pkg/action/list.go b/pkg/action/list.go
index 0f85de519..5ba0c4770 100644
--- a/pkg/action/list.go
+++ b/pkg/action/list.go
@@ -20,6 +20,8 @@ import (
 	"path"
 	"regexp"
 
+	"k8s.io/apimachinery/pkg/labels"
+
 	"helm.sh/helm/v3/pkg/release"
 	"helm.sh/helm/v3/pkg/releaseutil"
 )
@@ -126,6 +128,7 @@ type List struct {
 	Deployed     bool
 	Failed       bool
 	Pending      bool
+	Selector     string
 }
 
 // NewList constructs a new *List
@@ -156,6 +159,7 @@ func (l *List) Run() ([]*release.Release, error) {
 		if filter != nil && !filter.MatchString(rel.Name) {
 			return false
 		}
+
 		return true
 	})
 
@@ -178,6 +182,13 @@ func (l *List) Run() ([]*release.Release, error) {
 	// latest releases, otherwise outdated entries can be returned
 	results = l.filterStateMask(results)
 
+	// Skip anything that doesn't match the selector
+	selectorObj, err := labels.Parse(l.Selector)
+	if err != nil {
+		return nil, err
+	}
+	results = l.filterSelector(results, selectorObj)
+
 	// Unfortunately, we have to sort before truncating, which can incur substantial overhead
 	l.sort(results)
 
@@ -260,6 +271,18 @@ func (l *List) filterStateMask(releases []*release.Release) []*release.Release {
 	return desiredStateReleases
 }
 
+func (l *List) filterSelector(releases []*release.Release, selector labels.Selector) []*release.Release {
+	desiredStateReleases := make([]*release.Release, 0)
+
+	for _, rls := range releases {
+		if selector.Matches(labels.Set(rls.Labels)) {
+			desiredStateReleases = append(desiredStateReleases, rls)
+		}
+	}
+
+	return desiredStateReleases
+}
+
 // SetStateMask calculates the state mask based on parameters.
 func (l *List) SetStateMask() {
 	if l.All {
diff --git a/pkg/release/release.go b/pkg/release/release.go
index 8582a86f3..b90612873 100644
--- a/pkg/release/release.go
+++ b/pkg/release/release.go
@@ -37,6 +37,9 @@ type Release struct {
 	Version int `json:"version,omitempty"`
 	// Namespace is the kubernetes namespace of the release.
 	Namespace string `json:"namespace,omitempty"`
+	// Labels of the release.
+	// Disabled encoding into Json cause labels are stored in storage driver metadata field.
+	Labels map[string]string `json:"-"`
 }
 
 // SetStatus is a helper for setting the status on a release.
diff --git a/pkg/storage/driver/cfgmaps.go b/pkg/storage/driver/cfgmaps.go
index 71e635975..94c278875 100644
--- a/pkg/storage/driver/cfgmaps.go
+++ b/pkg/storage/driver/cfgmaps.go
@@ -105,6 +105,9 @@ func (cfgmaps *ConfigMaps) List(filter func(*rspb.Release) bool) ([]*rspb.Releas
 			cfgmaps.Log("list: failed to decode release: %v: %s", item, err)
 			continue
 		}
+
+		rls.Labels = item.ObjectMeta.Labels
+
 		if filter(rls) {
 			results = append(results, rls)
 		}
diff --git a/pkg/storage/driver/secrets.go b/pkg/storage/driver/secrets.go
index 44280f70f..2e8530d0c 100644
--- a/pkg/storage/driver/secrets.go
+++ b/pkg/storage/driver/secrets.go
@@ -97,6 +97,9 @@ func (secrets *Secrets) List(filter func(*rspb.Release) bool) ([]*rspb.Release,
 			secrets.Log("list: failed to decode release: %v: %s", item, err)
 			continue
 		}
+
+		rls.Labels = item.ObjectMeta.Labels
+
 		if filter(rls) {
 			results = append(results, rls)
 		}


Test patch:
diff --git a/pkg/action/list_test.go b/pkg/action/list_test.go
index b8e2ece68..73009d523 100644
--- a/pkg/action/list_test.go
+++ b/pkg/action/list_test.go
@@ -320,3 +320,49 @@ func TestFilterLatestReleases(t *testing.T) {
 		assert.ElementsMatch(t, expectedFilteredList, filteredList)
 	})
 }
+
+func TestSelectorList(t *testing.T) {
+	r1 := releaseStub()
+	r1.Name = "r1"
+	r1.Version = 1
+	r1.Labels = map[string]string{"key": "value1"}
+	r2 := releaseStub()
+	r2.Name = "r2"
+	r2.Version = 1
+	r2.Labels = map[string]string{"key": "value2"}
+	r3 := releaseStub()
+	r3.Name = "r3"
+	r3.Version = 1
+	r3.Labels = map[string]string{}
+
+	lister := newListFixture(t)
+	for _, rel := range []*release.Release{r1, r2, r3} {
+		if err := lister.cfg.Releases.Create(rel); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	t.Run("should fail selector parsing", func(t *testing.T) {
+		is := assert.New(t)
+		lister.Selector = "a?=b"
+
+		_, err := lister.Run()
+		is.Error(err)
+	})
+
+	t.Run("should select one release with matching label", func(t *testing.T) {
+		lister.Selector = "key==value1"
+		res, _ := lister.Run()
+
+		expectedFilteredList := []*release.Release{r1}
+		assert.ElementsMatch(t, expectedFilteredList, res)
+	})
+
+	t.Run("should select two releases with non matching label", func(t *testing.T) {
+		lister.Selector = "key!=value1"
+		res, _ := lister.Run()
+
+		expectedFilteredList := []*release.Release{r2, r3}
+		assert.ElementsMatch(t, expectedFilteredList, res)
+	})
+}