Template the names

pkg/kn/commands/service/configuration_edit_flags.go

@@ -38,8 +38,7 @@ type ConfigurationEditFlags struct {
 	RevisionName               string
 
 	// Preferences about how to do the action.
-	GenerateRevisionName bool
-	ForceCreate          bool
+	ForceCreate bool
 
 	// Bookkeeping
 	flags []string
@@ -50,47 +49,49 @@ type ResourceFlags struct {
 	Memory string
 }
 
+// markFlagMakesRevision indicates that a flag will create a new revision if you
+// set it.
+func (p *ConfigurationEditFlags) markFlagMakesRevision(f string) {
+	p.flags = append(p.flags, f)
+}
+
 func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 	command.Flags().StringVar(&p.Image, "image", "", "Image to run.")
-	p.flags = append(p.flags, "image")
+	p.markFlagMakesRevision("image")
 	command.Flags().StringArrayVarP(&p.Env, "env", "e", []string{},
 		"Environment variable to set. NAME=value; you may provide this flag "+
 			"any number of times to set multiple environment variables.")
-	p.flags = append(p.flags, "env")
+	p.markFlagMakesRevision("env")
 	command.Flags().StringVar(&p.RequestsFlags.CPU, "requests-cpu", "", "The requested CPU (e.g., 250m).")
-	p.flags = append(p.flags, "requests-cpu")
+	p.markFlagMakesRevision("requests-cpu")
 	command.Flags().StringVar(&p.RequestsFlags.Memory, "requests-memory", "", "The requested memory (e.g., 64Mi).")
-	p.flags = append(p.flags, "requests-memory")
+	p.markFlagMakesRevision("requests-memory")
 	command.Flags().StringVar(&p.LimitsFlags.CPU, "limits-cpu", "", "The limits on the requested CPU (e.g., 1000m).")
-	p.flags = append(p.flags, "limits-cpu")
+	p.markFlagMakesRevision("limits-cpu")
 	command.Flags().StringVar(&p.LimitsFlags.Memory, "limits-memory", "",
 		"The limits on the requested memory (e.g., 1024Mi).")
-	p.flags = append(p.flags, "limits-memory")
+	p.markFlagMakesRevision("limits-memory")
 	command.Flags().IntVar(&p.MinScale, "min-scale", 0, "Minimal number of replicas.")
-	p.flags = append(p.flags, "min-scale")
+	p.markFlagMakesRevision("min-scale")
 	command.Flags().IntVar(&p.MaxScale, "max-scale", 0, "Maximal number of replicas.")
-	p.flags = append(p.flags, "max-scale")
+	p.markFlagMakesRevision("max-scale")
 	command.Flags().IntVar(&p.ConcurrencyTarget, "concurrency-target", 0,
 		"Recommendation for when to scale up based on the concurrent number of incoming request. "+
 			"Defaults to --concurrency-limit when given.")
-	p.flags = append(p.flags, "concurrency-target")
+	p.markFlagMakesRevision("concurrency-target")
 	command.Flags().IntVar(&p.ConcurrencyLimit, "concurrency-limit", 0,
 		"Hard Limit of concurrent requests to be processed by a single replica.")
-	p.flags = append(p.flags, "concurrency-limit")
+	p.markFlagMakesRevision("concurrency-limit")
 	command.Flags().Int32VarP(&p.Port, "port", "p", 0, "The port where application listens on.")
-	p.flags = append(p.flags, "port")
-	command.Flags().StringVar(&p.RevisionName, "revision-name", "",
+	p.markFlagMakesRevision("port")
+	command.Flags().StringVar(&p.RevisionName, "revision-name", "{{.Service}}-{{.Random 5}}-{{.Generation}}",
 		"The revision name to set. If you don't add the service name as a prefix, it'll be added for you.")
-	p.flags = append(p.flags, "revision-name")
+	p.markFlagMakesRevision("revision-name")
 }
 
 // AddUpdateFlags adds the flags specific to update.
 func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) {
 	p.addSharedFlags(command)
-
-	command.Flags().BoolVar(&p.GenerateRevisionName, "generate-revision-name", true,
-		"Automatically generate a revision name client-side. If false, the revision name is cleared.")
-	p.flags = append(p.flags, "generate-revision-name")
 }
 
 // AddCreateFlags adds the flags specific to create
@@ -111,8 +112,6 @@ func (p *ConfigurationEditFlags) Apply(
 		return err
 	}
 
-	anyMutation := p.AnyMutation(cmd)
-
 	envMap := map[string]string{}
 	for _, pairStr := range p.Env {
 		pairSlice := strings.SplitN(pairStr, "=", 2)
@@ -127,19 +126,16 @@ func (p *ConfigurationEditFlags) Apply(
 		return err
 	}
 
-	name := ""
-	if cmd.Flags().Changed("revision-name") {
-		name = p.RevisionName
-		if !strings.HasPrefix(name, service.Name+"-") {
-			name = service.Name + "-" + name
-		}
-	} else if p.GenerateRevisionName {
-		name = servinglib.GenerateRevisionName(service)
+	name, err := servinglib.GenerateRevisionName(p.RevisionName, service)
+	if err != nil {
+		return err
 	}
-	if anyMutation {
+
+	if p.AnyMutation(cmd) {
 		err = servinglib.UpdateName(template, name)
 		if err == servinglib.ApiTooOldError && !cmd.Flags().Changed("revision-name") {
-			// pass
+			// Ignore the error if we don't support revision names and nobody
+			// explicitly asked for one.
 		} else if err != nil {
 			return err
 		}

pkg/kn/commands/service/service_update_test.go

@@ -22,6 +22,7 @@ import (
 	"testing"
 
 	"gotest.tools/assert"
+	"gotest.tools/assert/cmp"
 
 	"github.com/knative/client/pkg/kn/commands"
 	servinglib "github.com/knative/client/pkg/serving"
@@ -176,20 +177,8 @@ func TestServiceUpdateRevisionNameExplicit(t *testing.T) {
 
 	template.Name = "foo-asdf"
 
-	// Test prefix added by command
-	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "--revision-name", "xyzzy", "--namespace", "bar", "--async"}, false)
-	assert.NilError(t, err)
-	if !action.Matches("update", "services") {
-		t.Fatalf("Bad action %v", action)
-	}
-
-	template, err = servinglib.RevisionTemplateOfService(updated)
-	assert.NilError(t, err)
-	assert.Equal(t, "foo-xyzzy", template.Name)
-
 	// Test user provides prefix
-	action, updated, _, err = fakeServiceUpdate(orig, []string{
+	action, updated, _, err := fakeServiceUpdate(orig, []string{
 		"service", "update", "foo", "--revision-name", "foo-dogs", "--namespace", "bar", "--async"}, false)
 	assert.NilError(t, err)
 	if !action.Matches("update", "services") {
@@ -235,7 +224,7 @@ func TestServiceUpdateRevisionNameCleared(t *testing.T) {
 	template.Name = "foo-asdf"
 
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--generate-revision-name=false", "--async"}, false)
+		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--revision-name=", "--async"}, false)
 
 	assert.NilError(t, err)
 	if !action.Matches("update", "services") {
@@ -244,7 +233,7 @@ func TestServiceUpdateRevisionNameCleared(t *testing.T) {
 
 	template, err = servinglib.RevisionTemplateOfService(updated)
 	assert.NilError(t, err)
-	assert.Assert(t, template.Name == "")
+	assert.Assert(t, cmp.Equal(template.Name, ""))
 }
 
 func TestServiceUpdateRevisionNameNoMutationNoChange(t *testing.T) {

pkg/serving/service.go

@@ -15,10 +15,11 @@
 package serving
 
 import (
+	"bytes"
 	"errors"
-	"fmt"
 	"math/rand"
 	"strings"
+	"text/template"
 
 	servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1"
 )
@@ -46,16 +47,33 @@ var charChoices = []string{
 	"y", "z",
 }
 
+type revisionTemplContext struct {
+	Service    string
+	Generation int64
+}
+
+func (c *revisionTemplContext) Random(l int) string {
+	chars := make([]string, 0, l)
+	for i := 0; i < l; i++ {
+		chars = append(chars, charChoices[rand.Int()%len(charChoices)])
+	}
+	return strings.Join(chars, "")
+}
+
 // GenerateRevisionName returns an automatically-generated name suitable for the
 // next revision of the given service.
-func GenerateRevisionName(service *servingv1alpha1.Service) string {
-	prefix := service.Name
-	generation := service.Generation + 1
-	chars := []string{}
-	for i := 0; i < 5; i++ {
-		chars = append(chars, charChoices[rand.Int()%len(charChoices)])
+func GenerateRevisionName(nameTempl string, service *servingv1alpha1.Service) (string, error) {
+	templ, err := template.New("revisionName").Parse(nameTempl)
+	if err != nil {
+		return "", err
+	}
+	context := &revisionTemplContext{
+		Service:    service.Name,
+		Generation: service.Generation + 1,
 	}
-	return fmt.Sprintf("%s-%s-%d", prefix, strings.Join(chars, ""), generation)
+	buf := new(bytes.Buffer)
+	err = templ.Execute(buf, context)
+	return buf.String(), err
 }
 
 func getConfiguration(service *servingv1alpha1.Service) (*servingv1alpha1.ConfigurationSpec, error) {