Repository: knative/client (go)

Issue:
Add support to combine service create --filename with other options
<!-- If you need to report a security issue with Knative, send an email to knative-security@googlegroups.com. -->

### Feature request

Per PR #913 review [discussion](https://github.com/knative/client/pull/913#discussion_r451481222) and [comment](https://github.com/knative/client/pull/913#discussion_r450685124). There's a possibility to extend the initial functionality of `kn service create --filename` with other options. In a nutshell to view provided file as a baseline that can be further modified but other option flags.
There should be a reasonable amount of tests added to verify common usage pattern works, e.g. command line options are reflected, take priority and can override existing values, lists are merged etc.

### Use case

Create a service from file and add env variable:
```
kn service create mysvc --filename mysvc.yaml --env VERSION="v2"
```


Create a service from file and specify service account:
```
kn service create mysvc --filename mysvc.yaml --service-account specialSA
```

#### Implementation notes

The following code block should be added and suggestion points from [this PR](https://github.com/knative/client/pull/913#discussion_r451481222) reflected.

```go
err = editFlags.Apply(&service, nil, cmd)
if err != nil {
    return nil, err
}
```

<!--
Optional Classifications, remove the '> ' to select:
/kind good-first-issue
> /kind proposal
> /kind doc
> /kind cleanup
-->


Pull request description:
feat: Add support to combine service create --filename with other options
## Description

Add enhancement to the command `kn service create -f` to accept other options to modify the created service.

Per discussion in #923 we should decide how `name` is handled:
- Provided cmdline `name` overrides the name in the file
- Keep the current implementation that if `name` is provided on cmdline it must match the name in the file.


## Changes

*  Add support to combine service create --filename with other options

## Reference

<!-- Please add the corresponding issue number which this pull request is about to fix -->
Fixes #923 

<!--
Please add an entrty to CHANGELOG.adoc file, too, as part of your Pull Request.

In the following cases, add a short description of PR to the unreleased section in CHANGELOG.adoc:

- 🎁 New feature
- 🐛 Bug fix
- ✨ Feature Update
- 🐣 Refactoring
- 🗑️ Remove feature or internal logic

See other entries in CHANGELOG.adoc as an example for how to add the entry, including a reference to the corresponding issue/PR

PLEASE DON'T ADD THAT LINE HERE IN THE PULL-REQUEST DESCRIPTION BUT DIRECTLY IN CHANGELOG.ADOC AND ADD CHANGELOG.ADOC AS PART OF YOUR PULL-REQUEST.
-->

<!--
To automatically lint go code in this pull request uncomment the line below. You get feedback as comments on your pull request then -->

<!--
/lint
-->


Patch:
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
index 0ec5a95d..4fbaeb8e 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%"]
diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md
index e358c7c0..b07a3410 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 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.
diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go
index 6321e9a8..1cffcaf3 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 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.
diff --git a/pkg/kn/commands/service/create.go b/pkg/kn/commands/service/create.go
index a2095d8c..a8a93db3 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
 }


Test patch:
diff --git a/pkg/kn/commands/service/create_test.go b/pkg/kn/commands/service/create_test.go
index 575d38b0..12746bca 100644
--- a/pkg/kn/commands/service/create_test.go
+++ b/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"))
+}