Adding support merging new custom labels with original release labels during upgrade

Signed-off-by: Dmitry Chepurovskiy <dm3ch@dm3ch.net>
Signed-off-by: Dmitry Chepurovskiy <me@dm3ch.net>

cmd/helm/install.go

@@ -155,7 +155,7 @@ func addInstallFlags(cmd *cobra.Command, f *pflag.FlagSet, client *action.Instal
 	f.BoolVar(&client.Atomic, "atomic", false, "if set, the installation process deletes the installation on failure. The --wait flag will be set automatically if --atomic is used")
 	f.BoolVar(&client.SkipCRDs, "skip-crds", false, "if set, no CRDs will be installed. By default, CRDs are installed if not already present")
 	f.BoolVar(&client.SubNotes, "render-subchart-notes", false, "if set, render subchart notes along with the parent")
-	f.StringToStringVarP(&client.Labels, "labels", "l", nil, "Labels that would be added to relese metadata. Should be divided by comma. (Currently works only with configmap and secret storage drivers). Original release labels wouldn't persisted in upgraded release.")
+	f.StringToStringVarP(&client.Labels, "labels", "l", nil, "Labels that would be added to relese metadata. Should be divided by comma. (Currently works only with configmap and secret storage drivers).")
 	addValueOptionsFlags(f, valueOpts)
 	addChartPathOptionsFlags(f, &client.ChartPathOptions)
 

cmd/helm/upgrade.go

@@ -229,7 +229,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 	f.IntVar(&client.MaxHistory, "history-max", settings.MaxHistory, "limit the maximum number of revisions saved per release. Use 0 for no limit")
 	f.BoolVar(&client.CleanupOnFail, "cleanup-on-fail", false, "allow deletion of new resources created in this upgrade when upgrade fails")
 	f.BoolVar(&client.SubNotes, "render-subchart-notes", false, "if set, render subchart notes along with the parent")
-	f.StringToStringVarP(&client.Labels, "labels", "l", nil, "Labels that would be added to relese metadata. Should be divided by comma. (Currently works only with configmap and secret storage drivers). Original release labels wouldn't persisted in upgraded release.")
+	f.StringToStringVarP(&client.Labels, "labels", "l", nil, "Labels that would be added to relese metadata. Should be divided by comma. (Currently works only with configmap and secret storage drivers). Original release labels would be merged with upgrade labels. You can unset label using null.")
 	f.StringVar(&client.Description, "description", "", "add a custom description")
 	f.BoolVar(&client.DependencyUpdate, "dependency-update", false, "update dependencies if they are missing before installing the chart")
 	addChartPathOptionsFlags(f, &client.ChartPathOptions)

pkg/action/upgrade.go

@@ -252,7 +252,7 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[strin
 		Version:  revision,
 		Manifest: manifestDoc.String(),
 		Hooks:    hooks,
-		Labels:   u.Labels,
+		Labels:   mergeCustomLabels(lastRelease.Labels, u.Labels),
 	}
 
 	if len(notesTxt) > 0 {
@@ -571,3 +571,13 @@ func objectKey(r *resource.Info) string {
 	gvk := r.Object.GetObjectKind().GroupVersionKind()
 	return fmt.Sprintf("%s/%s/%s/%s", gvk.GroupVersion().String(), gvk.Kind, r.Namespace, r.Name)
 }
+
+func mergeCustomLabels(current, desired map[string]string) map[string]string {
+	labels := mergeStrStrMaps(current, desired)
+	for k, v := range labels {
+		if v == "null" {
+			delete(labels, k)
+		}
+	}
+	return labels
+}

pkg/action/upgrade_test.go

@@ -19,6 +19,7 @@ package action
 import (
 	"context"
 	"fmt"
+	"reflect"
 	"testing"
 	"time"
 
@@ -388,6 +389,21 @@ func TestUpgradeRelease_Interrupted_Atomic(t *testing.T) {
 	is.Equal(updatedRes.Info.Status, release.StatusDeployed)
 }
 
+func TestMergeCustomLabels(t *testing.T) {
+	var tests = [][3]map[string]string{
+		{nil, nil, map[string]string{}},
+		{map[string]string{}, map[string]string{}, map[string]string{}},
+		{map[string]string{"k1": "v1", "k2": "v2"}, nil, map[string]string{"k1": "v1", "k2": "v2"}},
+		{nil, map[string]string{"k1": "v1", "k2": "v2"}, map[string]string{"k1": "v1", "k2": "v2"}},
+		{map[string]string{"k1": "v1", "k2": "v2"}, map[string]string{"k1": "null", "k2": "v3"}, map[string]string{"k2": "v3"}},
+	}
+	for _, test := range tests {
+		if output := mergeCustomLabels(test[0], test[1]); !reflect.DeepEqual(test[2], output) {
+			t.Errorf("Expected {%v}, got {%v}", test[2], output)
+		}
+	}
+}
+
 func TestUpgradeRelease_Labels(t *testing.T) {
 	is := assert.New(t)
 	upAction := upgradeAction(t)
@@ -395,10 +411,9 @@ func TestUpgradeRelease_Labels(t *testing.T) {
 	rel := releaseStub()
 	rel.Name = "labels"
 	// It's needed to check that suppressed release would keep original labels
-	// Also it's needed for check that original release labels not passed to upgraded release, cause right now this functionality is not implemented and would cause problems with deletion of labels (meant nullifing existing labels)
 	rel.Labels = map[string]string{
 		"key1": "val1",
-		"key2": "val2",
+		"key2": "val2.1",
 	}
 	rel.Info.Status = release.StatusDeployed
 
@@ -406,14 +421,15 @@ func TestUpgradeRelease_Labels(t *testing.T) {
 	is.NoError(err)
 
 	upAction.Labels = map[string]string{
+		"key1": "null",
+		"key2": "val2.2",
 		"key3": "val3",
-		"key4": "val4",
 	}
 	// setting newValues and upgrading
 	res, err := upAction.Run(rel.Name, buildChart(), nil)
 	is.NoError(err)
 
-	// Now make sure it is actually upgraded
+	// Now make sure it is actually upgraded and labels were merged
 	updatedRes, err := upAction.cfg.Releases.Get(res.Name, 2)
 	is.NoError(err)
 
@@ -422,7 +438,7 @@ func TestUpgradeRelease_Labels(t *testing.T) {
 		return
 	}
 	is.Equal(release.StatusDeployed, updatedRes.Info.Status)
-	is.Equal(upAction.Labels, updatedRes.Labels)
+	is.Equal(mergeCustomLabels(rel.Labels, upAction.Labels), updatedRes.Labels)
 
 	// Now make sure it is suppressed release still contains original labels
 	initialRes, err := upAction.cfg.Releases.Get(res.Name, 1)