update: Change default to server-side generated revision names (exten…

…ded) (#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)
-	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)
 	}
 

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