From 42c4076c279e71d2673f3625b126b74aafd343f4 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Thu, 16 Jul 2020 21:27:20 +0200 Subject: [PATCH 1/8] feat: Add support to combine service create --filename with other options --- docs/cmd/kn_service_create.md | 2 +- .../service/configuration_edit_flags.go | 5 +- pkg/kn/commands/service/create.go | 5 +- pkg/kn/commands/service/create_test.go | 62 +++++++++++++++++++ 4 files changed, 69 insertions(+), 5 deletions(-) diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index e358c7c033..7ad289d86e 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -64,7 +64,7 @@ kn service create NAME --image IMAGE --concurrency-utilization int Percentage of concurrent requests utilization before scaling up. (default 70) -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-). --env-from stringArray Add environment variables from a ConfigMap (prefix cm: or config-map:) or a Secret (prefix secret:). Example: --env-from cm:myconfigmap or --env-from secret:mysecret. You can use this flag multiple times. To unset a ConfigMap/Secret reference, append "-" to the name, e.g. --env-from cm:myconfigmap-. - -f, --filename string Create a service from file. + -f, --filename string Create a service from file. The created service can be futher modified by combining with other options.Example: -f /path/to/file --env NAME=value will also add environment variable. --force Create service forcefully, replaces existing service if any. -h, --help help for create --image string Image to run. diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 6321e9a8ad..5e42419d67 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -278,8 +278,11 @@ func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) { p.addSharedFlags(command) command.Flags().BoolVar(&p.ForceCreate, "force", false, "Create service forcefully, replaces existing service if any.") - command.Flags().StringVarP(&p.Filename, "filename", "f", "", "Create a service from file.") + command.Flags().StringVarP(&p.Filename, "filename", "f", "", "Create a service from file. "+ + "The created service can be futher modified by combining with other options."+ + "Example: -f /path/to/file --env NAME=value will also add environment variable.") command.MarkFlagFilename("filename") + p.markFlagMakesRevision("filename") } // Apply mutates the given service according to the flags in the command. diff --git a/pkg/kn/commands/service/create.go b/pkg/kn/commands/service/create.go index a2095d8c13..a8a93db3f8 100644 --- a/pkg/kn/commands/service/create.go +++ b/pkg/kn/commands/service/create.go @@ -303,12 +303,11 @@ func constructServiceFromFile(cmd *cobra.Command, editFlags ConfigurationEditFla // Set namespace in case it's specified as --namespace service.ObjectMeta.Namespace = namespace - // We need to generate revision to have --force replace working - revName, err := servinglib.GenerateRevisionName(editFlags.RevisionName, &service) + // Apply options provided from cmdline + err = editFlags.Apply(&service, nil, cmd) if err != nil { return nil, err } - service.Spec.Template.Name = revName return &service, nil } diff --git a/pkg/kn/commands/service/create_test.go b/pkg/kn/commands/service/create_test.go index 575d38b0c2..871860e1af 100644 --- a/pkg/kn/commands/service/create_test.go +++ b/pkg/kn/commands/service/create_test.go @@ -973,3 +973,65 @@ func TestServiceCreateInvalidDataYAML(t *testing.T) { _, _, _, err = fakeServiceCreate([]string{"service", "create", "foo", "--filename", tempFile}, false) assert.Assert(t, util.ContainsAll(err.Error(), "found", "tab", "violates", "indentation")) } + +func TestServiceCreateFromYAMLWithOverride(t *testing.T) { + tempDir, err := ioutil.TempDir("", "kn-file") + defer os.RemoveAll(tempDir) + assert.NilError(t, err) + + tempFile := filepath.Join(tempDir, "service.yaml") + err = ioutil.WriteFile(tempFile, []byte(serviceYAML), os.FileMode(0666)) + assert.NilError(t, err) + // Merge env vars + expectedEnvVars := map[string]string{ + "TARGET": "Go Sample v1", + "FOO": "BAR"} + action, created, _, err := fakeServiceCreate([]string{ + "service", "create", "foo", "--filename", tempFile, "--env", "FOO=BAR"}, false) + assert.NilError(t, err) + assert.Assert(t, action.Matches("create", "services")) + assert.Equal(t, created.Name, "foo") + + actualEnvVar, err := servinglib.EnvToMap(created.Spec.Template.Spec.GetContainer().Env) + assert.NilError(t, err) + assert.DeepEqual(t, actualEnvVar, expectedEnvVars) + + // Override env vars + expectedEnvVars = map[string]string{ + "TARGET": "FOOBAR", + "FOO": "BAR"} + action, created, _, err = fakeServiceCreate([]string{ + "service", "create", "foo", "--filename", tempFile, "--env", "TARGET=FOOBAR", "--env", "FOO=BAR"}, false) + assert.NilError(t, err) + assert.Assert(t, action.Matches("create", "services")) + assert.Equal(t, created.Name, "foo") + + actualEnvVar, err = servinglib.EnvToMap(created.Spec.Template.Spec.GetContainer().Env) + assert.NilError(t, err) + assert.DeepEqual(t, actualEnvVar, expectedEnvVars) + + // Remove existing env vars + expectedEnvVars = map[string]string{ + "FOO": "BAR"} + action, created, _, err = fakeServiceCreate([]string{ + "service", "create", "foo", "--filename", tempFile, "--env", "TARGET-", "--env", "FOO=BAR"}, false) + assert.NilError(t, err) + assert.Assert(t, action.Matches("create", "services")) + assert.Equal(t, created.Name, "foo") + + actualEnvVar, err = servinglib.EnvToMap(created.Spec.Template.Spec.GetContainer().Env) + assert.NilError(t, err) + assert.DeepEqual(t, actualEnvVar, expectedEnvVars) + + // Multiple edit flags + expectedAnnotations := map[string]string{ + "foo": "bar"} + action, created, _, err = fakeServiceCreate([]string{"service", "create", "foo", "--filename", tempFile, + "--service-account", "foo", "--cmd", "/foo/bar", "-a", "foo=bar"}, false) + assert.NilError(t, err) + assert.Assert(t, action.Matches("create", "services")) + assert.Equal(t, created.Name, "foo") + assert.DeepEqual(t, created.Spec.Template.Spec.GetContainer().Command, []string{"/foo/bar"}) + assert.Equal(t, created.Spec.Template.Spec.ServiceAccountName, "foo") + assert.DeepEqual(t, created.ObjectMeta.Annotations, expectedAnnotations) +} From ba133ed43c5a953dc30df423561b570ca70d848c Mon Sep 17 00:00:00 2001 From: David Simansky Date: Thu, 16 Jul 2020 21:38:37 +0200 Subject: [PATCH 2/8] fix: Fix spelling in usage message --- docs/cmd/kn_service_create.md | 2 +- pkg/kn/commands/service/configuration_edit_flags.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index 7ad289d86e..a7ec5bbd1f 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -64,7 +64,7 @@ kn service create NAME --image IMAGE --concurrency-utilization int Percentage of concurrent requests utilization before scaling up. (default 70) -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-). --env-from stringArray Add environment variables from a ConfigMap (prefix cm: or config-map:) or a Secret (prefix secret:). Example: --env-from cm:myconfigmap or --env-from secret:mysecret. You can use this flag multiple times. To unset a ConfigMap/Secret reference, append "-" to the name, e.g. --env-from cm:myconfigmap-. - -f, --filename string Create a service from file. The created service can be futher modified by combining with other options.Example: -f /path/to/file --env NAME=value will also add environment variable. + -f, --filename string Create a service from file. The created service can be further modified by combining with other options.Example: -f /path/to/file --env NAME=value will also add environment variable. --force Create service forcefully, replaces existing service if any. -h, --help help for create --image string Image to run. diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 5e42419d67..f6296902d0 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -279,7 +279,7 @@ func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) { command.Flags().BoolVar(&p.ForceCreate, "force", false, "Create service forcefully, replaces existing service if any.") command.Flags().StringVarP(&p.Filename, "filename", "f", "", "Create a service from file. "+ - "The created service can be futher modified by combining with other options."+ + "The created service can be further modified by combining with other options."+ "Example: -f /path/to/file --env NAME=value will also add environment variable.") command.MarkFlagFilename("filename") p.markFlagMakesRevision("filename") From 2c7338d5bc34a05450fb37197b47f623629ba5c3 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Fri, 17 Jul 2020 09:39:39 +0200 Subject: [PATCH 3/8] fix: Fix usage message --- docs/cmd/kn_service_create.md | 2 +- pkg/kn/commands/service/configuration_edit_flags.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index a7ec5bbd1f..de326a76e6 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -64,7 +64,7 @@ kn service create NAME --image IMAGE --concurrency-utilization int Percentage of concurrent requests utilization before scaling up. (default 70) -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-). --env-from stringArray Add environment variables from a ConfigMap (prefix cm: or config-map:) or a Secret (prefix secret:). Example: --env-from cm:myconfigmap or --env-from secret:mysecret. You can use this flag multiple times. To unset a ConfigMap/Secret reference, append "-" to the name, e.g. --env-from cm:myconfigmap-. - -f, --filename string Create a service from file. The created service can be further modified by combining with other options.Example: -f /path/to/file --env NAME=value will also add environment variable. + -f, --filename string Create a service from file. The created service can be further modified by combining with other options. For example -f /path/to/file --env NAME=value will also add environment variable. --force Create service forcefully, replaces existing service if any. -h, --help help for create --image string Image to run. diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index f6296902d0..91e58f9729 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -279,8 +279,8 @@ func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) { command.Flags().BoolVar(&p.ForceCreate, "force", false, "Create service forcefully, replaces existing service if any.") command.Flags().StringVarP(&p.Filename, "filename", "f", "", "Create a service from file. "+ - "The created service can be further modified by combining with other options."+ - "Example: -f /path/to/file --env NAME=value will also add environment variable.") + "The created service can be further modified by combining with other options. "+ + "For example -f /path/to/file --env NAME=value will also add environment variable.") command.MarkFlagFilename("filename") p.markFlagMakesRevision("filename") } From 857c0460b3dbabe440841a133d683ee9d82b936a Mon Sep 17 00:00:00 2001 From: David Simansky Date: Fri, 17 Jul 2020 10:49:31 +0200 Subject: [PATCH 4/8] fix: Add test to cover errors --- pkg/kn/commands/service/create_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/pkg/kn/commands/service/create_test.go b/pkg/kn/commands/service/create_test.go index 871860e1af..12746bca80 100644 --- a/pkg/kn/commands/service/create_test.go +++ b/pkg/kn/commands/service/create_test.go @@ -1035,3 +1035,23 @@ func TestServiceCreateFromYAMLWithOverride(t *testing.T) { assert.Equal(t, created.Spec.Template.Spec.ServiceAccountName, "foo") assert.DeepEqual(t, created.ObjectMeta.Annotations, expectedAnnotations) } + +func TestServiceCreateFromYAMLWithOverrideError(t *testing.T) { + tempDir, err := ioutil.TempDir("", "kn-file") + defer os.RemoveAll(tempDir) + assert.NilError(t, err) + + tempFile := filepath.Join(tempDir, "service.yaml") + err = ioutil.WriteFile(tempFile, []byte(serviceYAML), os.FileMode(0666)) + assert.NilError(t, err) + + _, _, _, err = fakeServiceCreate([]string{ + "service", "create", "foo", "--filename", tempFile, "--image", "foo/bar", "--image", "bar/foo"}, false) + assert.Assert(t, err != nil) + assert.Assert(t, util.ContainsAll(err.Error(), "\"--image\"", "invalid", "argument", "only", "once")) + + _, _, _, err = fakeServiceCreate([]string{ + "service", "create", "foo", "--filename", tempFile, "--scale", "-1"}, false) + assert.Assert(t, err != nil) + assert.Assert(t, util.ContainsAll(err.Error(), "expected", "0", "<=", "2147483647", "autoscaling.knative.dev/maxScale")) +} From 1f8bf11b061cd393f3ddb7fdf07fc951f8f6cf0b Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 21 Jul 2020 12:06:25 +0200 Subject: [PATCH 5/8] fix: Usage message wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Roland Huß --- 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 91e58f9729..1cffcaf313 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -280,7 +280,7 @@ func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) { "Create service forcefully, replaces existing service if any.") command.Flags().StringVarP(&p.Filename, "filename", "f", "", "Create a service from file. "+ "The created service can be further modified by combining with other options. "+ - "For example -f /path/to/file --env NAME=value will also add environment variable.") + "For example, -f /path/to/file --env NAME=value adds also an environment variable.") command.MarkFlagFilename("filename") p.markFlagMakesRevision("filename") } From d76201c46c6b9f8fefb6314ac8606182c7276ad4 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 21 Jul 2020 12:07:45 +0200 Subject: [PATCH 6/8] fix: Update codegen --- docs/cmd/kn_service_create.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index de326a76e6..b07a341095 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -64,7 +64,7 @@ kn service create NAME --image IMAGE --concurrency-utilization int Percentage of concurrent requests utilization before scaling up. (default 70) -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-). --env-from stringArray Add environment variables from a ConfigMap (prefix cm: or config-map:) or a Secret (prefix secret:). Example: --env-from cm:myconfigmap or --env-from secret:mysecret. You can use this flag multiple times. To unset a ConfigMap/Secret reference, append "-" to the name, e.g. --env-from cm:myconfigmap-. - -f, --filename string Create a service from file. The created service can be further modified by combining with other options. For example -f /path/to/file --env NAME=value will also add environment variable. + -f, --filename string Create a service from file. The created service can be further modified by combining with other options. For example, -f /path/to/file --env NAME=value adds also an environment variable. --force Create service forcefully, replaces existing service if any. -h, --help help for create --image string Image to run. From 3e0a752493483725f7cfc78e96075ec06764fa44 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 21 Jul 2020 14:30:58 +0200 Subject: [PATCH 7/8] chore: Add changelog entry --- CHANGELOG.adoc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 0ec5a95d3b..b023bea963 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -12,6 +12,18 @@ | https://github.com/knative/client/pull/[#] //// +## Unreleased + +[cols="1,10,3", options="header", width="100%"] +|=== +| | Description | PR + +| 🎁 +| Add support to combine service create --filename with other options +| https://github.com/knative/client/pull/937[#937] + +|=== + ## v0.16.0 (2020-07-14) [cols="1,10,3", options="header", width="100%"] From 062345dc23dd608e2823d95ab434ae65985492e7 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 21 Jul 2020 14:32:00 +0200 Subject: [PATCH 8/8] fix: Fix changelog formatting --- CHANGELOG.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index b023bea963..4fbaeb8ea4 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -19,7 +19,7 @@ | | Description | PR | 🎁 -| Add support to combine service create --filename with other options +| Add support to combine service create --filename with other options | https://github.com/knative/client/pull/937[#937] |===