Add option for adding labels only to service and/or template (#703)

* Add option for adding labels only to service and/or template

In this PR we are introducing 2 additional flags for setting label for
service(label-service) and revision(label-revision) only.

Signed-off-by: Andrew Su <asu@pivotal.io>

* Update docs

Signed-off-by: Andrew Su <asu@pivotal.io>

* Add changelog entry

Signed-off-by: Andrew Su <asu@pivotal.io>

* Refactor UpdateLabels method to be more generic.

* Refactored labels common code.

Signed-off-by: Shash Reddy <shashwathireddy@gmail.com>

* Pass pointer refernce instead of copy to update labels

Co-authored-by: Roland Huß <roland@ro14nd.de>
Co-authored-by: Andrew Su <asu@pivotal.io>

CHANGELOG.adoc

@@ -26,6 +26,10 @@
 | Replaced `kn source cron` with `kn source ping`. `--schedule` is not mandatory anymore and defaults to "* * * * *" (every minute)
 | https://github.com/knative/client/issues/564[#564]
 
+| ✨
+| Add option for adding labels only to service and/or template
+| https://github.com/knative/client/issues/675[#675]
+
 | ✨
 | Update to Knative serving 0.13.0 and Knative eventing 0.13.1
 | https://github.com/knative/client/issues/564[#564]

docs/cmd/kn_service_create.md

@@ -54,7 +54,9 @@ kn service create NAME --image IMAGE [flags]
       --force                        Create service forcefully, replaces existing service if any.
   -h, --help                         help for create
       --image string                 Image to run.
-  -l, --label stringArray         Service label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-).
+  -l, --label stringArray            Labels to set for both Service and Revision. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-).
+      --label-revision stringArray   Revision label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-). This flag takes precedence over "label" flag.
+      --label-service stringArray    Service label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-). This flag takes precedence over "label" flag.
       --limits-cpu string            The limits on the requested CPU (e.g., 1000m).
       --limits-memory string         The limits on the requested memory (e.g., 1024Mi).
       --lock-to-digest               Keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision) (default true)

docs/cmd/kn_service_update.md

@@ -49,7 +49,9 @@ kn service update NAME [flags]
       --env-from stringArray         Add environment variables from a ConfigMap (prefix cm: or config-map:) or a Secret (prefix secret:). Example: --env-from cm:myconfigmap or --env-from secret:mysecret. You can use this flag multiple times. To unset a ConfigMap/Secret reference, append "-" to the name, e.g. --env-from cm:myconfigmap-.
   -h, --help                         help for update
       --image string                 Image to run.
-  -l, --label stringArray         Service label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-).
+  -l, --label stringArray            Labels to set for both Service and Revision. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-).
+      --label-revision stringArray   Revision label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-). This flag takes precedence over "label" flag.
+      --label-service stringArray    Service label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-). This flag takes precedence over "label" flag.
       --limits-cpu string            The limits on the requested CPU (e.g., 1000m).
       --limits-memory string         The limits on the requested memory (e.g., 1024Mi).
       --lock-to-digest               Keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision) (default true)

pkg/kn/commands/service/configuration_edit_flags.go

@@ -20,8 +20,11 @@ import (
 
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
+
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 	"knative.dev/client/pkg/kn/flags"
 	servinglib "knative.dev/client/pkg/serving"
 	"knative.dev/client/pkg/util"
@@ -47,6 +50,8 @@ type ConfigurationEditFlags struct {
 	AutoscaleWindow            string
 	Port                       int32
 	Labels                     []string
+	LabelsService              []string
+	LabelsRevision             []string
 	NamePrefix                 string
 	RevisionName               string
 	ServiceAccountName         string
@@ -160,10 +165,22 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 	command.Flags().Int32VarP(&p.Port, "port", "p", 0, "The port where application listens on.")
 	p.markFlagMakesRevision("port")
 	command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{},
-		"Service label to set. name=value; you may provide this flag "+
+		"Labels to set for both Service and Revision. name=value; you may provide this flag "+
 			"any number of times to set multiple labels. "+
 			"To unset, specify the label name followed by a \"-\" (e.g., name-).")
 	p.markFlagMakesRevision("label")
+	command.Flags().StringArrayVarP(&p.LabelsService, "label-service", "", []string{},
+		"Service label to set. name=value; you may provide this flag "+
+			"any number of times to set multiple labels. "+
+			"To unset, specify the label name followed by a \"-\" (e.g., name-). This flag takes "+
+			"precedence over \"label\" flag.")
+	p.markFlagMakesRevision("label-service")
+	command.Flags().StringArrayVarP(&p.LabelsRevision, "label-revision", "", []string{},
+		"Revision label to set. name=value; you may provide this flag "+
+			"any number of times to set multiple labels. "+
+			"To unset, specify the label name followed by a \"-\" (e.g., name-). This flag takes "+
+			"precedence over \"label\" flag.")
+	p.markFlagMakesRevision("label-revision")
 	command.Flags().StringVar(&p.RevisionName, "revision-name", "{{.Service}}-{{.Random 5}}-{{.Generation}}",
 		"The revision name to set. Must start with the service name and a dash as a prefix. "+
 			"Empty revision name will result in the server generating a name for the revision. "+
@@ -362,16 +379,20 @@ func (p *ConfigurationEditFlags) Apply(
 		}
 	}
 
-	if cmd.Flags().Changed("label") {
+	if cmd.Flags().Changed("label") || cmd.Flags().Changed("label-service") || cmd.Flags().Changed("label-revision") {
-		labelsMap, err := util.MapFromArrayAllowingSingles(p.Labels, "=")
+		labelsAllMap, err := util.MapFromArrayAllowingSingles(p.Labels, "=")
 		if err != nil {
 			return errors.Wrap(err, "Invalid --label")
 		}
 
-		labelsToRemove := util.ParseMinusSuffix(labelsMap)
+		err = p.updateLabels(&service.ObjectMeta, p.LabelsService, labelsAllMap)
-		err = servinglib.UpdateLabels(service, template, labelsMap, labelsToRemove)
 		if err != nil {
-			return err
+			return errors.Wrap(err, "Invalid --label-service")
+		}
+
+		err = p.updateLabels(&template.ObjectMeta, p.LabelsRevision, labelsAllMap)
+		if err != nil {
+			return errors.Wrap(err, "Invalid --label-revision")
 		}
 	}
 
@@ -406,6 +427,20 @@ func (p *ConfigurationEditFlags) Apply(
 	return nil
 }
 
+func (p *ConfigurationEditFlags) updateLabels(obj *metav1.ObjectMeta, flagLabels []string, labelsAllMap map[string]string) error {
+	labelFlagMap, err := util.MapFromArrayAllowingSingles(flagLabels, "=")
+	if err != nil {
+		return errors.Wrap(err, "Unable to parse label flags")
+	}
+	labelsMap := make(util.StringMap)
+	labelsMap.Merge(labelsAllMap)
+	labelsMap.Merge(labelFlagMap)
+	revisionLabelsToRemove := util.ParseMinusSuffix(labelsMap)
+	obj.Labels = servinglib.UpdateLabels(obj.Labels, labelsMap, revisionLabelsToRemove)
+
+	return nil
+}
+
 func (p *ConfigurationEditFlags) computeResources(resourceFlags ResourceFlags) (corev1.ResourceList, error) {
 	resourceList := corev1.ResourceList{}
 

pkg/serving/config_changes.go

@@ -374,34 +374,20 @@ func UpdateResources(template *servingv1.RevisionTemplateSpec, requestsResourceL
 	return nil
 }
 
-// ServiceOnlyLabels should only appear on the Service and NOT on the
+// UpdateLabels updates the labels by adding items from `add` then removing any items from `remove`
-// Revision template
+func UpdateLabels(labelsMap map[string]string, add map[string]string, remove []string) map[string]string {
-var ServiceOnlyLabels = map[string]struct{}{
+	if labelsMap == nil {
-	serving.GroupName + "/visibility": {},
+		labelsMap = map[string]string{}
-}
+	}
 
-// UpdateLabels updates the labels identically on a service and template.
+	for key, value := range add {
-// Does not overwrite the entire Labels field, only makes the requested updates
+		labelsMap[key] = value
-func UpdateLabels(service *servingv1.Service, template *servingv1.RevisionTemplateSpec, toUpdate map[string]string, toRemove []string) error {
-	if service.ObjectMeta.Labels == nil {
-		service.ObjectMeta.Labels = make(map[string]string)
 	}
-	if template.ObjectMeta.Labels == nil {
+	for _, key := range remove {
-		template.ObjectMeta.Labels = make(map[string]string)
+		delete(labelsMap, key)
 	}
-	for key, value := range toUpdate {
-		service.ObjectMeta.Labels[key] = value
 
-		// Only add it to the template if it's not in our ServiceOnly list
+	return labelsMap
-		if _, ok := ServiceOnlyLabels[key]; !ok {
-			template.ObjectMeta.Labels[key] = value
-		}
-	}
-	for _, key := range toRemove {
-		delete(service.ObjectMeta.Labels, key)
-		delete(template.ObjectMeta.Labels, key)
-	}
-	return nil
 }
 
 // UpdateAnnotations updates the annotations identically on a service and template.

pkg/serving/config_changes_test.go

@@ -335,27 +335,9 @@ func TestUpdateLabelsNew(t *testing.T) {
 		"a": "foo",
 		"b": "bar",
 	}
-	tLabels := labels // revision template labels
 
-	// Only test service-specific labels if we have any
+	service.ObjectMeta.Labels = UpdateLabels(service.ObjectMeta.Labels, labels, []string{})
-	if len(ServiceOnlyLabels) != 0 {
+	template.ObjectMeta.Labels = UpdateLabels(template.ObjectMeta.Labels, labels, []string{})
-		// Make a copy of the expected labels so we can modify the original
-		// list w/o changing what's expected for the revsion template
-		tLabels = map[string]string{}
-		for k, v := range labels {
-			tLabels[k] = v
-		}
-
-		// Just add a random value from the list to make sure it doesn't show
-		// up in the revision template
-		for k := range ServiceOnlyLabels {
-			labels[k] = "testing"
-			break
-		}
-	}
-
-	err := UpdateLabels(service, template, labels, []string{})
-	assert.NilError(t, err)
 
 	actual := service.ObjectMeta.Labels
 	if !reflect.DeepEqual(labels, actual) {
@@ -363,8 +345,8 @@ func TestUpdateLabelsNew(t *testing.T) {
 	}
 
 	actual = template.ObjectMeta.Labels
-	if !reflect.DeepEqual(tLabels, actual) {
+	if !reflect.DeepEqual(labels, actual) {
-		t.Fatalf("Template labels did not match expected %v found %v", tLabels, actual)
+		t.Fatalf("Template labels did not match expected %v found %v", labels, actual)
 	}
 }
 
@@ -378,20 +360,35 @@ func TestUpdateLabelsExisting(t *testing.T) {
 		"c": "bat",
 		"d": "",
 	}
-	err := UpdateLabels(service, template, labels, []string{})
+	tlabels := map[string]string{
-	assert.NilError(t, err)
+		"a": "notfoo",
-	expected := map[string]string{
+		"c": "bat",
+		"d": "",
+		"r": "poo",
+	}
+
+	service.ObjectMeta.Labels = UpdateLabels(service.ObjectMeta.Labels, labels, []string{})
+	template.ObjectMeta.Labels = UpdateLabels(template.ObjectMeta.Labels, tlabels, []string{})
+
+	expectedServiceLabel := map[string]string{
 		"a": "notfoo",
 		"b": "bar",
 		"c": "bat",
 		"d": "",
 	}
+	expectedRevLabel := map[string]string{
+		"a": "notfoo",
+		"b": "bar",
+		"c": "bat",
+		"d": "",
+		"r": "poo",
+	}
 
 	actual := service.ObjectMeta.Labels
-	assert.DeepEqual(t, expected, actual)
+	assert.DeepEqual(t, expectedServiceLabel, actual)
 
 	actual = template.ObjectMeta.Labels
-	assert.DeepEqual(t, expected, actual)
+	assert.DeepEqual(t, expectedRevLabel, actual)
 }
 
 func TestUpdateLabelsRemoveExisting(t *testing.T) {
@@ -400,8 +397,9 @@ func TestUpdateLabelsRemoveExisting(t *testing.T) {
 	template.ObjectMeta.Labels = map[string]string{"a": "foo", "b": "bar"}
 
 	remove := []string{"b"}
-	err := UpdateLabels(service, template, map[string]string{}, remove)
+	service.ObjectMeta.Labels = UpdateLabels(service.ObjectMeta.Labels, map[string]string{}, remove)
-	assert.NilError(t, err)
+	template.ObjectMeta.Labels = UpdateLabels(template.ObjectMeta.Labels, map[string]string{}, remove)
+
 	expected := map[string]string{
 		"a": "foo",
 	}