From a3b4eeab05593208ae5f9582c8f1c2dd48df7dde Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 11 Feb 2020 08:28:50 +0100 Subject: [PATCH 1/6] Fix --image flag to only allow single occurence --- CHANGELOG.adoc | 4 +++ .../service/configuration_edit_flags.go | 25 ++++++++++++++++--- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 1d9d0a44dd..99505e2126 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -45,6 +45,10 @@ | remove unecessary `sync` parameter in setup call | https://github.com/knative/client/pull/656[#656] +| 🐛 +| Fix `--image` flag to only allow single occurence +| https://github.com/knative/client/pull/646[#646] + ## v0.12.0 (2020-01-29) [cols="1,10,3", options="header", width="100%"] diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 25753a8551..b26cf78912 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -30,7 +30,7 @@ import ( type ConfigurationEditFlags struct { // Direct field manipulation - Image string + Image singletonString Env []string EnvFrom []string Mount []string @@ -67,6 +67,25 @@ type ResourceFlags struct { Memory string } +// -- singletonString Value +// Custom implementation of flag.Value interface to prevent multiple value assignment. +// Useful to enforce single use of flag, e.g. --image. +type singletonString string + +func (s *singletonString) Set(val string) error { + if len(*s) > 0 { + return errors.New("value is already set") + } + *s = singletonString(val) + return nil +} + +func (s *singletonString) Type() string { + return "string" +} + +func (s *singletonString) String() string { return string(*s) } + // markFlagMakesRevision indicates that a flag will create a new revision if you // set it. func (p *ConfigurationEditFlags) markFlagMakesRevision(f string) { @@ -75,7 +94,7 @@ func (p *ConfigurationEditFlags) markFlagMakesRevision(f string) { // addSharedFlags adds the flags common between create & update. func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) { - command.Flags().StringVar(&p.Image, "image", "", "Image to run.") + command.Flags().VarP(&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 "+ @@ -251,7 +270,7 @@ func (p *ConfigurationEditFlags) Apply( } imageSet := false if cmd.Flags().Changed("image") { - err = servinglib.UpdateImage(template, p.Image) + err = servinglib.UpdateImage(template, p.Image.String()) if err != nil { return err } From 71e12f9b968aef9fa3e58e76327f7908aba4328c Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 11 Feb 2020 08:47:34 +0100 Subject: [PATCH 2/6] Fix changelog link --- CHANGELOG.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 99505e2126..fd138675d5 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -47,7 +47,7 @@ | 🐛 | Fix `--image` flag to only allow single occurence -| https://github.com/knative/client/pull/646[#646] +| https://github.com/knative/client/pull/647[#647] ## v0.12.0 (2020-01-29) From d90fbaf93cb0f520ddef16ce4b0708088dd20f0d Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 11 Feb 2020 12:02:24 +0100 Subject: [PATCH 3/6] Reflect feedback in error message and type name --- .../service/configuration_edit_flags.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index b26cf78912..8731eec008 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -30,7 +30,7 @@ import ( type ConfigurationEditFlags struct { // Direct field manipulation - Image singletonString + Image uniqueStringArg Env []string EnvFrom []string Mount []string @@ -67,24 +67,24 @@ type ResourceFlags struct { Memory string } -// -- singletonString Value +// -- uniqueStringArg Value // Custom implementation of flag.Value interface to prevent multiple value assignment. -// Useful to enforce single use of flag, e.g. --image. -type singletonString string +// Useful to enforce unique use of flags, e.g. --image. +type uniqueStringArg string -func (s *singletonString) Set(val string) error { +func (s *uniqueStringArg) Set(val string) error { if len(*s) > 0 { - return errors.New("value is already set") + return errors.New("can be provided only once") } - *s = singletonString(val) + *s = uniqueStringArg(val) return nil } -func (s *singletonString) Type() string { +func (s *uniqueStringArg) Type() string { return "string" } -func (s *singletonString) String() string { return string(*s) } +func (s *uniqueStringArg) String() string { return string(*s) } // markFlagMakesRevision indicates that a flag will create a new revision if you // set it. From d4756e75dcb7fbd2c89f8d661ae6144c30ad79a4 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 11 Feb 2020 12:03:23 +0100 Subject: [PATCH 4/6] Add unit tests --- pkg/kn/commands/service/create_test.go | 6 ++++++ pkg/kn/commands/service/update_test.go | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/pkg/kn/commands/service/create_test.go b/pkg/kn/commands/service/create_test.go index 47f96a5d09..94efeb20b9 100644 --- a/pkg/kn/commands/service/create_test.go +++ b/pkg/kn/commands/service/create_test.go @@ -135,6 +135,12 @@ func TestServiceCreateImage(t *testing.T) { } } +func TestServiceCreateWithMultipleImages(t *testing.T) { + _, _, _, err := fakeServiceCreate([]string{ + "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--image", "gcr.io/bar/foo:baz", "--no-wait"}, false, false) + assert.Error(t, err, "invalid argument \"gcr.io/bar/foo:baz\" for \"--image\" flag: can be provided only once") +} + func TestServiceCreateImageSync(t *testing.T) { action, created, output, err := fakeServiceCreate([]string{ "service", "create", "foo", "--image", "gcr.io/foo/bar:baz"}, false) diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go index 868dc271d6..9a47e3067f 100644 --- a/pkg/kn/commands/service/update_test.go +++ b/pkg/kn/commands/service/update_test.go @@ -191,6 +191,13 @@ func TestServiceUpdateImage(t *testing.T) { } } +func TestServiceUpdateWithMultipleImages(t *testing.T) { + orig := newEmptyService() + _, _, _, err := fakeServiceUpdate(orig, []string{ + "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--image", "gcr.io/bar/foo:baz", "--no-wait"}, false) + assert.Error(t, err, "invalid argument \"gcr.io/bar/foo:baz\" for \"--image\" flag: can be provided only once") +} + func TestServiceUpdateCommand(t *testing.T) { orig := newEmptyService() From 84d1d9f5d9e872e767ce9477f59943fb612ee0d4 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 11 Feb 2020 14:46:20 +0100 Subject: [PATCH 5/6] Reflect unit tests improvement from feedback --- pkg/kn/commands/service/create_test.go | 4 +++- pkg/kn/commands/service/update_test.go | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/kn/commands/service/create_test.go b/pkg/kn/commands/service/create_test.go index 94efeb20b9..3689de23c9 100644 --- a/pkg/kn/commands/service/create_test.go +++ b/pkg/kn/commands/service/create_test.go @@ -29,6 +29,7 @@ import ( "knative.dev/client/pkg/kn/commands" servinglib "knative.dev/client/pkg/serving" + "knative.dev/client/pkg/util" "knative.dev/client/pkg/wait" corev1 "k8s.io/api/core/v1" @@ -138,7 +139,8 @@ func TestServiceCreateImage(t *testing.T) { func TestServiceCreateWithMultipleImages(t *testing.T) { _, _, _, err := fakeServiceCreate([]string{ "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--image", "gcr.io/bar/foo:baz", "--no-wait"}, false, false) - assert.Error(t, err, "invalid argument \"gcr.io/bar/foo:baz\" for \"--image\" flag: can be provided only once") + + assert.Assert(t, util.ContainsAll(err.Error(), "\"--image\"", "\"gcr.io/bar/foo:baz\"", "flag", "once")) } func TestServiceCreateImageSync(t *testing.T) { diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go index 9a47e3067f..5f783e6f27 100644 --- a/pkg/kn/commands/service/update_test.go +++ b/pkg/kn/commands/service/update_test.go @@ -195,7 +195,8 @@ func TestServiceUpdateWithMultipleImages(t *testing.T) { orig := newEmptyService() _, _, _, err := fakeServiceUpdate(orig, []string{ "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--image", "gcr.io/bar/foo:baz", "--no-wait"}, false) - assert.Error(t, err, "invalid argument \"gcr.io/bar/foo:baz\" for \"--image\" flag: can be provided only once") + + assert.Assert(t, util.ContainsAll(err.Error(), "\"--image\"", "\"gcr.io/bar/foo:baz\"", "flag", "once")) } func TestServiceUpdateCommand(t *testing.T) { From 29fb37c9b65c6e9187f504f0f912fabd98f509b7 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Wed, 12 Feb 2020 08:39:06 +0100 Subject: [PATCH 6/6] Fix unit tests due to changes in master --- pkg/kn/commands/service/create_test.go | 2 +- pkg/kn/commands/service/update_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/kn/commands/service/create_test.go b/pkg/kn/commands/service/create_test.go index 3689de23c9..6eb522acd9 100644 --- a/pkg/kn/commands/service/create_test.go +++ b/pkg/kn/commands/service/create_test.go @@ -138,7 +138,7 @@ func TestServiceCreateImage(t *testing.T) { func TestServiceCreateWithMultipleImages(t *testing.T) { _, _, _, err := fakeServiceCreate([]string{ - "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--image", "gcr.io/bar/foo:baz", "--no-wait"}, false, false) + "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--image", "gcr.io/bar/foo:baz", "--no-wait"}, false) assert.Assert(t, util.ContainsAll(err.Error(), "\"--image\"", "\"gcr.io/bar/foo:baz\"", "flag", "once")) } diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go index 5f783e6f27..a04fd0e9ab 100644 --- a/pkg/kn/commands/service/update_test.go +++ b/pkg/kn/commands/service/update_test.go @@ -194,7 +194,7 @@ func TestServiceUpdateImage(t *testing.T) { func TestServiceUpdateWithMultipleImages(t *testing.T) { orig := newEmptyService() _, _, _, err := fakeServiceUpdate(orig, []string{ - "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--image", "gcr.io/bar/foo:baz", "--no-wait"}, false) + "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--image", "gcr.io/bar/foo:baz", "--no-wait"}) assert.Assert(t, util.ContainsAll(err.Error(), "\"--image\"", "\"gcr.io/bar/foo:baz\"", "flag", "once")) }