Repository: knative/client (go)

Issue:
Use server-generated revisions names as default for services (instead of BYO revision name)
<!-- If you need to report a security issue with Knative, send an email to knative-security@googlegroups.com. -->

<!--
Optional classifications (remove the '> ' to select)
> /kind question
> /kind good-first-issue
> /kind usage
> /kind doc
-->

As discussed in https://github.com/knative/serving/issues/9544 BYO revision names that we use by default (i.e. the client set the revision name and not the serving controller), has some issues when it comes to sink bindings, but also for `kn service apply` we can't use BYO revision names. Tbh, I always have a hard time to remember the actual use case for what BYO revision names are used (and again, I forgot about it ;-(. I conclude, that the use case for BYO revision names is so rare that it does not justified to use this as a default (when contrasting this to the trouble that it brings elsewhere).

My suggestion is to make server-side naming the default and *only* when `--revision-name` is specified we switch to BYO revision names. And the moment, switching off BYO revision naming is even very obfuscated as one has to provide a `--revision-name=` (i.e. an empty name) so switch this off.

The switch should work transparently except for that one use that I don't remember anymore (@evankanderson I think you explained it to me earlier, but I forgot again. sorry :( . From a UX perspective we just would not allow anymore a `--revision-name` without or empty argument, and just use server-side revision naming by default when this option is not given.

Opinions ?

Pull request:
update: Change default to server-side generated revision names (extended)
Fixes #1144.

This is an extension of #1236 with better handling for service updates. If tests are passing then this PR should be merged instead of #1236 

Patch:
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
index 08bb9fe9..5e36c203 100644
--- a/CHANGELOG.adoc
+++ b/CHANGELOG.adoc
@@ -37,6 +37,10 @@
 | 🐣
 | Do not print `serviceUID` and `configUID` labels in service export results
 | https://github.com/knative/client/pull/1194[#1194]
+
+| ✨
+| Use server-side generated revision names by default now. For BYO revision names use `--revision-name` for service commands
+| https://github.com/knative/client/issues/1240[#1240]
 |===
 
 ## v0.20.0 (2021-01-14)
diff --git a/docs/cmd/kn_service_apply.md b/docs/cmd/kn_service_apply.md
index b8456f3b..e6c52bad 100644
--- a/docs/cmd/kn_service_apply.md
+++ b/docs/cmd/kn_service_apply.md
@@ -58,7 +58,7 @@ kn service apply s0 --filename my-svc.yml
   -p, --port string                       The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
       --pull-secret string                Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --request strings                   The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
-      --revision-name string              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. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}")
+      --revision-name string              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. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants (e.g. {{.Service}}-{{.Random 5}}-{{.Generation}})
       --scale string                      Set the Minimum and Maximum number of replicas. You can use this flag to set both to a single value, or set a range with min/max values, or set either min or max values without specifying the other. Example: --scale 5 (scale-min = 5, scale-max = 5) or --scale 1..5 (scale-min = 1, scale-max = 5) or --scale 1.. (scale-min = 1, scale-max = unchanged) or --scale ..5 (scale-min = unchanged, scale-max = 5)
       --scale-init int                    Initial number of replicas with which a service starts. Can be 0 or a positive integer.
       --scale-max int                     Maximum number of replicas.
diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md
index 7dd6f659..365f35f0 100644
--- a/docs/cmd/kn_service_create.md
+++ b/docs/cmd/kn_service_create.md
@@ -83,7 +83,7 @@ kn service create NAME --image IMAGE
   -p, --port string                       The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
       --pull-secret string                Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --request strings                   The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
-      --revision-name string              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. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}")
+      --revision-name string              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. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants (e.g. {{.Service}}-{{.Random 5}}-{{.Generation}})
       --scale string                      Set the Minimum and Maximum number of replicas. You can use this flag to set both to a single value, or set a range with min/max values, or set either min or max values without specifying the other. Example: --scale 5 (scale-min = 5, scale-max = 5) or --scale 1..5 (scale-min = 1, scale-max = 5) or --scale 1.. (scale-min = 1, scale-max = unchanged) or --scale ..5 (scale-min = unchanged, scale-max = 5)
       --scale-init int                    Initial number of replicas with which a service starts. Can be 0 or a positive integer.
       --scale-max int                     Maximum number of replicas.
diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md
index 8eeeae7c..8a44fa30 100644
--- a/docs/cmd/kn_service_update.md
+++ b/docs/cmd/kn_service_update.md
@@ -66,7 +66,7 @@ kn service update NAME
   -p, --port string                       The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
       --pull-secret string                Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --request strings                   The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
-      --revision-name string              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. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}")
+      --revision-name string              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. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants (e.g. {{.Service}}-{{.Random 5}}-{{.Generation}})
       --scale string                      Set the Minimum and Maximum number of replicas. You can use this flag to set both to a single value, or set a range with min/max values, or set either min or max values without specifying the other. Example: --scale 5 (scale-min = 5, scale-max = 5) or --scale 1..5 (scale-min = 1, scale-max = 5) or --scale 1.. (scale-min = 1, scale-max = unchanged) or --scale ..5 (scale-min = unchanged, scale-max = 5)
       --scale-init int                    Initial number of replicas with which a service starts. Can be 0 or a positive integer.
       --scale-max int                     Maximum number of replicas.
diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go
index 19afcc78..f714d916 100644
--- a/pkg/kn/commands/service/configuration_edit_flags.go
+++ b/pkg/kn/commands/service/configuration_edit_flags.go
@@ -104,9 +104,6 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 
 	knflags.AddBothBoolFlagsUnhidden(command.Flags(), &p.ClusterLocal, "cluster-local", "", false,
 		"Specify that the service be private. (--no-cluster-local will make the service publicly available)")
-	//TODO: Need to also not change revision when already set (solution to issue #646)
-	p.markFlagMakesRevision("cluster-local")
-	p.markFlagMakesRevision("no-cluster-local")
 
 	command.Flags().IntVar(&p.ConcurrencyTarget, "concurrency-target", 0,
 		"Recommendation for when to scale up based on the concurrent number of incoming request. "+
@@ -140,11 +137,12 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 			"precedence over the \"label\" flag.")
 	p.markFlagMakesRevision("label-revision")
 
-	command.Flags().StringVar(&p.RevisionName, "revision-name", "{{.Service}}-{{.Random 5}}-{{.Generation}}",
+	command.Flags().StringVar(&p.RevisionName, "revision-name", "",
 		"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. "+
 			"Accepts golang templates, allowing {{.Service}} for the service name, "+
-			"{{.Generation}} for the generation, and {{.Random [n]}} for n random consonants.")
+			"{{.Generation}} for the generation, and {{.Random [n]}} for n random consonants "+
+			"(e.g. {{.Service}}-{{.Random 5}}-{{.Generation}})")
 	p.markFlagMakesRevision("revision-name")
 
 	knflags.AddBothBoolFlagsUnhidden(command.Flags(), &p.LockToDigest, "lock-to-digest", "", true,
@@ -206,30 +204,31 @@ func (p *ConfigurationEditFlags) Apply(
 		return err
 	}
 
-	name, err := servinglib.GenerateRevisionName(p.RevisionName, service)
-	if err != nil {
-		return err
-	}
+	name := ""
+	if p.RevisionName != "" {
+		name, err = servinglib.GenerateRevisionName(p.RevisionName, service)
+		if err != nil {
+			return err
+		}
 
-	if p.AnyMutation(cmd) {
-		template.Name = name
+		if p.AnyMutation(cmd) {
+			template.Name = name
+		}
 	}
 
-	imageSet := false
-	if cmd.Flags().Changed("image") {
-		imageSet = true
-	}
 	_, userImagePresent := template.Annotations[servinglib.UserImageAnnotationKey]
 	freezeMode := userImagePresent || cmd.Flags().Changed("lock-to-digest")
 	if p.LockToDigest && p.AnyMutation(cmd) && freezeMode {
 		servinglib.SetUserImageAnnot(template)
-		if !imageSet {
+		if !cmd.Flags().Changed("image") {
 			err = servinglib.FreezeImageToDigest(template, baseRevision)
 			if err != nil {
 				return err
 			}
 		}
-	} else if !p.LockToDigest {
+	}
+
+	if !p.LockToDigest {
 		servinglib.UnsetUserImageAnnot(template)
 	}
 
diff --git a/pkg/kn/commands/service/create.go b/pkg/kn/commands/service/create.go
index f4bcd77f..395db100 100644
--- a/pkg/kn/commands/service/create.go
+++ b/pkg/kn/commands/service/create.go
@@ -155,10 +155,14 @@ func createService(client clientservingv1.KnServingClient, service *servingv1.Se
 }
 
 func replaceService(client clientservingv1.KnServingClient, service *servingv1.Service, waitFlags commands.WaitFlags, out io.Writer, targetFlag string) error {
-	err := prepareAndUpdateService(client, service)
+	changed, err := prepareAndUpdateService(client, service)
 	if err != nil {
 		return err
 	}
+	if !changed {
+		fmt.Fprintf(out, "Service '%s' replaced in namespace '%s' (unchanged).\n", service.Name, client.Namespace())
+		return nil
+	}
 	return waitIfRequested(client, waitFlags, service.Name, "Replacing", "replaced", targetFlag, out)
 }
 
@@ -171,12 +175,12 @@ func waitIfRequested(client clientservingv1.KnServingClient, waitFlags commands.
 	return waitForServiceToGetReady(client, serviceName, waitFlags.TimeoutInSeconds, verbDone, out)
 }
 
-func prepareAndUpdateService(client clientservingv1.KnServingClient, service *servingv1.Service) error {
+func prepareAndUpdateService(client clientservingv1.KnServingClient, service *servingv1.Service) (bool, error) {
 	var retries = 0
 	for {
 		existingService, err := client.GetService(service.Name)
 		if err != nil {
-			return err
+			return false, err
 		}
 
 		// Copy over some annotations that we want to keep around. Erase others
@@ -200,16 +204,16 @@ func prepareAndUpdateService(client clientservingv1.KnServingClient, service *se
 		}
 
 		service.ResourceVersion = existingService.ResourceVersion
-		err = client.UpdateService(service)
+		changed, err := client.UpdateService(service)
 		if err != nil {
 			// Retry to update when a resource version conflict exists
 			if apierrors.IsConflict(err) && retries < MaxUpdateRetries {
 				retries++
 				continue
 			}
-			return err
+			return changed, err
 		}
-		return nil
+		return changed, nil
 	}
 }
 
diff --git a/pkg/kn/commands/service/update.go b/pkg/kn/commands/service/update.go
index 00817044..6343824b 100644
--- a/pkg/kn/commands/service/update.go
+++ b/pkg/kn/commands/service/update.go
@@ -109,12 +109,19 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
 			}
 
 			// Do the actual update with retry in case of conflicts
-			err = client.UpdateServiceWithRetry(name, updateFunc, MaxUpdateRetries)
+			changed, err := client.UpdateServiceWithRetry(name, updateFunc, MaxUpdateRetries)
 			if err != nil {
 				return err
 			}
-
 			out := cmd.OutOrStdout()
+
+			// No need to wait if not changed
+			if !changed {
+				fmt.Fprintf(out, "Service '%s' updated in namespace '%s'.\n", args[0], namespace)
+				fmt.Fprintln(out, "No new revision has been created.")
+				return nil
+			}
+
 			if waitFlags.Wait && targetFlag == "" {
 				fmt.Fprintf(out, "Updating Service '%s' in namespace '%s':\n", args[0], namespace)
 				fmt.Fprintln(out, "")
diff --git a/pkg/serving/v1/client.go b/pkg/serving/v1/client.go
index c5aa30d2..68c5d53c 100644
--- a/pkg/serving/v1/client.go
+++ b/pkg/serving/v1/client.go
@@ -62,12 +62,12 @@ type KnServingClient interface {
 
 	// UpdateService updates the given service. For a more robust variant with automatic
 	// conflict resolution see UpdateServiceWithRetry
-	UpdateService(service *servingv1.Service) error
+	UpdateService(service *servingv1.Service) (bool, error)
 
 	// UpdateServiceWithRetry updates service and retries if there is a version conflict.
 	// The updateFunc receives a deep copy of the existing service and can add update it in
-	// place.
-	UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, nrRetries int) error
+	// place. Return if the service creates a new generation or not
+	UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, nrRetries int) (bool, error)
 
 	// Apply a service's definition to the cluster. The full service declaration needs to be provided,
 	// which is different to UpdateService which can also do a partial update. If the given
@@ -241,36 +241,37 @@ func (cl *knServingClient) CreateService(service *servingv1.Service) error {
 }
 
 // Update the given service
-func (cl *knServingClient) UpdateService(service *servingv1.Service) error {
-	_, err := cl.client.Services(cl.namespace).Update(context.TODO(), service, v1.UpdateOptions{})
+func (cl *knServingClient) UpdateService(service *servingv1.Service) (bool, error) {
+	updated, err := cl.client.Services(cl.namespace).Update(context.TODO(), service, v1.UpdateOptions{})
 	if err != nil {
-		return err
+		return false, err
 	}
-	return updateServingGvk(service)
+	changed := service.ObjectMeta.Generation != updated.ObjectMeta.Generation
+	return changed, updateServingGvk(service)
 }
 
 // Update the given service with a retry in case of a conflict
-func (cl *knServingClient) UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, nrRetries int) error {
+func (cl *knServingClient) UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, nrRetries int) (bool, error) {
 	return updateServiceWithRetry(cl, name, updateFunc, nrRetries)
 }
 
 // Extracted to be usable with the Mocking client
-func updateServiceWithRetry(cl KnServingClient, name string, updateFunc ServiceUpdateFunc, nrRetries int) error {
+func updateServiceWithRetry(cl KnServingClient, name string, updateFunc ServiceUpdateFunc, nrRetries int) (bool, error) {
 	var retries = 0
 	for {
 		service, err := cl.GetService(name)
 		if err != nil {
-			return err
+			return false, err
 		}
 		if service.GetDeletionTimestamp() != nil {
-			return fmt.Errorf("can't update service %s because it has been marked for deletion", name)
+			return false, fmt.Errorf("can't update service %s because it has been marked for deletion", name)
 		}
 		updatedService, err := updateFunc(service.DeepCopy())
 		if err != nil {
-			return err
+			return false, err
 		}
 
-		err = cl.UpdateService(updatedService)
+		changed, err := cl.UpdateService(updatedService)
 		if err != nil {
 			// Retry to update when a resource version conflict exists
 			if apierrors.IsConflict(err) && retries < nrRetries {
@@ -279,9 +280,9 @@ func updateServiceWithRetry(cl KnServingClient, name string, updateFunc ServiceU
 				time.Sleep(time.Second)
 				continue
 			}
-			return fmt.Errorf("giving up after %d retries: %w", nrRetries, err)
+			return false, fmt.Errorf("giving up after %d retries: %w", nrRetries, err)
 		}
-		return nil
+		return changed, nil
 	}
 }
 
diff --git a/pkg/serving/v1/client_mock.go b/pkg/serving/v1/client_mock.go
index 4d1f8951..c2256743 100644
--- a/pkg/serving/v1/client_mock.go
+++ b/pkg/serving/v1/client_mock.go
@@ -90,17 +90,17 @@ func (c *MockKnServingClient) CreateService(service *servingv1.Service) error {
 }
 
 // Update the given service
-func (sr *ServingRecorder) UpdateService(service interface{}, err error) {
-	sr.r.Add("UpdateService", []interface{}{service}, []interface{}{err})
+func (sr *ServingRecorder) UpdateService(service interface{}, hasChanged bool, err error) {
+	sr.r.Add("UpdateService", []interface{}{service}, []interface{}{hasChanged, err})
 }
 
-func (c *MockKnServingClient) UpdateService(service *servingv1.Service) error {
+func (c *MockKnServingClient) UpdateService(service *servingv1.Service) (bool, error) {
 	call := c.recorder.r.VerifyCall("UpdateService", service)
-	return mock.ErrorOrNil(call.Result[0])
+	return call.Result[0].(bool), mock.ErrorOrNil(call.Result[1])
 }
 
 // Delegate to shared retry method
-func (c *MockKnServingClient) UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, maxRetry int) error {
+func (c *MockKnServingClient) UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, maxRetry int) (bool, error) {
 	return updateServiceWithRetry(c, name, updateFunc, maxRetry)
 }
 
diff --git a/pkg/serving/v1/gitops.go b/pkg/serving/v1/gitops.go
index 5e342045..d27ad8d4 100644
--- a/pkg/serving/v1/gitops.go
+++ b/pkg/serving/v1/gitops.go
@@ -178,17 +178,17 @@ func writeFile(obj runtime.Object, fp, format string) error {
 
 // UpdateService updates the service in
 // the local directory
-func (cl *knServingGitOpsClient) UpdateService(service *servingv1.Service) error {
+func (cl *knServingGitOpsClient) UpdateService(service *servingv1.Service) (bool, error) {
 	// check if file exist
 	if _, err := cl.GetService(service.ObjectMeta.Name); err != nil {
-		return err
+		return false, err
 	}
 	// replace file
-	return cl.CreateService(service)
+	return true, cl.CreateService(service)
 }
 
 // UpdateServiceWithRetry updates the service in the local directory
-func (cl *knServingGitOpsClient) UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, nrRetries int) error {
+func (cl *knServingGitOpsClient) UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, nrRetries int) (bool, error) {
 	return updateServiceWithRetry(cl, name, updateFunc, nrRetries)
 }
 
diff --git a/pkg/wait/wait_for_ready.go b/pkg/wait/wait_for_ready.go
index 0d0ddb53..00b1b3d3 100644
--- a/pkg/wait/wait_for_ready.go
+++ b/pkg/wait/wait_for_ready.go
@@ -166,6 +166,18 @@ func (w *waitForReadyConfig) waitForReadyCondition(watcher watch.Interface, star
 				return true, false, nil
 			}
 
+			// Check whether resource is in sync already (meta.generation == status.observedGeneration)
+			inSync, err := generationCheck(event.Object)
+			if err != nil {
+				return false, false, err
+			}
+
+			// Skip events if generations has not yet been consolidated, regardless of type.
+			// Wait for the next event to come in until the generations align
+			if !inSync {
+				continue
+			}
+
 			// Skip event if its not a MODIFIED event, as only MODIFIED events update the condition
 			// we are looking for.
 			// This will filter out all synthetic ADDED events that created bt the API server for
@@ -180,15 +192,6 @@ func (w *waitForReadyConfig) waitForReadyCondition(watcher watch.Interface, star
 				continue
 			}
 
-			// Skip event if generations has not yet been consolidated
-			inSync, err := generationCheck(event.Object)
-			if err != nil {
-				return false, false, err
-			}
-			if !inSync {
-				continue
-			}
-
 			conditions, err := w.conditionsExtractor(event.Object)
 			if err != nil {
 				return false, false, err


Test patch:
diff --git a/pkg/kn/commands/service/create_test.go b/pkg/kn/commands/service/create_test.go
index d43ac93a..dbf4888b 100644
--- a/pkg/kn/commands/service/create_test.go
+++ b/pkg/kn/commands/service/create_test.go
@@ -137,6 +137,9 @@ func TestServiceCreateImage(t *testing.T) {
 		!strings.Contains(output, commands.FakeNamespace) {
 		t.Fatalf("wrong stdout message: %v", output)
 	}
+
+	// No revision name set by default
+	assert.Assert(t, template.Name == "")
 }
 
 func TestServiceCreateWithMultipleImages(t *testing.T) {
diff --git a/pkg/kn/commands/service/service_update_mock_test.go b/pkg/kn/commands/service/service_update_mock_test.go
index 7ef7a602..4bfa10e9 100644
--- a/pkg/kn/commands/service/service_update_mock_test.go
+++ b/pkg/kn/commands/service/service_update_mock_test.go
@@ -131,7 +131,7 @@ func recordServiceUpdateWithSuccess(r *clientservingv1.ServingRecorder, svcName
 	r.GetService(svcName, nil, errors.NewNotFound(servingv1.Resource("service"), svcName))
 	r.CreateService(newService, nil)
 	r.GetService(svcName, newService, nil)
-	r.UpdateService(updatedService, nil)
+	r.UpdateService(updatedService, true, nil)
 }
 
 func TestServiceUpdateEnvFromAddingWithConfigMap(t *testing.T) {
@@ -283,7 +283,7 @@ func TestServiceUpdateEnvFromRemovalWithConfigMap(t *testing.T) {
 	r.GetService(svcName, updatedService1, nil)
 	//r.UpdateService(updatedService2, nil) // since an error happens, update is not triggered here
 	r.GetService(svcName, updatedService2, nil)
-	r.UpdateService(updatedService3, nil)
+	r.UpdateService(updatedService3, true, nil)
 
 	output, err := executeServiceCommand(client,
 		"create", svcName, "--image", "gcr.io/foo/bar:baz",
@@ -372,7 +372,7 @@ func TestServiceUpdateEnvFromRemovalWithEmptyName(t *testing.T) {
 	r.CreateService(newService, nil)
 	r.GetService(svcName, newService, nil)
 	r.GetService(svcName, newService, nil)
-	r.UpdateService(updatedService1, nil)
+	r.UpdateService(updatedService1, true, nil)
 
 	output, err := executeServiceCommand(client,
 		"create", svcName, "--image", "gcr.io/foo/bar:baz",
@@ -625,11 +625,11 @@ func TestServiceUpdateEnvFromRemovalWithSecret(t *testing.T) {
 	r.GetService(svcName, nil, errors.NewNotFound(servingv1.Resource("service"), svcName))
 	r.CreateService(newService, nil)
 	r.GetService(svcName, newService, nil)
-	r.UpdateService(updatedService1, nil)
+	r.UpdateService(updatedService1, true, nil)
 	r.GetService(svcName, updatedService1, nil)
 	//r.UpdateService(updatedService2, nil) // since an error happens, update is not triggered here
 	r.GetService(svcName, updatedService2, nil)
-	r.UpdateService(updatedService3, nil)
+	r.UpdateService(updatedService3, true, nil)
 
 	output, err := executeServiceCommand(client,
 		"create", svcName, "--image", "gcr.io/foo/bar:baz",
@@ -1383,9 +1383,9 @@ func TestServiceUpdateWithRemovingMount(t *testing.T) {
 	r.GetService(svcName, nil, errors.NewNotFound(servingv1.Resource("service"), svcName))
 	r.CreateService(newService, nil)
 	r.GetService(svcName, newService, nil)
-	r.UpdateService(updatedService1, nil)
+	r.UpdateService(updatedService1, true, nil)
 	r.GetService(svcName, updatedService1, nil)
-	r.UpdateService(updatedService2, nil)
+	r.UpdateService(updatedService2, true, nil)
 
 	output, err := executeServiceCommand(client,
 		"create", svcName, "--image", "gcr.io/foo/bar:baz",
diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go
index da8f2201..cd633413 100644
--- a/pkg/kn/commands/service/update_test.go
+++ b/pkg/kn/commands/service/update_test.go
@@ -62,10 +62,12 @@ func fakeServiceUpdate(original *servingv1.Service, args []string) (
 			if !ok {
 				return true, nil, fmt.Errorf("wrong kind of action %v", action)
 			}
-			updated, ok = updateAction.GetObject().(*servingv1.Service)
+			given, ok := updateAction.GetObject().(*servingv1.Service)
 			if !ok {
 				return true, nil, errors.New("was passed the wrong object")
 			}
+			updated = given.DeepCopy()
+			updated.Generation = given.Generation + 1
 			return true, updated, nil
 		})
 	fakeServing.AddReactor("get", "services",
@@ -271,7 +273,7 @@ func TestServiceUpdateRevisionNameGenerated(t *testing.T) {
 
 	// Test prefix added by command
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--no-wait"})
+		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--no-wait", "--revision-name", "{{.Service}}-{{.Random 5}}-{{.Generation}}"})
 	assert.NilError(t, err)
 	if !action.Matches("update", "services") {
 		t.Fatalf("Bad action %v", action)
@@ -282,6 +284,24 @@ func TestServiceUpdateRevisionNameGenerated(t *testing.T) {
 	assert.Assert(t, !(template.Name == "foo-asdf"))
 }
 
+func TestServiceUpdateRevisionNameDefault(t *testing.T) {
+	orig := newEmptyService()
+
+	template := orig.Spec.Template
+	template.Name = "foo-asdf"
+
+	// Test prefix added by command
+	action, updated, _, err := fakeServiceUpdate(orig, []string{
+		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy"})
+	assert.NilError(t, err)
+	if !action.Matches("update", "services") {
+		t.Fatalf("Bad action %v", action)
+	}
+
+	template = updated.Spec.Template
+	assert.Assert(t, cmp.Equal(template.Name, ""))
+}
+
 func TestServiceUpdateRevisionNameCleared(t *testing.T) {
 	orig := newEmptyService()
 
diff --git a/pkg/serving/v1/client_mock_test.go b/pkg/serving/v1/client_mock_test.go
index a44f355c..3b3f9ec3 100644
--- a/pkg/serving/v1/client_mock_test.go
+++ b/pkg/serving/v1/client_mock_test.go
@@ -36,7 +36,7 @@ func TestMockKnClient(t *testing.T) {
 	recorder.ListServices(mock.Any(), nil, nil)
 	recorder.ListServices(mock.Any(), nil, nil)
 	recorder.CreateService(&servingv1.Service{}, nil)
-	recorder.UpdateService(&servingv1.Service{}, nil)
+	recorder.UpdateService(&servingv1.Service{}, false, nil)
 	recorder.ApplyService(&servingv1.Service{}, true, nil)
 	recorder.DeleteService("hello", time.Duration(10)*time.Second, nil)
 	recorder.WaitForService("hello", time.Duration(10)*time.Second, wait.NoopMessageCallback(), nil, 10*time.Second)
diff --git a/pkg/serving/v1/client_test.go b/pkg/serving/v1/client_test.go
index 9fa8d518..a5a2bf6e 100644
--- a/pkg/serving/v1/client_test.go
+++ b/pkg/serving/v1/client_test.go
@@ -157,27 +157,29 @@ func TestCreateService(t *testing.T) {
 func TestUpdateService(t *testing.T) {
 	serving, client := setup()
 	serviceUpdate := newService("update-service")
+	serviceUpdate.ObjectMeta.Generation = 2
 
 	serving.AddReactor("update", "services",
 		func(a clienttesting.Action) (bool, runtime.Object, error) {
 			assert.Equal(t, testNamespace, a.GetNamespace())
 			name := a.(clienttesting.UpdateAction).GetObject().(metav1.Object).GetName()
 			if name == serviceUpdate.Name {
-				serviceUpdate.Generation = 3
-				return true, serviceUpdate, nil
+				serviceReturn := newService("update-service")
+				serviceReturn.Generation = 3
+				return true, serviceReturn, nil
 			}
 			return true, nil, fmt.Errorf("error while updating service %s", name)
 		})
 
 	t.Run("updating a service without error", func(t *testing.T) {
-		err := client.UpdateService(serviceUpdate)
+		changed, err := client.UpdateService(serviceUpdate)
 		assert.NilError(t, err)
-		assert.Equal(t, int64(3), serviceUpdate.Generation)
+		assert.Assert(t, changed)
 		validateGroupVersionKind(t, serviceUpdate)
 	})
 
 	t.Run("updating a service with error", func(t *testing.T) {
-		err := client.UpdateService(newService("unknown"))
+		_, err := client.UpdateService(newService("unknown"))
 		assert.ErrorContains(t, err, "unknown")
 	})
 }
diff --git a/pkg/serving/v1/gitops_test.go b/pkg/serving/v1/gitops_test.go
index 009f419e..0c097fd4 100644
--- a/pkg/serving/v1/gitops_test.go
+++ b/pkg/serving/v1/gitops_test.go
@@ -105,13 +105,15 @@ func TestGitOpsOperations(t *testing.T) {
 		assert.DeepEqual(t, allServices, result)
 	})
 	t.Run("update service with retry foo", func(t *testing.T) {
-		err := fooclient.UpdateServiceWithRetry("foo", func(svc *servingv1.Service) (*servingv1.Service, error) {
+		changed, err := fooclient.UpdateServiceWithRetry("foo", func(svc *servingv1.Service) (*servingv1.Service, error) {
 			return svc, nil
 		}, 1)
+		assert.Assert(t, changed)
 		assert.NilError(t, err)
 	})
 	t.Run("update service foo", func(t *testing.T) {
-		err := fooclient.UpdateService(fooUpdateSvc)
+		changed, err := fooclient.UpdateService(fooUpdateSvc)
+		assert.Assert(t, changed)
 		assert.NilError(t, err)
 	})
 	t.Run("check updated service foo", func(t *testing.T) {
@@ -176,14 +178,17 @@ func TestGitOpsSingleFile(t *testing.T) {
 		assert.DeepEqual(t, testSvc, result)
 	})
 	t.Run("update service foo", func(t *testing.T) {
-		err := fooclient.UpdateService(updateSvc)
+		changed, err := fooclient.UpdateService(updateSvc)
 		assert.NilError(t, err)
+		assert.Assert(t, changed)
 
-		err = barclient.UpdateService(updateSvc)
+		changed, err = barclient.UpdateService(updateSvc)
 		assert.NilError(t, err)
+		assert.Assert(t, changed)
 
-		err = bazclient.UpdateService(updateSvc)
+		changed, err = bazclient.UpdateService(updateSvc)
 		assert.NilError(t, err)
+		assert.Assert(t, changed)
 	})
 	t.Run("list services", func(t *testing.T) {
 		result, err := fooclient.ListServices()
diff --git a/pkg/wait/wait_for_ready_test.go b/pkg/wait/wait_for_ready_test.go
index a8ffc93c..b909efc0 100644
--- a/pkg/wait/wait_for_ready_test.go
+++ b/pkg/wait/wait_for_ready_test.go
@@ -156,10 +156,10 @@ func pMessages(max int) []string {
 func peNormal(name string) ([]watch.Event, int) {
 	messages := pMessages(2)
 	return []watch.Event{
-		{Type: watch.Added, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", messages[0])},
-		{Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", messages[0])},
-		{Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionTrue, "", messages[1])},
-		{Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "")},
+		{Type: watch.Added, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", messages[0], 1, 2)},
+		{Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", messages[0], 2, 2)},
+		{Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionTrue, "", messages[1], 2, 2)},
+		{Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "", 2, 2)},
 	}, len(messages)
 }
 
diff --git a/test/e2e/revision_test.go b/test/e2e/revision_test.go
index 4ccdcf3f..c3d5a817 100644
--- a/test/e2e/revision_test.go
+++ b/test/e2e/revision_test.go
@@ -51,7 +51,7 @@ func TestRevision(t *testing.T) {
 	test.RevisionListWithService(r, "hello")
 
 	t.Log("update hello service and increase revision count to 3")
-	test.ServiceUpdate(r, "hello", "--env", "TARGET=kn", "--port", "8888")
+	test.ServiceUpdate(r, "hello", "--env", "TARGET=kn", "--port", "9000")
 
 	t.Log("delete three revisions with one revision a nonexistent")
 	existRevision1 := test.FindRevisionByGeneration(r, "hello", 1)
diff --git a/test/e2e/service_test.go b/test/e2e/service_test.go
index 9875026d..6b483c2f 100644
--- a/test/e2e/service_test.go
+++ b/test/e2e/service_test.go
@@ -96,7 +96,7 @@ func serviceCreatePrivateUpdatePublic(r *test.KnRunResultCollector, serviceName
 	out = r.KnTest().Kn().Run("service", "update", serviceName,
 		"--image", pkgtest.ImagePath("helloworld"), "--no-cluster-local")
 	r.AssertNoError(out)
-	assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, "service", serviceName, "updated", "namespace", r.KnTest().Kn().Namespace(), "ready"))
+	assert.Check(r.T(), util.ContainsAllIgnoreCase(out.Stdout, "service", serviceName, "no new revision", "namespace", r.KnTest().Kn().Namespace()))
 
 	out = r.KnTest().Kn().Run("service", "describe", serviceName, "--verbose")
 	r.AssertNoError(out)
diff --git a/test/e2e/traffic_split_test.go b/test/e2e/traffic_split_test.go
index 178c48ed..0b7857c2 100644
--- a/test/e2e/traffic_split_test.go
+++ b/test/e2e/traffic_split_test.go
@@ -292,7 +292,7 @@ func TestTrafficSplit(t *testing.T) {
 			serviceCreateWithOptions(r, serviceName, "--revision-name", rev1)
 
 			// existing state: traffic block having two targets
-			test.ServiceUpdate(r, serviceName, "--env", "TARGET=v2")
+			test.ServiceUpdate(r, serviceName, "--env", "TARGET=v2", "--revision-name", serviceName+"-rev-2")
 
 			// desired state: tag non-@latest revision with two tags and 50-50% traffic each
 			test.ServiceUpdate(r, serviceName,