update: Change default to server-side generated revision names (extended) (#1240)

* update: Change default to server-side generated revision names

Fixes #1144

* update: Fix wait loop for synthetic events for which the generations are already in sync

* fix assertion as a service label change does not create a new revision

* fix: --cluster-local does not create a new revision but just changes a service's label

* chore: Fixed formatting

* update: Check generation in update already before doing a watch

* fixed lint issue

* fix test assertions

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)

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.

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.

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.

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)
+	name := ""
-	if err != nil {
+	if p.RevisionName != "" {
-		return err
+		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)
 	}
 

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
 	}
 }
 

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) {

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",

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, "")

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()
 

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.
+	// place. Return if the service creates a new generation or not
-	UpdateServiceWithRetry(name string, updateFunc ServiceUpdateFunc, nrRetries int) error
+	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 {
+func (cl *knServingClient) UpdateService(service *servingv1.Service) (bool, error) {
-	_, err := cl.client.Services(cl.namespace).Update(context.TODO(), service, v1.UpdateOptions{})
+	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
 	}
 }
 

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) {
+func (sr *ServingRecorder) UpdateService(service interface{}, hasChanged bool, err error) {
-	sr.r.Add("UpdateService", []interface{}{service}, []interface{}{err})
+	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)
 }
 

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)

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
+				serviceReturn := newService("update-service")
-				return true, serviceUpdate, nil
+				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")
 	})
 }

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)
 }
 

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()

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

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.Added, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", messages[0], 1, 2)},
-		{Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionUnknown, "", messages[0])},
+		{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])},
+		{Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionUnknown, corev1.ConditionTrue, "", messages[1], 2, 2)},
-		{Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "")},
+		{Type: watch.Modified, Object: CreateTestServiceWithConditions(name, corev1.ConditionTrue, corev1.ConditionTrue, "", "", 2, 2)},
 	}, len(messages)
 }
 

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)

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)

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,