Service and Revision labels (#342)

* Add support for service labels with --label or -l

* Add tests

* Fix typo

* Move MapToArray helper method to separate file

* Allow unsetting labels by passing empty value

* Fix test

* Add test case for label removal

* Wrap error message with flag

* Update docs to include --label

* Update labels on both services and revisions

* Add some test cases around weirder user input

* Change unset behavior and allow setting empty env and label

* Update docs for new unset behavior

* Make single keys to map to empty values

* Move helper to util

* Use assert.DeepEqual

* Use new mock test + check both service and revision

* Use new method and correct year

docs/cmd/kn_service_create.md

@@ -42,10 +42,11 @@ kn service create NAME --image IMAGE [flags]
       --async                    Create service and don't wait for it to become ready.
       --concurrency-limit int    Hard Limit of concurrent requests to be processed by a single replica.
       --concurrency-target int   Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given.
-  -e, --env stringArray          Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables.
+  -e, --env stringArray          Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables. To unset, specify the environment variable name followed by a "-" (e.g., NAME-).
       --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-).
       --limits-cpu string        The limits on the requested CPU (e.g., 1000m).
       --limits-memory string     The limits on the requested memory (e.g., 1024Mi).
       --max-scale int            Maximal number of replicas.

docs/cmd/kn_service_update.md

@@ -30,9 +30,10 @@ kn service update NAME [flags]
       --async                    Update service and don't wait for it to become ready.
       --concurrency-limit int    Hard Limit of concurrent requests to be processed by a single replica.
       --concurrency-target int   Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given.
-  -e, --env stringArray          Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables.
+  -e, --env stringArray          Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables. To unset, specify the environment variable name followed by a "-" (e.g., NAME-).
   -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-).
       --limits-cpu string        The limits on the requested CPU (e.g., 1000m).
       --limits-memory string     The limits on the requested memory (e.g., 1024Mi).
       --max-scale int            Maximal number of replicas.

pkg/kn/commands/service/configuration_edit_flags.go

@@ -15,11 +15,12 @@
 package service
 
 import (
-	"fmt"
 	"strings"
 
 	servinglib "github.com/knative/client/pkg/serving"
+	util "github.com/knative/client/pkg/util"
 	servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1"
+	errors "github.com/pkg/errors"
 	"github.com/spf13/cobra"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -35,6 +36,7 @@ type ConfigurationEditFlags struct {
 	ConcurrencyTarget          int
 	ConcurrencyLimit           int
 	Port                       int32
+	Labels                     []string
 }
 
 type ResourceFlags struct {
@@ -46,7 +48,8 @@ func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) {
 	command.Flags().StringVar(&p.Image, "image", "", "Image to run.")
 	command.Flags().StringArrayVarP(&p.Env, "env", "e", []string{},
 		"Environment variable to set. NAME=value; you may provide this flag "+
-			"any number of times to set multiple environment variables.")
+			"any number of times to set multiple environment variables. "+
+			"To unset, specify the environment variable name followed by a \"-\" (e.g., NAME-).")
 	command.Flags().StringVar(&p.RequestsFlags.CPU, "requests-cpu", "", "The requested CPU (e.g., 250m).")
 	command.Flags().StringVar(&p.RequestsFlags.Memory, "requests-memory", "", "The requested memory (e.g., 64Mi).")
 	command.Flags().StringVar(&p.LimitsFlags.CPU, "limits-cpu", "", "The limits on the requested CPU (e.g., 1000m).")
@@ -56,6 +59,10 @@ func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) {
 	command.Flags().IntVar(&p.ConcurrencyTarget, "concurrency-target", 0, "Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given.")
 	command.Flags().IntVar(&p.ConcurrencyLimit, "concurrency-limit", 0, "Hard Limit of concurrent requests to be processed by a single replica.")
 	command.Flags().Int32VarP(&p.Port, "port", "p", 0, "The port where application listens on.")
+	command.Flags().StringArrayVarP(&p.Labels, "label", "l", []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-).")
 }
 
 func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) {
@@ -71,18 +78,22 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co
 		return err
 	}
 
-	envMap := map[string]string{}
-	for _, pairStr := range p.Env {
-		pairSlice := strings.SplitN(pairStr, "=", 2)
-		if len(pairSlice) <= 1 {
-			return fmt.Errorf(
-				"--env argument requires a value that contains the '=' character; got %s",
-				pairStr)
+	if cmd.Flags().Changed("env") {
+		envMap, err := util.MapFromArrayAllowingSingles(p.Env, "=")
+		if err != nil {
+			return errors.Wrap(err, "Invalid --env")
+		}
+		envToRemove := []string{}
+		for name := range envMap {
+			if strings.HasSuffix(name, "-") {
+				envToRemove = append(envToRemove, name[:len(name)-1])
+				delete(envMap, name)
+			}
+		}
+		err = servinglib.UpdateEnvVars(template, envMap, envToRemove)
+		if err != nil {
+			return err
 		}
-		envMap[pairSlice[0]] = pairSlice[1]
-	}
-	if err := servinglib.UpdateEnvVars(template, envMap); err != nil {
-		return err
 	}
 
 	if cmd.Flags().Changed("image") {
@@ -139,6 +150,24 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co
 		}
 	}
 
+	if cmd.Flags().Changed("label") {
+		labelsMap, err := util.MapFromArrayAllowingSingles(p.Labels, "=")
+		if err != nil {
+			return errors.Wrap(err, "Invalid --label")
+		}
+		labelsToRemove := []string{}
+		for key := range labelsMap {
+			if strings.HasSuffix(key, "-") {
+				labelsToRemove = append(labelsToRemove, key[:len(key)-1])
+				delete(labelsMap, key)
+			}
+		}
+		err = servinglib.UpdateLabels(service, template, labelsMap, labelsToRemove)
+		if err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 

pkg/kn/commands/service/service_create_mock_test.go

@@ -19,10 +19,13 @@ import (
 
 	"github.com/knative/pkg/apis"
 	"gotest.tools/assert"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
 
+	servinglib "github.com/knative/client/pkg/serving"
 	knclient "github.com/knative/client/pkg/serving/v1alpha1"
 
 	"github.com/knative/client/pkg/util"
@@ -53,6 +56,58 @@ func TestServiceCreateImageMock(t *testing.T) {
 	r.Validate()
 }
 
+func TestServiceCreateLabel(t *testing.T) {
+	client := knclient.NewMockKnClient(t)
+
+	r := client.Recorder()
+	r.GetService("foo", nil, errors.NewNotFound(v1alpha1.Resource("service"), "foo"))
+
+	service := getService("foo")
+	expected := map[string]string{
+		"a":     "mouse",
+		"b":     "cookie",
+		"empty": "",
+	}
+	service.ObjectMeta.Labels = expected
+	template, err := servinglib.RevisionTemplateOfService(service)
+	assert.NilError(t, err)
+	template.ObjectMeta.Labels = expected
+	template.Spec.DeprecatedContainer.Image = "gcr.io/foo/bar:baz"
+	r.CreateService(service, nil)
+
+	output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", "-l", "a=mouse", "--label", "b=cookie", "--label=empty", "--async")
+	assert.NilError(t, err)
+	assert.Assert(t, util.ContainsAll(output, "created", "foo", "default"))
+
+	r.Validate()
+}
+
+func getService(name string) *v1alpha1.Service {
+	service := &v1alpha1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: "default",
+		},
+		Spec: v1alpha1.ServiceSpec{
+			DeprecatedRunLatest: &v1alpha1.RunLatestType{
+				Configuration: v1alpha1.ConfigurationSpec{
+					DeprecatedRevisionTemplate: &v1alpha1.RevisionTemplateSpec{
+						Spec: v1alpha1.RevisionSpec{
+							DeprecatedContainer: &corev1.Container{
+								Resources: corev1.ResourceRequirements{
+									Limits:   corev1.ResourceList{},
+									Requests: corev1.ResourceList{},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	return service
+}
+
 func getServiceWithUrl(name string, urlName string) *v1alpha1.Service {
 	service := v1alpha1.Service{}
 	url, _ := apis.ParseURL(urlName)

pkg/kn/commands/service/service_create_test.go

@@ -158,7 +158,7 @@ func TestServiceCreateImageSync(t *testing.T) {
 
 func TestServiceCreateEnv(t *testing.T) {
 	action, created, _, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "-e", "A=DOGS", "--env", "B=WOLVES", "--async"}, false, false)
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "-e", "A=DOGS", "--env", "B=WOLVES", "--env=EMPTY", "--async"}, false, false)
 
 	if err != nil {
 		t.Fatal(err)
@@ -167,8 +167,10 @@ func TestServiceCreateEnv(t *testing.T) {
 	}
 
 	expectedEnvVars := map[string]string{
-		"A": "DOGS",
-		"B": "WOLVES"}
+		"A":     "DOGS",
+		"B":     "WOLVES",
+		"EMPTY": "",
+	}
 
 	template, err := servinglib.RevisionTemplateOfService(created)
 	actualEnvVars, err := servinglib.EnvToMap(template.Spec.DeprecatedContainer.Env)

pkg/kn/commands/service/service_update_test.go

@@ -232,11 +232,15 @@ func TestServiceUpdateEnv(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	template.Spec.DeprecatedContainer.Env = []corev1.EnvVar{
+		{Name: "EXISTING", Value: "thing"},
+		{Name: "OTHEREXISTING"},
+	}
 
 	servinglib.UpdateImage(template, "gcr.io/foo/bar:baz")
 
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "-e", "TARGET=Awesome", "--async"}, false)
+		"service", "update", "foo", "-e", "TARGET=Awesome", "--env", "EXISTING-", "--env=OTHEREXISTING-=whatever", "--async"}, false)
 
 	if err != nil {
 		t.Fatal(err)
@@ -374,6 +378,60 @@ func TestServiceUpdateRequestsLimitsCPU_and_Memory(t *testing.T) {
 	}
 }
 
+func TestServiceUpdateLabelWhenEmpty(t *testing.T) {
+	original := newEmptyService()
+
+	action, updated, _, err := fakeServiceUpdate(original, []string{
+		"service", "update", "foo", "-l", "a=mouse", "--label", "b=cookie", "-l=single", "--async"}, false)
+
+	if err != nil {
+		t.Fatal(err)
+	} else if !action.Matches("update", "services") {
+		t.Fatalf("Bad action %v", action)
+	}
+
+	expected := map[string]string{
+		"a":      "mouse",
+		"b":      "cookie",
+		"single": "",
+	}
+	actual := updated.ObjectMeta.Labels
+	assert.DeepEqual(t, expected, actual)
+
+	template, err := servinglib.RevisionTemplateOfService(updated)
+	assert.NilError(t, err)
+	actual = template.ObjectMeta.Labels
+	assert.DeepEqual(t, expected, actual)
+}
+
+func TestServiceUpdateLabelExisting(t *testing.T) {
+	original := newEmptyService()
+	original.ObjectMeta.Labels = map[string]string{"already": "here", "tobe": "removed"}
+	originalTemplate, _ := servinglib.RevisionTemplateOfService(original)
+	originalTemplate.ObjectMeta.Labels = map[string]string{"already": "here", "tobe": "removed"}
+
+	action, updated, _, err := fakeServiceUpdate(original, []string{
+		"service", "update", "foo", "-l", "already=gone", "--label=tobe-", "--label", "b=", "--async"}, false)
+
+	if err != nil {
+		t.Fatal(err)
+	} else if !action.Matches("update", "services") {
+		t.Fatalf("Bad action %v", action)
+	}
+
+	expected := map[string]string{
+		"already": "gone",
+		"b":       "",
+	}
+	actual := updated.ObjectMeta.Labels
+	assert.DeepEqual(t, expected, actual)
+
+	template, err := servinglib.RevisionTemplateOfService(updated)
+	assert.NilError(t, err)
+	actual = template.ObjectMeta.Labels
+	assert.DeepEqual(t, expected, actual)
+}
+
 func newEmptyService() *v1alpha1.Service {
 	return &v1alpha1.Service{
 		TypeMeta: metav1.TypeMeta{

pkg/serving/config_changes.go

@@ -28,12 +28,13 @@ import (
 // UpdateEnvVars gives the configuration all the env var values listed in the given map of
 // vars.  Does not touch any environment variables not mentioned, but it can add
 // new env vars and change the values of existing ones.
-func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, vars map[string]string) error {
+func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, toUpdate map[string]string, toRemove []string) error {
 	container, err := ContainerOfRevisionTemplate(template)
 	if err != nil {
 		return err
 	}
-	container.Env = updateEnvVarsFromMap(container.Env, vars)
+	envVars := updateEnvVarsFromMap(container.Env, toUpdate)
+	container.Env = removeEnvVars(envVars, toRemove)
 	return nil
 }
 
@@ -152,6 +153,26 @@ func UpdateResources(template *servingv1alpha1.RevisionTemplateSpec, requestsRes
 	return nil
 }
 
+// UpdateLabels updates the labels identically on a service and template.
+// Does not overwrite the entire Labels field, only makes the requested updates
+func UpdateLabels(service *servingv1alpha1.Service, template *servingv1alpha1.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 {
+		template.ObjectMeta.Labels = make(map[string]string)
+	}
+	for key, value := range toUpdate {
+		service.ObjectMeta.Labels[key] = value
+		template.ObjectMeta.Labels[key] = value
+	}
+	for _, key := range toRemove {
+		delete(service.ObjectMeta.Labels, key)
+		delete(template.ObjectMeta.Labels, key)
+	}
+	return nil
+}
+
 // =======================================================================================
 
 func updateEnvVarsFromMap(env []corev1.EnvVar, vars map[string]string) []corev1.EnvVar {
@@ -176,3 +197,15 @@ func updateEnvVarsFromMap(env []corev1.EnvVar, vars map[string]string) []corev1.
 	}
 	return env
 }
+
+func removeEnvVars(env []corev1.EnvVar, toRemove []string) []corev1.EnvVar {
+	for _, name := range toRemove {
+		for i, envVar := range env {
+			if envVar.Name == name {
+				env = append(env[:i], env[i+1:]...)
+				break
+			}
+		}
+	}
+	return env
+}