Return error if invalid value is specified by users

pkg/kn/commands/service/configuration_edit_flags.go

@@ -112,19 +112,31 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co
 	}
 
 	if cmd.Flags().Changed("min-scale") {
-		servinglib.UpdateMinScale(template, p.MinScale)
+		err = servinglib.UpdateMinScale(template, p.MinScale)
+		if err != nil {
+			return err
+		}
 	}
 
 	if cmd.Flags().Changed("max-scale") {
-		servinglib.UpdateMaxScale(template, p.MaxScale)
+		err = servinglib.UpdateMaxScale(template, p.MaxScale)
+		if err != nil {
+			return err
+		}
 	}
 
 	if cmd.Flags().Changed("concurrency-target") {
-		servinglib.UpdateConcurrencyTarget(template, p.ConcurrencyTarget)
+		err = servinglib.UpdateConcurrencyTarget(template, p.ConcurrencyTarget)
+		if err != nil {
+			return err
+		}
 	}
 
 	if cmd.Flags().Changed("concurrency-limit") {
-		servinglib.UpdateConcurrencyLimit(template, p.ConcurrencyLimit)
+		err = servinglib.UpdateConcurrencyLimit(template, p.ConcurrencyLimit)
+		if err != nil {
+			return err
+		}
 	}
 
 	return nil

pkg/serving/config_changes.go

@@ -15,6 +15,7 @@
 package serving
 
 import (
+	"context"
 	"fmt"
 	"strconv"
 
@@ -37,47 +38,55 @@ func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, vars map[stri
 }
 
 // UpdateMinScale updates min scale annotation
-func UpdateMinScale(template *servingv1alpha1.RevisionTemplateSpec, min int) {
-	UpdateAnnotation(template, autoscaling.MinScaleAnnotationKey, strconv.Itoa(min))
+func UpdateMinScale(template *servingv1alpha1.RevisionTemplateSpec, min int) error {
+	return UpdateAnnotation(template, autoscaling.MinScaleAnnotationKey, strconv.Itoa(min))
 }
 
 // UpdatMaxScale updates max scale annotation
-func UpdateMaxScale(template *servingv1alpha1.RevisionTemplateSpec, max int) {
-	UpdateAnnotation(template, autoscaling.MaxScaleAnnotationKey, strconv.Itoa(max))
+func UpdateMaxScale(template *servingv1alpha1.RevisionTemplateSpec, max int) error {
+	return UpdateAnnotation(template, autoscaling.MaxScaleAnnotationKey, strconv.Itoa(max))
 }
 
 // UpdateConcurrencyTarget updates container concurrency annotation
-func UpdateConcurrencyTarget(template *servingv1alpha1.RevisionTemplateSpec, target int) {
+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
+		return fmt.Errorf("Invalid %s annotation value: must be an intger greater than 0", autoscaling.TargetAnnotationKey)
 	}
-	UpdateAnnotation(template, autoscaling.TargetAnnotationKey, strconv.Itoa(target))
+
+	return UpdateAnnotation(template, autoscaling.TargetAnnotationKey, strconv.Itoa(target))
 }
 
 // UpdateConcurrencyLimit updates container concurrency limit
-func UpdateConcurrencyLimit(template *servingv1alpha1.RevisionTemplateSpec, limit int) {
-	if limit >= 0 {
-		template.Spec.ContainerConcurrency = servingv1beta1.RevisionContainerConcurrencyType(limit)
+func UpdateConcurrencyLimit(template *servingv1alpha1.RevisionTemplateSpec, limit int) error {
+	cc := servingv1beta1.RevisionContainerConcurrencyType(limit)
+	// Validate input limit
+	ctx := context.Background()
+	if err := cc.Validate(ctx).ViaField("spec.containerConcurrency"); err != nil {
+		return fmt.Errorf("Invalid containerConcurrency revision spec: %s", err)
 	}
+	template.Spec.ContainerConcurrency = cc
+	return nil
 }
 
 // UpdateAnnotation updates (or adds) an annotation to the given service
-func UpdateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation string, value string) {
+func UpdateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation string, value string) error {
 	annoMap := template.Annotations
 	if annoMap == nil {
 		annoMap = make(map[string]string)
 		template.Annotations = annoMap
 	}
+
 	// Validate autoscaling annotations and returns the same value as before if input value is invalid
 	in := make(map[string]string)
 	in[annotation] = value
 	if err := autoscaling.ValidateAnnotations(in); err != nil {
-		return
+		return err
 	}
 
 	annoMap[annotation] = value
+	return nil
 }
 
 // EnvToMap is an utility function to translate between the API list form of env vars, and the

test/e2e/service_options_test.go

@@ -46,8 +46,8 @@ func TestServiceOptions(t *testing.T) {
 		test.serviceDescribeConcurrencyLimit(t, "hello", "300")
 	})
 
-	t.Run("update concurrency options with invalid value for hello service and returns no change", func(t *testing.T) {
-		test.serviceUpdate(t, "hello", []string{"--concurrency-limit", "-1", "--concurrency-target", "0"})
+	t.Run("update concurrency options with invalid value for hello service and returns error", func(t *testing.T) {
+		test.serviceUpdateWithInvalidValue(t, "hello", []string{"--concurrency-limit", "-1", "--concurrency-target", "0"})
 	})
 
 	t.Run("returns steady concurrency options for hello service", func(t *testing.T) {
@@ -86,3 +86,10 @@ func (test *e2eTest) serviceDescribeConcurrencyTarget(t *testing.T, serviceName,
 	expectedOutput := fmt.Sprintf("autoscaling.knative.dev/target: \"%s\"", concurrencyTarget)
 	assert.Check(t, util.ContainsAll(out, expectedOutput))
 }
+
+func (test *e2eTest) serviceUpdateWithInvalidValue(t *testing.T, serviceName string, args []string) {
+	out, err := test.kn.RunWithOpts(append([]string{"service", "update", serviceName}, args...), runOpts{NoNamespace: false})
+	assert.NilError(t, err)
+
+	assert.Check(t, util.ContainsAll(out, "Invalid"))
+}