From 6dcb613f2273e19dd9684a54f937c533b846c4e1 Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Thu, 18 Jul 2019 17:27:41 -0700 Subject: [PATCH 01/14] Groundwork for naming revisions automatically and deliberately --- docs/cmd/kn_service_create.md | 1 + docs/cmd/kn_service_update.md | 2 + .../service/configuration_edit_flags.go | 62 ++++++++++++++++--- 3 files changed, 57 insertions(+), 8 deletions(-) diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index 6aa44876eb..028065dc6a 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -51,6 +51,7 @@ kn service create NAME --image IMAGE [flags] --limits-memory string The limits on the requested memory (e.g., 1024Mi). --max-scale int Maximal number of replicas. --min-scale int Minimal number of replicas. + --name string The revision name prefix to set. Implies --generate-revision-name=false -n, --namespace string List the requested object(s) in given namespace. -p, --port int32 The port where application listens on. --requests-cpu string The requested CPU (e.g., 250m). diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md index b908587a90..763a7bb702 100644 --- a/docs/cmd/kn_service_update.md +++ b/docs/cmd/kn_service_update.md @@ -31,6 +31,7 @@ kn service update NAME [flags] --concurrency-limit int Hard Limit of concurrent requests to be processed by a single replica. --concurrency-target int Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given. -e, --env stringArray Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables. To unset, specify the environment variable name followed by a "-" (e.g., NAME-). + --generate-revision-name Automatically generate a revision name client-side. If false, the revision name is cleared. (default true) -h, --help help for update --image string Image to run. -l, --label stringArray Service label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-). @@ -38,6 +39,7 @@ kn service update NAME [flags] --limits-memory string The limits on the requested memory (e.g., 1024Mi). --max-scale int Maximal number of replicas. --min-scale int Minimal number of replicas. + --name string The revision name prefix to set. Implies --generate-revision-name=false -n, --namespace string List the requested object(s) in given namespace. -p, --port int32 The port where application listens on. --requests-cpu string The requested CPU (e.g., 250m). diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index aefab59823..fad3a93d50 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -27,16 +27,24 @@ import ( ) type ConfigurationEditFlags struct { + // Direct field manipulation Image string Env []string RequestsFlags, LimitsFlags ResourceFlags - ForceCreate bool MinScale int MaxScale int ConcurrencyTarget int ConcurrencyLimit int Port int32 Labels []string + NamePrefix string + + // Preferences about how to do the action. + GenerateRevisionName bool + ForceCreate bool + + // Bookkeeping + flags []string } type ResourceFlags struct { @@ -44,34 +52,63 @@ type ResourceFlags struct { Memory string } -func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) { +func (p *ConfigurationEditFlags) AddSharedFlags(command *cobra.Command) { command.Flags().StringVar(&p.Image, "image", "", "Image to run.") + p.flags = append(p.flags, "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. "+ "To unset, specify the environment variable name followed by a \"-\" (e.g., NAME-).") + p.flags = append(p.flags, "env") command.Flags().StringVar(&p.RequestsFlags.CPU, "requests-cpu", "", "The requested CPU (e.g., 250m).") + p.flags = append(p.flags, "requests-cpu") command.Flags().StringVar(&p.RequestsFlags.Memory, "requests-memory", "", "The requested memory (e.g., 64Mi).") + p.flags = append(p.flags, "requests-memory") command.Flags().StringVar(&p.LimitsFlags.CPU, "limits-cpu", "", "The limits on the requested CPU (e.g., 1000m).") - command.Flags().StringVar(&p.LimitsFlags.Memory, "limits-memory", "", "The limits on the requested memory (e.g., 1024Mi).") + p.flags = append(p.flags, "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") command.Flags().IntVar(&p.MinScale, "min-scale", 0, "Minimal number of replicas.") + p.flags = append(p.flags, "min-scale") command.Flags().IntVar(&p.MaxScale, "max-scale", 0, "Maximal number of replicas.") - 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.") - 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, "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") + 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") command.Flags().Int32VarP(&p.Port, "port", "p", 0, "The port where application listens on.") command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{}, "Service label to set. name=value; you may provide this flag "+ "any number of times to set multiple labels. "+ "To unset, specify the label name followed by a \"-\" (e.g., name-).") + p.flags = append(p.flags, "label") + p.flags = append(p.flags, "port") + command.Flags().StringVar(&p.NamePrefix, "name", "", + "The revision name prefix to set. Implies --generate-revision-name=false") + p.flags = append(p.flags, "name") +} +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") } func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) { - p.AddUpdateFlags(command) - command.Flags().BoolVar(&p.ForceCreate, "force", false, "Create service forcefully, replaces existing service if any.") + p.AddSharedFlags(command) + command.Flags().BoolVar(&p.ForceCreate, "force", false, + "Create service forcefully, replaces existing service if any.") command.MarkFlagRequired("image") } -func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *cobra.Command) error { +func (p *ConfigurationEditFlags) Apply( + service *servingv1alpha1.Service, + cmd *cobra.Command) error { template, err := servinglib.RevisionTemplateOfService(service) if err != nil { @@ -194,3 +231,12 @@ func (p *ConfigurationEditFlags) computeResources(resourceFlags ResourceFlags) ( return resourceList, nil } + +func (p *ConfigurationEditFlags) AnyMutation(cmd *cobra.Command) bool { + for _, flag := range p.flags { + if cmd.Flags().Changed(flag) { + return true + } + } + return false +} From 8080e3bbe45e4cb6eb15dc3900773072eb73805c Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Fri, 19 Jul 2019 16:27:48 -0700 Subject: [PATCH 02/14] Tests for name updates --- cmd/kn/main.go | 3 + docs/cmd/kn_service_create.md | 2 +- docs/cmd/kn_service_update.md | 2 +- .../service/configuration_edit_flags.go | 23 +++- .../commands/service/service_update_test.go | 104 ++++++++++++++++++ pkg/serving/config_changes.go | 6 + pkg/serving/config_changes_test.go | 7 ++ pkg/serving/service.go | 18 +++ 8 files changed, 160 insertions(+), 5 deletions(-) diff --git a/cmd/kn/main.go b/cmd/kn/main.go index 878fa7311d..dcb0476921 100644 --- a/cmd/kn/main.go +++ b/cmd/kn/main.go @@ -16,7 +16,9 @@ package main import ( "fmt" + "math/rand" "os" + "time" "github.com/knative/client/pkg/kn/core" "github.com/spf13/viper" @@ -30,6 +32,7 @@ var err error func main() { defer cleanup() + rand.Seed(time.Now().UnixNano()) err = core.NewDefaultKnCommand().Execute() if err != nil { fmt.Fprintln(os.Stderr, err) diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index 028065dc6a..aae175aad6 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -51,11 +51,11 @@ kn service create NAME --image IMAGE [flags] --limits-memory string The limits on the requested memory (e.g., 1024Mi). --max-scale int Maximal number of replicas. --min-scale int Minimal number of replicas. - --name string The revision name prefix to set. Implies --generate-revision-name=false -n, --namespace string List the requested object(s) in given namespace. -p, --port int32 The port where application listens on. --requests-cpu string The requested CPU (e.g., 250m). --requests-memory string The requested memory (e.g., 64Mi). + --revision-name string The revision name to set. If you don't add the service name as a prefix, it'll be added for you. --wait-timeout int Seconds to wait before giving up on waiting for service to be ready. (default 60) ``` diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md index 763a7bb702..d7bad5efd5 100644 --- a/docs/cmd/kn_service_update.md +++ b/docs/cmd/kn_service_update.md @@ -39,11 +39,11 @@ kn service update NAME [flags] --limits-memory string The limits on the requested memory (e.g., 1024Mi). --max-scale int Maximal number of replicas. --min-scale int Minimal number of replicas. - --name string The revision name prefix to set. Implies --generate-revision-name=false -n, --namespace string List the requested object(s) in given namespace. -p, --port int32 The port where application listens on. --requests-cpu string The requested CPU (e.g., 250m). --requests-memory string The requested memory (e.g., 64Mi). + --revision-name string The revision name to set. If you don't add the service name as a prefix, it'll be added for you. --wait-timeout int Seconds to wait before giving up on waiting for service to be ready. (default 60) ``` diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index fad3a93d50..4813135638 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -38,6 +38,7 @@ type ConfigurationEditFlags struct { Port int32 Labels []string NamePrefix string + RevisionName string // Preferences about how to do the action. GenerateRevisionName bool @@ -87,9 +88,9 @@ func (p *ConfigurationEditFlags) AddSharedFlags(command *cobra.Command) { "To unset, specify the label name followed by a \"-\" (e.g., name-).") p.flags = append(p.flags, "label") p.flags = append(p.flags, "port") - command.Flags().StringVar(&p.NamePrefix, "name", "", - "The revision name prefix to set. Implies --generate-revision-name=false") - p.flags = append(p.flags, "name") + command.Flags().StringVar(&p.RevisionName, "revision-name", "", + "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") } func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) { p.AddSharedFlags(command) @@ -132,7 +133,23 @@ func (p *ConfigurationEditFlags) Apply( return err } } + anyMutation := p.AnyMutation(cmd) + 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) + } + if anyMutation { + err = servinglib.UpdateName(template, name) + if err != nil { + return err + } + } if cmd.Flags().Changed("image") { err = servinglib.UpdateImage(template, p.Image) if err != nil { diff --git a/pkg/kn/commands/service/service_update_test.go b/pkg/kn/commands/service/service_update_test.go index aa3cb234e4..cef0d0ef8c 100644 --- a/pkg/kn/commands/service/service_update_test.go +++ b/pkg/kn/commands/service/service_update_test.go @@ -166,6 +166,110 @@ func TestServiceUpdateImage(t *testing.T) { } } +func TestServiceUpdateRevisionNameExplicit(t *testing.T) { + orig := newEmptyService() + + template, err := servinglib.RevisionTemplateOfService(orig) + if err != nil { + t.Fatal(err) + } + + 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{ + "service", "update", "foo", "--revision-name", "foo-dogs", "--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-dogs", template.Name) + +} + +func TestServiceUpdateRevisionNameGenerated(t *testing.T) { + orig := newEmptyService() + + template, err := servinglib.RevisionTemplateOfService(orig) + if err != nil { + t.Fatal(err) + } + + template.Name = "foo-asdf" + + // Test prefix added by command + action, updated, _, err := fakeServiceUpdate(orig, []string{ + "service", "update", "foo", "--image", "gcr.io/foo/quux: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.Assert(t, strings.HasPrefix(template.Name, "foo-")) + assert.Assert(t, !(template.Name == "foo-asdf")) +} + +func TestServiceUpdateRevisionNameCleared(t *testing.T) { + orig := newEmptyService() + + template, err := servinglib.RevisionTemplateOfService(orig) + if err != nil { + t.Fatal(err) + } + 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) + + 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.Assert(t, template.Name == "") +} + +func TestServiceUpdateRevisionNameNoMutationNoChange(t *testing.T) { + orig := newEmptyService() + + template, err := servinglib.RevisionTemplateOfService(orig) + if err != nil { + t.Fatal(err) + } + + template.Name = "foo-asdf" + + // Test prefix added by command + action, updated, _, err := fakeServiceUpdate(orig, []string{ + "service", "update", "foo", "--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, template.Name, "foo-asdf") +} + func TestServiceUpdateMaxMinScale(t *testing.T) { original := newEmptyService() diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 6bfdd66475..e33ee34f87 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -92,6 +92,12 @@ func UpdateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation return nil } +// Update the name +func UpdateName(template *servingv1alpha1.RevisionTemplateSpec, name string) error { + template.Name = name + return nil +} + // EnvToMap is an utility function to translate between the API list form of env vars, and the // more convenient map form. func EnvToMap(vars []corev1.EnvVar) (map[string]string, error) { diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index a3a3f9c393..ecbbeffef8 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -253,6 +253,13 @@ func TestUpdateContainerPort(t *testing.T) { checkPortUpdate(t, template, 80) } +func TestUpdateName(t *testing.T) { + template, _ := getV1alpha1Config() + err := UpdateName(template, "foo-asdf") + assert.NilError(t, err) + assert.Equal(t, "foo-asdf", template.Name) +} + func checkPortUpdate(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, port int32) { if template.Spec.GetContainer().Ports[0].ContainerPort != port { t.Error("Failed to update the container port") diff --git a/pkg/serving/service.go b/pkg/serving/service.go index 54c7f76d7b..44165b5956 100644 --- a/pkg/serving/service.go +++ b/pkg/serving/service.go @@ -16,6 +16,9 @@ package serving import ( "errors" + "fmt" + "math/rand" + "strings" servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1" ) @@ -38,6 +41,21 @@ func RevisionTemplateOfService(service *servingv1alpha1.Service) (*servingv1alph return config.DeprecatedRevisionTemplate, nil } +var charChoices = []string{ + "b", "c", "d", "f", "g", "h", "j", "k", "l", "m", "n", "p", "q", "r", "s", "t", "v", "w", "x", + "y", "z", +} + +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)]) + } + return fmt.Sprintf("%s-%s-%d", prefix, strings.Join(chars, ""), generation) +} + func getConfiguration(service *servingv1alpha1.Service) (*servingv1alpha1.ConfigurationSpec, error) { if service.Spec.DeprecatedRunLatest != nil { return &service.Spec.DeprecatedRunLatest.Configuration, nil From 7e35c5be242b4cff2755b91e32c1752833a026a7 Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Fri, 19 Jul 2019 16:52:35 -0700 Subject: [PATCH 03/14] Add godoc comments --- pkg/kn/commands/service/configuration_edit_flags.go | 12 +++++++++--- pkg/serving/config_changes.go | 2 +- pkg/serving/service.go | 2 ++ 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 4813135638..c98ec3428d 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -53,7 +53,7 @@ type ResourceFlags struct { Memory string } -func (p *ConfigurationEditFlags) AddSharedFlags(command *cobra.Command) { +func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) { command.Flags().StringVar(&p.Image, "image", "", "Image to run.") p.flags = append(p.flags, "image") command.Flags().StringArrayVarP(&p.Env, "env", "e", []string{}, @@ -92,21 +92,25 @@ func (p *ConfigurationEditFlags) AddSharedFlags(command *cobra.Command) { "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") } + +// AddUpdateFlags adds the flags specific to update. func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) { - p.AddSharedFlags(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 func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) { - p.AddSharedFlags(command) + p.addSharedFlags(command) command.Flags().BoolVar(&p.ForceCreate, "force", false, "Create service forcefully, replaces existing service if any.") command.MarkFlagRequired("image") } +// Apply mutates the given service according to the flags in the command. func (p *ConfigurationEditFlags) Apply( service *servingv1alpha1.Service, cmd *cobra.Command) error { @@ -249,6 +253,8 @@ func (p *ConfigurationEditFlags) computeResources(resourceFlags ResourceFlags) ( return resourceList, nil } +// AnyMutation returns tue if there are any revision template mutations in the +// command. func (p *ConfigurationEditFlags) AnyMutation(cmd *cobra.Command) bool { for _, flag := range p.flags { if cmd.Flags().Changed(flag) { diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index e33ee34f87..9c701da956 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -92,7 +92,7 @@ func UpdateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation return nil } -// Update the name +// UpdateName updates the revision name. func UpdateName(template *servingv1alpha1.RevisionTemplateSpec, name string) error { template.Name = name return nil diff --git a/pkg/serving/service.go b/pkg/serving/service.go index 44165b5956..a80fcb9757 100644 --- a/pkg/serving/service.go +++ b/pkg/serving/service.go @@ -46,6 +46,8 @@ var charChoices = []string{ "y", "z", } +// 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 From aa84fb5bcf4e135db362e6c4dff21b6e2a354f09 Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Fri, 19 Jul 2019 17:03:20 -0700 Subject: [PATCH 04/14] Changelog entry for naming flags --- CHANGELOG.adoc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index ec6da09de8..3df6cc4c10 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -38,6 +38,10 @@ | `kn service list` lists services sorted by alphabetical order | https://github.com/knative/client/pull/330[#330] +| 🎁 +| Add --revision-name flag +| https://github.com/knative/client/pull/282[#282] + |=== ## v0.2.0 (2019-07-10) From 97ab0a444f537e3184c8c2904f0a7e65070a5851 Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Fri, 19 Jul 2019 17:25:26 -0700 Subject: [PATCH 05/14] Error when trying to BYO revision name to an old-format service --- pkg/kn/commands/service/configuration_edit_flags.go | 4 +++- pkg/serving/config_changes.go | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index c98ec3428d..d102356bbe 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -150,7 +150,9 @@ func (p *ConfigurationEditFlags) Apply( } if anyMutation { err = servinglib.UpdateName(template, name) - if err != nil { + if err == servinglib.ApiTooOldError && !cmd.Flags().Changed("revision-name") { + // pass + } else if err != nil { return err } } diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 9c701da956..3e9d681919 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -16,6 +16,7 @@ package serving import ( "context" + "errors" "fmt" "strconv" @@ -92,8 +93,13 @@ func UpdateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation return nil } +var ApiTooOldError = errors.New("The service is using too old of an API format for the operation") + // UpdateName updates the revision name. func UpdateName(template *servingv1alpha1.RevisionTemplateSpec, name string) error { + if template.Spec.DeprecatedContainer != nil { + return ApiTooOldError + } template.Name = name return nil } From dfdcc7e15b25806e37ad7b5de7f2699eede07d28 Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Fri, 19 Jul 2019 18:06:32 -0700 Subject: [PATCH 06/14] fix tests again --- .../commands/service/service_update_test.go | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/pkg/kn/commands/service/service_update_test.go b/pkg/kn/commands/service/service_update_test.go index cef0d0ef8c..8d96f33741 100644 --- a/pkg/kn/commands/service/service_update_test.go +++ b/pkg/kn/commands/service/service_update_test.go @@ -167,7 +167,7 @@ func TestServiceUpdateImage(t *testing.T) { } func TestServiceUpdateRevisionNameExplicit(t *testing.T) { - orig := newEmptyService() + orig := newEmptyServiceBeta() template, err := servinglib.RevisionTemplateOfService(orig) if err != nil { @@ -202,7 +202,7 @@ func TestServiceUpdateRevisionNameExplicit(t *testing.T) { } func TestServiceUpdateRevisionNameGenerated(t *testing.T) { - orig := newEmptyService() + orig := newEmptyServiceBeta() template, err := servinglib.RevisionTemplateOfService(orig) if err != nil { @@ -226,7 +226,7 @@ func TestServiceUpdateRevisionNameGenerated(t *testing.T) { } func TestServiceUpdateRevisionNameCleared(t *testing.T) { - orig := newEmptyService() + orig := newEmptyServiceBeta() template, err := servinglib.RevisionTemplateOfService(orig) if err != nil { @@ -248,7 +248,7 @@ func TestServiceUpdateRevisionNameCleared(t *testing.T) { } func TestServiceUpdateRevisionNameNoMutationNoChange(t *testing.T) { - orig := newEmptyService() + orig := newEmptyServiceBeta() template, err := servinglib.RevisionTemplateOfService(orig) if err != nil { @@ -560,6 +560,23 @@ func newEmptyService() *v1alpha1.Service { } } +func newEmptyServiceBeta() *v1alpha1.Service { + ret := &v1alpha1.Service{ + TypeMeta: metav1.TypeMeta{ + Kind: "Service", + APIVersion: "knative.dev/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "default", + }, + Spec: v1alpha1.ServiceSpec{}, + } + ret.Spec.Template = &v1alpha1.RevisionTemplateSpec{} + ret.Spec.Template.Spec.Containers = []corev1.Container{{}} + return ret +} + func createMockServiceWithResources(t *testing.T, requestCPU, requestMemory, limitsCPU, limitsMemory string) *v1alpha1.Service { service := &v1alpha1.Service{ TypeMeta: metav1.TypeMeta{ From 476cc762c7cb38df6ead3da1b960e52a9c089333 Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Thu, 8 Aug 2019 14:03:08 -0700 Subject: [PATCH 07/14] Template the names --- .../service/configuration_edit_flags.go | 59 +++++++++---------- .../commands/service/service_update_test.go | 19 ++---- pkg/serving/service.go | 34 ++++++++--- 3 files changed, 59 insertions(+), 53 deletions(-) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index d102356bbe..0b82075601 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -41,8 +41,7 @@ type ConfigurationEditFlags struct { RevisionName string // Preferences about how to do the action. - GenerateRevisionName bool - ForceCreate bool + ForceCreate bool // Bookkeeping flags []string @@ -53,53 +52,56 @@ 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. "+ "To unset, specify the environment variable name followed by a \"-\" (e.g., NAME-).") - 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.markFlagMakesRevision("port") command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{}, "Service label to set. name=value; you may provide this flag "+ "any number of times to set multiple labels. "+ "To unset, specify the label name followed by a \"-\" (e.g., name-).") - p.flags = append(p.flags, "label") - p.flags = append(p.flags, "port") - command.Flags().StringVar(&p.RevisionName, "revision-name", "", + p.markFlagMakesRevision("label") + 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 @@ -139,19 +141,16 @@ func (p *ConfigurationEditFlags) Apply( } anyMutation := p.AnyMutation(cmd) - 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 8d96f33741..83f8a80a81 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) { From 681ac3d75c3577b2b8667018dc2c94f945f75bba Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Thu, 8 Aug 2019 15:42:11 -0700 Subject: [PATCH 08/14] Polsh, remove unused flags, generate helptext, add better helptext --- docs/cmd/kn_service_create.md | 2 +- docs/cmd/kn_service_update.md | 3 +-- pkg/kn/commands/service/configuration_edit_flags.go | 7 ++++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index aae175aad6..f7698be18b 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -55,7 +55,7 @@ kn service create NAME --image IMAGE [flags] -p, --port int32 The port where application listens on. --requests-cpu string The requested CPU (e.g., 250m). --requests-memory string The requested memory (e.g., 64Mi). - --revision-name string The revision name to set. If you don't add the service name as a prefix, it'll be added for you. + --revision-name string The revision name to set. Must start with the service name and a dash as a prefix. 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}}") --wait-timeout int Seconds to wait before giving up on waiting for service to be ready. (default 60) ``` diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md index d7bad5efd5..9029289001 100644 --- a/docs/cmd/kn_service_update.md +++ b/docs/cmd/kn_service_update.md @@ -31,7 +31,6 @@ kn service update NAME [flags] --concurrency-limit int Hard Limit of concurrent requests to be processed by a single replica. --concurrency-target int Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given. -e, --env stringArray Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables. To unset, specify the environment variable name followed by a "-" (e.g., NAME-). - --generate-revision-name Automatically generate a revision name client-side. If false, the revision name is cleared. (default true) -h, --help help for update --image string Image to run. -l, --label stringArray Service label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-). @@ -43,7 +42,7 @@ kn service update NAME [flags] -p, --port int32 The port where application listens on. --requests-cpu string The requested CPU (e.g., 250m). --requests-memory string The requested memory (e.g., 64Mi). - --revision-name string The revision name to set. If you don't add the service name as a prefix, it'll be added for you. + --revision-name string The revision name to set. Must start with the service name and a dash as a prefix. 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}}") --wait-timeout int Seconds to wait before giving up on waiting for service to be ready. (default 60) ``` diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 0b82075601..f59e530d32 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -58,6 +58,7 @@ func (p *ConfigurationEditFlags) markFlagMakesRevision(f string) { p.flags = append(p.flags, f) } +// addSharedFlags adds the flags common between create & update. func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) { command.Flags().StringVar(&p.Image, "image", "", "Image to run.") p.markFlagMakesRevision("image") @@ -94,8 +95,9 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) { "To unset, specify the label name followed by a \"-\" (e.g., name-).") p.markFlagMakesRevision("label") 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.") + "The revision name to set. Must start with the service name and a dash as a prefix. "+ + "Accepts golang templates, allowing {{.Service}} for the service name, "+ + "{{.Generation}} for the generation, and {{.Random [n]}} for n random consonants.") p.markFlagMakesRevision("revision-name") } @@ -139,7 +141,6 @@ func (p *ConfigurationEditFlags) Apply( return err } } - anyMutation := p.AnyMutation(cmd) name, err := servinglib.GenerateRevisionName(p.RevisionName, service) if err != nil { From bda6abf6b8dcf6ad132836106ed98158c7e8062c Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Thu, 8 Aug 2019 16:01:20 -0700 Subject: [PATCH 09/14] Decapitalize error msg --- pkg/serving/config_changes.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 3e9d681919..39eb8485e2 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -93,7 +93,7 @@ func UpdateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation return nil } -var ApiTooOldError = errors.New("The service is using too old of an API format for the operation") +var ApiTooOldError = errors.New("the service is using too old of an API format for the operation") // UpdateName updates the revision name. func UpdateName(template *servingv1alpha1.RevisionTemplateSpec, name string) error { From 4a9e90f1b7383c7be3d571d98ff348ad3d1c183e Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Fri, 9 Aug 2019 15:55:45 -0700 Subject: [PATCH 10/14] Respond to Roland comments --- docs/cmd/kn_service_create.md | 2 +- docs/cmd/kn_service_update.md | 2 +- pkg/kn/commands/service/configuration_edit_flags.go | 1 + pkg/kn/commands/service/service_update_test.go | 10 +++++----- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index f7698be18b..7800ca32ce 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -55,7 +55,7 @@ kn service create NAME --image IMAGE [flags] -p, --port int32 The port where application listens on. --requests-cpu string The requested CPU (e.g., 250m). --requests-memory string The requested memory (e.g., 64Mi). - --revision-name string The revision name to set. Must start with the service name and a dash as a prefix. 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. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}") --wait-timeout int Seconds to wait before giving up on waiting for service to be ready. (default 60) ``` diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md index 9029289001..b730ead7fe 100644 --- a/docs/cmd/kn_service_update.md +++ b/docs/cmd/kn_service_update.md @@ -42,7 +42,7 @@ kn service update NAME [flags] -p, --port int32 The port where application listens on. --requests-cpu string The requested CPU (e.g., 250m). --requests-memory string The requested memory (e.g., 64Mi). - --revision-name string The revision name to set. Must start with the service name and a dash as a prefix. 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. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}") --wait-timeout int Seconds to wait before giving up on waiting for service to be ready. (default 60) ``` diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index f59e530d32..c094682b21 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -96,6 +96,7 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) { p.markFlagMakesRevision("label") command.Flags().StringVar(&p.RevisionName, "revision-name", "{{.Service}}-{{.Random 5}}-{{.Generation}}", "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.") p.markFlagMakesRevision("revision-name") diff --git a/pkg/kn/commands/service/service_update_test.go b/pkg/kn/commands/service/service_update_test.go index 83f8a80a81..3f2835fcee 100644 --- a/pkg/kn/commands/service/service_update_test.go +++ b/pkg/kn/commands/service/service_update_test.go @@ -168,7 +168,7 @@ func TestServiceUpdateImage(t *testing.T) { } func TestServiceUpdateRevisionNameExplicit(t *testing.T) { - orig := newEmptyServiceBeta() + orig := newEmptyServiceBetaAPIStyle() template, err := servinglib.RevisionTemplateOfService(orig) if err != nil { @@ -191,7 +191,7 @@ func TestServiceUpdateRevisionNameExplicit(t *testing.T) { } func TestServiceUpdateRevisionNameGenerated(t *testing.T) { - orig := newEmptyServiceBeta() + orig := newEmptyServiceBetaAPIStyle() template, err := servinglib.RevisionTemplateOfService(orig) if err != nil { @@ -215,7 +215,7 @@ func TestServiceUpdateRevisionNameGenerated(t *testing.T) { } func TestServiceUpdateRevisionNameCleared(t *testing.T) { - orig := newEmptyServiceBeta() + orig := newEmptyServiceBetaAPIStyle() template, err := servinglib.RevisionTemplateOfService(orig) if err != nil { @@ -237,7 +237,7 @@ func TestServiceUpdateRevisionNameCleared(t *testing.T) { } func TestServiceUpdateRevisionNameNoMutationNoChange(t *testing.T) { - orig := newEmptyServiceBeta() + orig := newEmptyServiceBetaAPIStyle() template, err := servinglib.RevisionTemplateOfService(orig) if err != nil { @@ -549,7 +549,7 @@ func newEmptyService() *v1alpha1.Service { } } -func newEmptyServiceBeta() *v1alpha1.Service { +func newEmptyServiceBetaAPIStyle() *v1alpha1.Service { ret := &v1alpha1.Service{ TypeMeta: metav1.TypeMeta{ Kind: "Service", From 5d6618f925002e91aec44ad4e20d949ad66e1cba Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Fri, 9 Aug 2019 15:56:40 -0700 Subject: [PATCH 11/14] Forgot to add test file --- pkg/serving/service_test.go | 53 +++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 pkg/serving/service_test.go diff --git a/pkg/serving/service_test.go b/pkg/serving/service_test.go new file mode 100644 index 0000000000..0be01ab6ad --- /dev/null +++ b/pkg/serving/service_test.go @@ -0,0 +1,53 @@ +// Copyright © 2018 The Knative Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package serving + +import ( + "math/rand" + "testing" + + "gotest.tools/assert" + + servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1" +) + +type generateNameTest struct { + templ string + result string + err string +} + +func TestGenerateName(t *testing.T) { + rand.Seed(1) + someRandomChars := (&revisionTemplContext{}).Random(20) + service := &servingv1alpha1.Service{} + service.Name = "foo" + service.Generation = 3 + cases := []generateNameTest{ + {"{{.Service}}-v-{{.Generation}}", "foo-v-4", ""}, + {"foo-asdf", "foo-asdf", ""}, + {"{{.Bad}}", "", "can't evaluate field Bad"}, + {"{{.Service}}-{{.Random 5}}", "foo-" + someRandomChars[0:5], ""}, + } + for _, c := range cases { + rand.Seed(1) + name, err := GenerateRevisionName(c.templ, service) + if c.err != "" { + assert.ErrorContains(t, err, c.err) + } else { + assert.Equal(t, name, c.result) + } + } +} From 657a61e3e2f734e44b548edd4de6b5ab0e715012 Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Fri, 9 Aug 2019 16:27:19 -0700 Subject: [PATCH 12/14] Be a good citizen, add more tests, try to get coverage happy. --- pkg/serving/service_test.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/pkg/serving/service_test.go b/pkg/serving/service_test.go index 0be01ab6ad..a5a3017c79 100644 --- a/pkg/serving/service_test.go +++ b/pkg/serving/service_test.go @@ -51,3 +51,30 @@ func TestGenerateName(t *testing.T) { } } } + +func TestRevisionTemplateOfServiceNewStyle(t *testing.T) { + service := &servingv1alpha1.Service{} + service.Name = "foo" + template := &servingv1alpha1.RevisionTemplateSpec{} + service.Spec.Template = template + got, err := RevisionTemplateOfService(service) + assert.NilError(t, err) + assert.Equal(t, got, template) +} + +func TestRevisionTemplateOfServiceOldStyle(t *testing.T) { + service := &servingv1alpha1.Service{} + service.Name = "foo" + template := &servingv1alpha1.RevisionTemplateSpec{} + service.Spec.DeprecatedRunLatest = &servingv1alpha1.RunLatestType{} + service.Spec.DeprecatedRunLatest.Configuration.DeprecatedRevisionTemplate = template + got, err := RevisionTemplateOfService(service) + assert.NilError(t, err) + assert.Equal(t, got, template) +} + +func TestRevisionTemplateOfServiceError(t *testing.T) { + service := &servingv1alpha1.Service{} + _, err := RevisionTemplateOfService(service) + assert.ErrorContains(t, err, "does not specify") +} From ad67c222e749caea59631196e410eaaccb29ed95 Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Fri, 9 Aug 2019 16:32:16 -0700 Subject: [PATCH 13/14] true dat --- pkg/kn/commands/service/configuration_edit_flags.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index c094682b21..ed3927a8a0 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -256,7 +256,7 @@ func (p *ConfigurationEditFlags) computeResources(resourceFlags ResourceFlags) ( return resourceList, nil } -// AnyMutation returns tue if there are any revision template mutations in the +// AnyMutation returns true if there are any revision template mutations in the // command. func (p *ConfigurationEditFlags) AnyMutation(cmd *cobra.Command) bool { for _, flag := range p.flags { From 07897d1ecd7cf01332abf752ed7029e7c55a3e71 Mon Sep 17 00:00:00 2001 From: Naomi Seyfer Date: Sat, 10 Aug 2019 09:50:00 -0700 Subject: [PATCH 14/14] Re-add implicit service prefix if one was not added. --- pkg/serving/service.go | 14 +++++++++++++- pkg/serving/service_test.go | 3 +++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/pkg/serving/service.go b/pkg/serving/service.go index c0c23a2919..d003f6d271 100644 --- a/pkg/serving/service.go +++ b/pkg/serving/service.go @@ -73,7 +73,19 @@ func GenerateRevisionName(nameTempl string, service *servingv1alpha1.Service) (s } buf := new(bytes.Buffer) err = templ.Execute(buf, context) - return buf.String(), err + if err != nil { + return "", err + } + res := buf.String() + // Empty is ok. + if res == "" { + return res, nil + } + prefix := service.Name + "-" + if !strings.HasPrefix(res, prefix) { + res = prefix + res + } + return res, nil } func getConfiguration(service *servingv1alpha1.Service) (*servingv1alpha1.ConfigurationSpec, error) { diff --git a/pkg/serving/service_test.go b/pkg/serving/service_test.go index a5a3017c79..53369e57c7 100644 --- a/pkg/serving/service_test.go +++ b/pkg/serving/service_test.go @@ -40,6 +40,9 @@ func TestGenerateName(t *testing.T) { {"foo-asdf", "foo-asdf", ""}, {"{{.Bad}}", "", "can't evaluate field Bad"}, {"{{.Service}}-{{.Random 5}}", "foo-" + someRandomChars[0:5], ""}, + {"", "", ""}, + {"andrew", "foo-andrew", ""}, + {"{{.Random 5}}", "foo-" + someRandomChars[0:5], ""}, } for _, c := range cases { rand.Seed(1)