(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

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_plugin.md

@@ -4,10 +4,9 @@ Plugin command group
 
 ### Synopsis
 
-Provides utilities for interacting and managing with `kn` plugins.
-
-Plugins provide extended functionality that is not part of the core `kn` command-line distribution.
+Provides utilities for interacting and managing with kn plugins.
 
+Plugins provide extended functionality that is not part of the core kn command-line distribution.
 Please refer to the documentation and examples for more information about how write your own plugins.
 
 ```
@@ -34,3 +33,4 @@ kn plugin [flags]
 
 * [kn](kn.md)	 - Knative client
 * [kn plugin list](kn_plugin_list.md)	 - List plugins
+

docs/cmd/kn_service_create.md

@@ -39,6 +39,7 @@ kn service create NAME --image IMAGE [flags]
 ### Options
 
 ```
+  -a, --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
 
 ```
+  -a, --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().StringArrayVarP(&p.Annotations, "annotation", "a", []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,27 @@ 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/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
-	// 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))
+	return UpdateRevisionTemplateAnnotation(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
-func UpdateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation string, value string) error {
+// UpdateRevisionTemplateAnnotation updates an annotation for the given Revision Template.
+// 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{"--annotations", "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)
+		}
+	}
+}