Specify names on service update and generate names client-side. (#282)

* Groundwork for naming revisions automatically and deliberately

* Tests for name updates

* Add godoc comments

* Changelog entry for naming flags

* Error when trying to BYO revision name to an old-format service

* fix tests again

* Template the names

* Polsh, remove unused flags, generate helptext, add better helptext

* Decapitalize error msg

* Respond to Roland comments

* Forgot to add test file

* Be a good citizen, add more tests, try to get coverage happy.

* true dat

* Re-add implicit service prefix if one was not added.

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)

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)

docs/cmd/kn_service_create.md

@@ -55,6 +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. 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)
 ```
 

docs/cmd/kn_service_update.md

@@ -42,6 +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. 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)
 ```
 

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
+	RevisionName               string
+
+	// Preferences about how to do the action.
+	ForceCreate bool
+
+	// Bookkeeping
+	flags []string
 }
 
 type ResourceFlags struct {
@@ -44,34 +52,73 @@ type ResourceFlags struct {
 	Memory string
 }
 
-func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) {
+// 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)
+}
+
+// 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")
 	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.markFlagMakesRevision("env")
 	command.Flags().StringVar(&p.RequestsFlags.CPU, "requests-cpu", "", "The requested CPU (e.g., 250m).")
+	p.markFlagMakesRevision("requests-cpu")
 	command.Flags().StringVar(&p.RequestsFlags.Memory, "requests-memory", "", "The requested memory (e.g., 64Mi).")
+	p.markFlagMakesRevision("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.markFlagMakesRevision("limits-cpu")
+	command.Flags().StringVar(&p.LimitsFlags.Memory, "limits-memory", "",
+		"The limits on the requested memory (e.g., 1024Mi).")
+	p.markFlagMakesRevision("limits-memory")
 	command.Flags().IntVar(&p.MinScale, "min-scale", 0, "Minimal number of replicas.")
+	p.markFlagMakesRevision("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.")
+	p.markFlagMakesRevision("max-scale")
-	command.Flags().IntVar(&p.ConcurrencyLimit, "concurrency-limit", 0, "Hard Limit of concurrent requests to be processed by a single replica.")
+	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.markFlagMakesRevision("concurrency-target")
+	command.Flags().IntVar(&p.ConcurrencyLimit, "concurrency-limit", 0,
+		"Hard Limit of concurrent requests to be processed by a single replica.")
+	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.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")
 }
 
+// AddUpdateFlags adds the flags specific to update.
+func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) {
+	p.addSharedFlags(command)
+}
+
+// AddCreateFlags adds the flags specific to create
 func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) {
-	p.AddUpdateFlags(command)
+	p.addSharedFlags(command)
-	command.Flags().BoolVar(&p.ForceCreate, "force", false, "Create service forcefully, replaces existing service if any.")
+	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 {
+// Apply mutates the given service according to the flags in the command.
+func (p *ConfigurationEditFlags) Apply(
+	service *servingv1alpha1.Service,
+	cmd *cobra.Command) error {
 
 	template, err := servinglib.RevisionTemplateOfService(service)
 	if err != nil {
@@ -96,6 +143,20 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co
 		}
 	}
 
+	name, err := servinglib.GenerateRevisionName(p.RevisionName, service)
+	if err != nil {
+		return err
+	}
+
+	if p.AnyMutation(cmd) {
+		err = servinglib.UpdateName(template, name)
+		if err == servinglib.ApiTooOldError && !cmd.Flags().Changed("revision-name") {
+			// Ignore the error if we don't support revision names and nobody
+			// explicitly asked for one.
+		} else if err != nil {
+			return err
+		}
+	}
 	if cmd.Flags().Changed("image") {
 		err = servinglib.UpdateImage(template, p.Image)
 		if err != nil {
@@ -194,3 +255,14 @@ func (p *ConfigurationEditFlags) computeResources(resourceFlags ResourceFlags) (
 
 	return resourceList, nil
 }
+
+// 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 {
+		if cmd.Flags().Changed(flag) {
+			return true
+		}
+	}
+	return false
+}

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"
@@ -166,6 +167,98 @@ func TestServiceUpdateImage(t *testing.T) {
 	}
 }
 
+func TestServiceUpdateRevisionNameExplicit(t *testing.T) {
+	orig := newEmptyServiceBetaAPIStyle()
+
+	template, err := servinglib.RevisionTemplateOfService(orig)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	template.Name = "foo-asdf"
+
+	// 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 := newEmptyServiceBetaAPIStyle()
+
+	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 := newEmptyServiceBetaAPIStyle()
+
+	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", "--revision-name=", "--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, cmp.Equal(template.Name, ""))
+}
+
+func TestServiceUpdateRevisionNameNoMutationNoChange(t *testing.T) {
+	orig := newEmptyServiceBetaAPIStyle()
+
+	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()
 
@@ -456,6 +549,23 @@ func newEmptyService() *v1alpha1.Service {
 	}
 }
 
+func newEmptyServiceBetaAPIStyle() *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{

pkg/serving/config_changes.go

@@ -16,6 +16,7 @@ package serving
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strconv"
 
@@ -92,6 +93,17 @@ 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
+}
+
 // 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) {

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

pkg/serving/service.go

@@ -15,7 +15,11 @@
 package serving
 
 import (
+	"bytes"
 	"errors"
+	"math/rand"
+	"strings"
+	"text/template"
 
 	servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1"
 )
@@ -38,6 +42,52 @@ 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",
+}
+
+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(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,
+	}
+	buf := new(bytes.Buffer)
+	err = templ.Execute(buf, context)
+	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) {
 	if service.Spec.DeprecatedRunLatest != nil {
 		return &service.Spec.DeprecatedRunLatest.Configuration, nil

pkg/serving/service_test.go

@@ -0,0 +1,83 @@
+// 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], ""},
+		{"", "", ""},
+		{"andrew", "foo-andrew", ""},
+		{"{{.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)
+		}
+	}
+}
+
+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")
+}