feat: Add support to combine service create --filename with other options (#937)

* feat: Add support to combine service create --filename with other options

* fix: Fix spelling in usage message

* fix: Fix usage message

* fix: Add test to cover errors

* fix: Usage message wording

Co-authored-by: Roland Huß <rhuss@redhat.com>

* fix: Update codegen

* chore: Add changelog entry

* fix: Fix changelog formatting

Co-authored-by: Roland Huß <rhuss@redhat.com>

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%"]

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 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.

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 further modified by combining with other options. "+
+		"For example, -f /path/to/file --env NAME=value adds also an environment variable.")
 	command.MarkFlagFilename("filename")
+	p.markFlagMakesRevision("filename")
 }
 
 // Apply mutates the given service according to the flags in the command.

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
+	// Apply options provided from cmdline
-	revName, err := servinglib.GenerateRevisionName(editFlags.RevisionName, &service)
+	err = editFlags.Apply(&service, nil, cmd)
 	if err != nil {
 		return nil, err
 	}
-	service.Spec.Template.Name = revName
 
 	return &service, nil
 }

pkg/kn/commands/service/create_test.go

@@ -973,3 +973,85 @@ 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)
+}
+
+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"))
+}