diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index de8d15f1e5..9df77ebe89 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/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 } diff --git a/pkg/kn/commands/service/service_update_test.go b/pkg/kn/commands/service/service_update_test.go index 58582377da..c6b72992bf 100644 --- a/pkg/kn/commands/service/service_update_test.go +++ b/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) { diff --git a/pkg/serving/service.go b/pkg/serving/service.go index a80fcb9757..c0c23a2919 100644 --- a/pkg/serving/service.go +++ b/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) {