feat(annotations): Adds annotation flag for service create and update (#422)

* feat(annotations): Adds annotation flag for service create and update

  - Adds specified annotations to service object meta and revision template meta
  - Adds --annotation / -a flag to service create and update options
  - User can specify '-' at the end of the annotation key to remove an annotation
  - Adds unit and e2e tests
  - Updates docs and changelog accordingly

* Adds example for service create with annotation

* Adds mock unit tests for service update with annotations

* Removes the short hand -a for annotation flag

CHANGELOG.adoc

@@ -62,6 +62,14 @@
 | Add --service-account flag
 | https://github.com/knative/client/pull/401[#401]
 
+| 🧽
+| Docs restructure
+| https://github.com/knative/client/pull/421[#421]
+
+| 🎁
+| Add --annotation flag
+| https://github.com/knative/client/pull/422[#422]
+
 |===
 
 ## v0.2.0 (2019-07-10)

docs/cmd/kn_service_create.md

@@ -34,11 +34,15 @@ kn service create NAME --image IMAGE [flags]
   # (earlier configured resource requests and limits will be replaced with default)
   # (earlier configured environment variables will be cleared too if any)
   kn service create --force s1 --image dev.local/ns/image:v1
+
+  # Create a service with annotation
+  kn service create s1 --image dev.local/ns/image:v3 --annotation sidecar.istio.io/inject=false
 ```
 
 ### Options
 
 ```
+      --annotation stringArray   Service annotation to set. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-).
       --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.

docs/cmd/kn_service_update.md

@@ -38,6 +38,7 @@ kn service update NAME [flags]
 ### Options
 
 ```
+      --annotation stringArray   Service annotation to set. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-).
       --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.

pkg/kn/commands/service/configuration_edit_flags.go

@@ -41,6 +41,7 @@ type ConfigurationEditFlags struct {
 	NamePrefix                 string
 	RevisionName               string
 	ServiceAccountName         string
+	Annotations                []string
 
 	// Preferences about how to do the action.
 	LockToDigest         bool
@@ -110,6 +111,11 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 	// Don't mark as changing the revision.
 	command.Flags().StringVar(&p.ServiceAccountName, "service-account", "", "Service account name to set. Empty service account name will result to clear the service account.")
 	p.markFlagMakesRevision("service-account")
+	command.Flags().StringArrayVar(&p.Annotations, "annotation", []string{},
+		"Service annotation to set. name=value; you may provide this flag "+
+			"any number of times to set multiple annotations. "+
+			"To unset, specify the annotation name followed by a \"-\" (e.g., name-).")
+	p.markFlagMakesRevision("annotation")
 }
 
 // AddUpdateFlags adds the flags specific to update.
@@ -256,6 +262,24 @@ func (p *ConfigurationEditFlags) Apply(
 		}
 	}
 
+	if cmd.Flags().Changed("annotation") {
+		annotationsMap, err := util.MapFromArrayAllowingSingles(p.Annotations, "=")
+		if err != nil {
+			return errors.Wrap(err, "Invalid --annotation")
+		}
+		annotationsToRemove := []string{}
+		for key := range annotationsMap {
+			if strings.HasSuffix(key, "-") {
+				annotationsToRemove = append(annotationsToRemove, key[:len(key)-1])
+				delete(annotationsMap, key)
+			}
+		}
+		err = servinglib.UpdateAnnotations(service, template, annotationsMap, annotationsToRemove)
+		if err != nil {
+			return err
+		}
+	}
+
 	if cmd.Flags().Changed("service-account") {
 		err = servinglib.UpdateServiceAccountName(template, p.ServiceAccountName)
 		if err != nil {

pkg/kn/commands/service/create.go

@@ -33,14 +33,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
+var create_example = `
-	var editFlags ConfigurationEditFlags
-	var waitFlags commands.WaitFlags
-
-	serviceCreateCommand := &cobra.Command{
-		Use:   "create NAME --image IMAGE",
-		Short: "Create a service.",
-		Example: `
   # Create a service 'mysvc' using image at dev.local/ns/image:latest
   kn service create mysvc --image dev.local/ns/image:latest
 
@@ -60,8 +53,19 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
   # Create or replace default resources of a service 's1' using --force flag
   # (earlier configured resource requests and limits will be replaced with default)
   # (earlier configured environment variables will be cleared too if any)
-  kn service create --force s1 --image dev.local/ns/image:v1`,
+  kn service create --force s1 --image dev.local/ns/image:v1
 
+  # Create a service with annotation
+  kn service create s1 --image dev.local/ns/image:v3 --annotation sidecar.istio.io/inject=false`
+
+func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command {
+	var editFlags ConfigurationEditFlags
+	var waitFlags commands.WaitFlags
+
+	serviceCreateCommand := &cobra.Command{
+		Use:     "create NAME --image IMAGE",
+		Short:   "Create a service.",
+		Example: create_example,
 		RunE: func(cmd *cobra.Command, args []string) (err error) {
 			if len(args) != 1 {
 				return errors.New("'service create' requires the service name given as single argument")

pkg/kn/commands/service/service_update_mock_test.go

@@ -69,3 +69,64 @@ func TestServiceUpdateEnvMock(t *testing.T) {
 
 	r.Validate()
 }
+
+func TestServiceUpdateAnnotationsMock(t *testing.T) {
+	client := knclient.NewMockKnClient(t)
+	svcName := "svc1"
+	newService := getService(svcName)
+	template, err := servinglib.RevisionTemplateOfService(newService)
+	assert.NilError(t, err)
+	template.Spec.GetContainer().Image = "gcr.io/foo/bar:baz"
+	newService.ObjectMeta.Annotations = map[string]string{
+		"an1": "staysConstant",
+		"an2": "getsUpdated",
+		"an3": "getsRemoved",
+	}
+	template.ObjectMeta.Annotations = map[string]string{
+		"an1":                             "staysConstant",
+		"an2":                             "getsUpdated",
+		"an3":                             "getsRemoved",
+		servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz",
+	}
+
+	updatedService := getService(svcName)
+	template, err = servinglib.RevisionTemplateOfService(updatedService)
+	assert.NilError(t, err)
+	template.Spec.GetContainer().Image = "gcr.io/foo/bar:baz"
+	updatedService.ObjectMeta.Annotations = map[string]string{
+		"an1": "staysConstant",
+		"an2": "isUpdated",
+	}
+	template.ObjectMeta.Annotations = map[string]string{
+		"an1":                             "staysConstant",
+		"an2":                             "isUpdated",
+		servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz",
+	}
+
+	r := client.Recorder()
+	r.GetService(svcName, nil, errors.NewNotFound(v1alpha1.Resource("service"), svcName))
+	r.CreateService(newService, nil)
+	r.GetService(svcName, newService, nil)
+	r.UpdateService(updatedService, nil)
+
+	output, err := executeServiceCommand(client,
+		"create", svcName, "--image", "gcr.io/foo/bar:baz",
+		"--annotation", "an1=staysConstant",
+		"--annotation", "an2=getsUpdated",
+		"--annotation", "an3=getsRemoved",
+		"--async", "--revision-name=",
+	)
+	assert.NilError(t, err)
+	assert.Assert(t, util.ContainsAll(output, "created", svcName, "default"))
+
+	output, err = executeServiceCommand(client,
+		"update", svcName,
+		"--annotation", "an2=isUpdated",
+		"--annotation", "an3-",
+		"--async", "--revision-name=",
+	)
+	assert.NilError(t, err)
+	assert.Assert(t, util.ContainsAll(output, "updated", svcName, "default"))
+
+	r.Validate()
+}

pkg/kn/commands/service/update.go

@@ -28,14 +28,7 @@ import (
 	"knative.dev/serving/pkg/apis/serving/v1alpha1"
 )
 
-func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
+var update_example = `
-	var editFlags ConfigurationEditFlags
-	var waitFlags commands.WaitFlags
-	var trafficFlags flags.Traffic
-	serviceUpdateCommand := &cobra.Command{
-		Use:   "update NAME [flags]",
-		Short: "Update a service.",
-		Example: `
   # Updates a service 'svc' with new environment variables
   kn service update svc --env KEY1=VALUE1 --env KEY2=VALUE2
 
@@ -54,7 +47,16 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
   kn service update svc --untag testing --tag @latest=staging
 
   # Add tag 'test' to echo-v3 revision with 10% traffic and rest to latest ready revision of service
-  kn service update svc --tag echo-v3=test --traffic test=10,@latest=90`,
+  kn service update svc --tag echo-v3=test --traffic test=10,@latest=90`
+
+func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
+	var editFlags ConfigurationEditFlags
+	var waitFlags commands.WaitFlags
+	var trafficFlags flags.Traffic
+	serviceUpdateCommand := &cobra.Command{
+		Use:     "update NAME [flags]",
+		Short:   "Update a service.",
+		Example: update_example,
 		RunE: func(cmd *cobra.Command, args []string) (err error) {
 			if len(args) != 1 {
 				return errors.New("requires the service name.")

pkg/serving/config_changes.go

@@ -52,24 +52,17 @@ func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, toUpdate map[
 
 // UpdateMinScale updates min scale annotation
 func UpdateMinScale(template *servingv1alpha1.RevisionTemplateSpec, min int) error {
-	return UpdateAnnotation(template, autoscaling.MinScaleAnnotationKey, strconv.Itoa(min))
+	return UpdateRevisionTemplateAnnotation(template, autoscaling.MinScaleAnnotationKey, strconv.Itoa(min))
 }
 
 // UpdatMaxScale updates max scale annotation
 func UpdateMaxScale(template *servingv1alpha1.RevisionTemplateSpec, max int) error {
-	return UpdateAnnotation(template, autoscaling.MaxScaleAnnotationKey, strconv.Itoa(max))
+	return UpdateRevisionTemplateAnnotation(template, autoscaling.MaxScaleAnnotationKey, strconv.Itoa(max))
 }
 
 // UpdateConcurrencyTarget updates container concurrency annotation
 func UpdateConcurrencyTarget(template *servingv1alpha1.RevisionTemplateSpec, target int) error {
-	// TODO(toVersus): Remove the following validation once serving library is updated to v0.8.0
+	return UpdateRevisionTemplateAnnotation(template, autoscaling.TargetAnnotationKey, strconv.Itoa(target))
-	// and just rely on ValidateAnnotations method.
-	if target < autoscaling.TargetMin {
-		return fmt.Errorf("invalid 'concurrency-target' value: must be an integer greater than 0: %s",
-			autoscaling.TargetAnnotationKey)
-	}
-
-	return UpdateAnnotation(template, autoscaling.TargetAnnotationKey, strconv.Itoa(target))
 }
 
 // UpdateConcurrencyLimit updates container concurrency limit
@@ -84,8 +77,9 @@ func UpdateConcurrencyLimit(template *servingv1alpha1.RevisionTemplateSpec, limi
 	return nil
 }
 
-// UpdateAnnotation updates (or adds) an annotation to the given service
+// UpdateRevisionTemplateAnnotation updates an annotation for the given Revision Template.
-func UpdateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation string, value string) error {
+// Also validates the autoscaling annotation values
+func UpdateRevisionTemplateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation string, value string) error {
 	annoMap := template.Annotations
 	if annoMap == nil {
 		annoMap = make(map[string]string)
@@ -245,6 +239,35 @@ func UpdateLabels(service *servingv1alpha1.Service, template *servingv1alpha1.Re
 	return nil
 }
 
+// UpdateAnnotations updates the annotations identically on a service and template.
+// Does not overwrite the entire Annotations field, only makes the requested updates.
+func UpdateAnnotations(
+	service *servingv1alpha1.Service,
+	template *servingv1alpha1.RevisionTemplateSpec,
+	toUpdate map[string]string,
+	toRemove []string) error {
+
+	if service.ObjectMeta.Annotations == nil {
+		service.ObjectMeta.Annotations = make(map[string]string)
+	}
+
+	if template.ObjectMeta.Annotations == nil {
+		template.ObjectMeta.Annotations = make(map[string]string)
+	}
+
+	for key, value := range toUpdate {
+		service.ObjectMeta.Annotations[key] = value
+		template.ObjectMeta.Annotations[key] = value
+	}
+
+	for _, key := range toRemove {
+		delete(service.ObjectMeta.Annotations, key)
+		delete(template.ObjectMeta.Annotations, key)
+	}
+
+	return nil
+}
+
 // UpdateServiceAccountName updates the service account name used for the corresponding knative service
 func UpdateServiceAccountName(template *servingv1alpha1.RevisionTemplateSpec, serviceAccountName string) error {
 	serviceAccountName = strings.TrimSpace(serviceAccountName)

pkg/serving/config_changes_test.go

@@ -419,6 +419,73 @@ func TestUpdateServiceAccountName(t *testing.T) {
 	assert.Equal(t, template.Spec.ServiceAccountName, "")
 }
 
+func TestUpdateAnnotationsNew(t *testing.T) {
+	service, template, _ := getV1alpha1Service()
+
+	annotations := map[string]string{
+		"a": "foo",
+		"b": "bar",
+	}
+	err := UpdateAnnotations(service, template, annotations, []string{})
+	assert.NilError(t, err)
+
+	actual := service.ObjectMeta.Annotations
+	if !reflect.DeepEqual(annotations, actual) {
+		t.Fatalf("Service annotations did not match expected %v found %v", annotations, actual)
+	}
+
+	actual = template.ObjectMeta.Annotations
+	if !reflect.DeepEqual(annotations, actual) {
+		t.Fatalf("Template annotations did not match expected %v found %v", annotations, actual)
+	}
+}
+
+func TestUpdateAnnotationsExisting(t *testing.T) {
+	service, template, _ := getV1alpha1Service()
+	service.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"}
+	template.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"}
+
+	annotations := map[string]string{
+		"a": "notfoo",
+		"c": "bat",
+		"d": "",
+	}
+	err := UpdateAnnotations(service, template, annotations, []string{})
+	assert.NilError(t, err)
+	expected := map[string]string{
+		"a": "notfoo",
+		"b": "bar",
+		"c": "bat",
+		"d": "",
+	}
+
+	actual := service.ObjectMeta.Annotations
+	assert.DeepEqual(t, expected, actual)
+
+	actual = template.ObjectMeta.Annotations
+	assert.DeepEqual(t, expected, actual)
+}
+
+func TestUpdateAnnotationsRemoveExisting(t *testing.T) {
+	service, template, _ := getV1alpha1Service()
+	service.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"}
+	template.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"}
+
+	remove := []string{"b"}
+	err := UpdateAnnotations(service, template, map[string]string{}, remove)
+	assert.NilError(t, err)
+	expected := map[string]string{
+		"a": "foo",
+	}
+
+	actual := service.ObjectMeta.Annotations
+	assert.DeepEqual(t, expected, actual)
+
+	actual = template.ObjectMeta.Annotations
+	assert.DeepEqual(t, expected, actual)
+}
+
+//
 // =========================================================================================================
 
 func getV1alpha1RevisionTemplateWithOldFields() (*servingv1alpha1.RevisionTemplateSpec, *corev1.Container) {

test/e2e/service_options_test.go

@@ -17,6 +17,7 @@
 package e2e
 
 import (
+	"fmt"
 	"testing"
 
 	"gotest.tools/assert"
@@ -69,6 +70,14 @@ func TestServiceOptions(t *testing.T) {
 	t.Run("delete service", func(t *testing.T) {
 		test.serviceDelete(t, "svc2")
 	})
+
+	t.Run("create, update and validate service with annotations", func(t *testing.T) {
+		test.serviceCreateWithOptions(t, "svc3", []string{"--annotation", "alpha=wolf", "--annotation", "brave=horse"})
+		test.validateServiceAnnotations(t, "svc3", map[string]string{"alpha": "wolf", "brave": "horse"})
+		test.serviceUpdate(t, "svc3", []string{"--annotation", "alpha=direwolf", "--annotation", "brave-"})
+		test.validateServiceAnnotations(t, "svc3", map[string]string{"alpha": "direwolf", "brave": ""})
+		test.serviceDelete(t, "svc3")
+	})
 }
 
 func (test *e2eTest) serviceCreateWithOptions(t *testing.T, serviceName string, options []string) {
@@ -142,3 +151,27 @@ func (test *e2eTest) validateServiceMaxScale(t *testing.T, serviceName, maxScale
 		assert.Equal(t, maxScale, out)
 	}
 }
+
+func (test *e2eTest) validateServiceAnnotations(t *testing.T, serviceName string, annotations map[string]string) {
+	metadataAnnotationsJsonpathFormat := "jsonpath={.metadata.annotations.%s}"
+	templateAnnotationsJsonpathFormat := "jsonpath={.spec.template.metadata.annotations.%s}"
+	oldTemplateAnnotationsJsonpathFormat := "jsonpath={.spec.runLatest.configuration.revisionTemplate.metadata.annotations.%s}"
+
+	for k, v := range annotations {
+		out, err := test.kn.RunWithOpts([]string{"service", "describe", serviceName, "-o", fmt.Sprintf(metadataAnnotationsJsonpathFormat, k)}, runOpts{})
+		assert.NilError(t, err)
+		assert.Equal(t, v, out)
+
+		out, err = test.kn.RunWithOpts([]string{"service", "describe", serviceName, "-o", fmt.Sprintf(templateAnnotationsJsonpathFormat, k)}, runOpts{})
+		assert.NilError(t, err)
+		if out != "" || v == "" {
+			assert.Equal(t, v, out)
+		} else {
+			// case where server returns fields like  spec.runLatest.configuration.revisionTemplate.metadata.annotations
+			// TODO: Remove this case when `runLatest` field is deprecated altogether / v1beta1
+			out, err := test.kn.RunWithOpts([]string{"service", "describe", serviceName, "-o", fmt.Sprintf(oldTemplateAnnotationsJsonpathFormat, k)}, runOpts{})
+			assert.NilError(t, err)
+			assert.Equal(t, v, out)
+		}
+	}
+}