Repository: knative/client (go)

Issue:
Help message for `kn service apply --env` is appropriate
<!-- If you need to report a security issue with Knative, send an email to knative-security@googlegroups.com. -->

### Bug report

`kn service apply --help` yields:

```
-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-).
```

This is not correct since you can't add or remove env vars with `apply` but just set the full set of env variables. 

Same possibly for `--labels` and `--annotations` (or any map-based option)

### Expected behavior

The help message for `--env` should be different for `kn service update` and `kn service apply` as the behaviour is different (`update` : incremental changes by adding/removing, `apply`: full state)

The same is true for `kn service create` where removing env vars also doesn't make any sense.

Thanks to @pmbanugo for reporting this issue.


Pull request:
Fix env, annotation and labels flags in service create/update/apply
## Description/Changes

<!-- Please add a short summary about what the pull request is going to bring on the table -->
Update descriptions to the following flags in `service create/update/apply`:
* env
* labels
* annotations

If you can not unset the label/annotation/env then don't mention it in the description of the flag.

## Reference

<!-- Please add the corresponding issue number which this pull request is about to fix -->
Fixes #1449 

<!--
Please add an entry 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/docs/cmd/kn_service_apply.md b/docs/cmd/kn_service_apply.md
index 064ffaa4..319bb203 100644
--- a/docs/cmd/kn_service_apply.md
+++ b/docs/cmd/kn_service_apply.md
@@ -29,7 +29,7 @@ kn service apply s0 --filename my-svc.yml
 ### Options
 
 ```
-  -a, --annotation stringArray            Annotations to set for both Service and Revision. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-).
+  -a, --annotation stringArray            Annotations to set for both Service and Revision. name=value; you may provide this flag any number of times to set multiple annotations.
       --annotation-revision stringArray   Revision annotation to set. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-). This flag takes precedence over the "annotation" flag.
       --annotation-service stringArray    Service annotation to set. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-). This flag takes precedence over the "annotation" flag.
       --arg stringArray                   Add argument to the container command. Example: --arg myArg1 --arg --myArg2 --arg myArg3=3. You can use this flag multiple times.
@@ -37,15 +37,15 @@ kn service apply s0 --filename my-svc.yml
       --cmd stringArray                   Specify command to be used as entrypoint instead of default one. Example: --cmd /app/start or --cmd sh --cmd /app/start.sh or --cmd /app/start --arg myArg to pass additional arguments.
       --concurrency-limit int             Hard Limit of concurrent requests to be processed by a single replica.
       --containers string                 Specify path to file including definition for additional containers, alternatively use '-' to read from stdin. Example: --containers ./containers.yaml or --containers -.
-  -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-).
+  -e, --env stringArray                   Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables.
       --env-file string                   Path to a file containing environment variables (e.g. --env-file=/home/knative/service1/env).
-      --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-.
-      --env-value-from stringArray        Add environment variable from a value of key in ConfigMap (prefix cm: or config-map:) or a Secret (prefix sc: or secret:). Example: --env-value-from NAME=cm:myconfigmap:key or --env-value-from NAME=secret:mysecret:key. You can use this flag multiple times. To unset a value from a ConfigMap/Secret key reference, append "-" to the key, e.g. --env-value-from ENV-.
+      --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.
+      --env-value-from stringArray        Add environment variable from a value of key in ConfigMap (prefix cm: or config-map:) or a Secret (prefix sc: or secret:). Example: --env-value-from NAME=cm:myconfigmap:key or --env-value-from NAME=secret:mysecret:key. You can use this flag multiple times.
   -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 apply
       --image string                      Image to run.
-  -l, --label stringArray                 Labels to set for both Service and Revision. 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-).
+  -l, --label stringArray                 Labels to set for both Service and Revision. name=value; you may provide this flag any number of times to set multiple labels.
       --label-revision stringArray        Revision 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-). This flag takes precedence over the "label" flag.
       --label-service stringArray         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-). This flag takes precedence over the "label" flag.
       --limit strings                     The resource requirement limits for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource limit, append "-" to the resource name, e.g. '--limit memory-'.
diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md
index f2cfdf59..1a414817 100644
--- a/docs/cmd/kn_service_create.md
+++ b/docs/cmd/kn_service_create.md
@@ -54,7 +54,7 @@ kn service create NAME --image IMAGE
 ### Options
 
 ```
-  -a, --annotation stringArray            Annotations to set for both Service and Revision. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-).
+  -a, --annotation stringArray            Annotations to set for both Service and Revision. name=value; you may provide this flag any number of times to set multiple annotations.
       --annotation-revision stringArray   Revision annotation to set. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-). This flag takes precedence over the "annotation" flag.
       --annotation-service stringArray    Service annotation to set. name=value; you may provide this flag any number of times to set multiple annotations. To unset, specify the annotation name followed by a "-" (e.g., name-). This flag takes precedence over the "annotation" flag.
       --arg stringArray                   Add argument to the container command. Example: --arg myArg1 --arg --myArg2 --arg myArg3=3. You can use this flag multiple times.
@@ -62,15 +62,15 @@ kn service create NAME --image IMAGE
       --cmd stringArray                   Specify command to be used as entrypoint instead of default one. Example: --cmd /app/start or --cmd sh --cmd /app/start.sh or --cmd /app/start --arg myArg to pass additional arguments.
       --concurrency-limit int             Hard Limit of concurrent requests to be processed by a single replica.
       --containers string                 Specify path to file including definition for additional containers, alternatively use '-' to read from stdin. Example: --containers ./containers.yaml or --containers -.
-  -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-).
+  -e, --env stringArray                   Environment variable to set. NAME=value; you may provide this flag any number of times to set multiple environment variables.
       --env-file string                   Path to a file containing environment variables (e.g. --env-file=/home/knative/service1/env).
-      --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-.
-      --env-value-from stringArray        Add environment variable from a value of key in ConfigMap (prefix cm: or config-map:) or a Secret (prefix sc: or secret:). Example: --env-value-from NAME=cm:myconfigmap:key or --env-value-from NAME=secret:mysecret:key. You can use this flag multiple times. To unset a value from a ConfigMap/Secret key reference, append "-" to the key, e.g. --env-value-from ENV-.
+      --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.
+      --env-value-from stringArray        Add environment variable from a value of key in ConfigMap (prefix cm: or config-map:) or a Secret (prefix sc: or secret:). Example: --env-value-from NAME=cm:myconfigmap:key or --env-value-from NAME=secret:mysecret:key. You can use this flag multiple times.
   -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.
-  -l, --label stringArray                 Labels to set for both Service and Revision. 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-).
+  -l, --label stringArray                 Labels to set for both Service and Revision. name=value; you may provide this flag any number of times to set multiple labels.
       --label-revision stringArray        Revision 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-). This flag takes precedence over the "label" flag.
       --label-service stringArray         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-). This flag takes precedence over the "label" flag.
       --limit strings                     The resource requirement limits for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource limit, append "-" to the resource name, e.g. '--limit memory-'.
diff --git a/pkg/kn/commands/container/add.go b/pkg/kn/commands/container/add.go
index c8788112..06a05f5c 100644
--- a/pkg/kn/commands/container/add.go
+++ b/pkg/kn/commands/container/add.go
@@ -75,6 +75,7 @@ func NewContainerAddCommand(p *commands.KnParams) *cobra.Command {
 		},
 	}
 	podSpecFlags.AddFlags(cmd.Flags())
+	podSpecFlags.AddUpdateFlags(cmd.Flags())
 	// Volume is not part of ContainerSpec
 	cmd.Flag("volume").Hidden = true
 
diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go
index 5f0bf072..f66e9836 100644
--- a/pkg/kn/commands/service/configuration_edit_flags.go
+++ b/pkg/kn/commands/service/configuration_edit_flags.go
@@ -128,12 +128,6 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 		"Percentage of concurrent requests utilization before scaling up.")
 	p.markFlagMakesRevision("scale-utilization")
 
-	command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{},
-		"Labels to set for both Service and Revision. 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().StringArrayVarP(&p.LabelsService, "label-service", "", []string{},
 		"Service label to set. name=value; you may provide this flag "+
 			"any number of times to set multiple labels. "+
@@ -160,12 +154,6 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 			"the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)")
 	// Don't mark as changing the revision.
 
-	command.Flags().StringArrayVarP(&p.Annotations, "annotation", "a", []string{},
-		"Annotations to set for both Service and Revision. name=value; you may provide this flag "+
-			"any number of times to set multiple annotations. "+
-			"To unset, specify the annotation name followed by a \"-\" (e.g., name-).")
-	p.markFlagMakesRevision("annotation")
-
 	command.Flags().StringArrayVarP(&p.AnnotationsService, "annotation-service", "", []string{},
 		"Service annotation to set. name=value; you may provide this flag "+
 			"any number of times to set multiple annotations. "+
@@ -187,11 +175,33 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
 // AddUpdateFlags adds the flags specific to update.
 func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) {
 	p.addSharedFlags(command)
+
+	flagNames := p.PodSpecFlags.AddUpdateFlags(command.Flags())
+	for _, name := range flagNames {
+		p.markFlagMakesRevision(name)
+	}
+
+	command.Flags().StringArrayVarP(&p.Annotations, "annotation", "a", []string{},
+		"Annotations to set for both Service and Revision. name=value; you may provide this flag "+
+			"any number of times to set multiple annotations. "+
+			"To unset, specify the annotation name followed by a \"-\" (e.g., name-).")
+	p.markFlagMakesRevision("annotation")
+	command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{},
+		"Labels to set for both Service and Revision. 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")
 }
 
 // AddCreateFlags adds the flags specific to create
 func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) {
 	p.addSharedFlags(command)
+
+	flagNames := p.PodSpecFlags.AddCreateFlags(command.Flags())
+	for _, name := range flagNames {
+		p.markFlagMakesRevision(name)
+	}
+
 	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. "+
@@ -199,6 +209,14 @@ func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) {
 		"For example, -f /path/to/file --env NAME=value adds also an environment variable.")
 	command.MarkFlagFilename("filename")
 	p.markFlagMakesRevision("filename")
+	command.Flags().StringArrayVarP(&p.Annotations, "annotation", "a", []string{},
+		"Annotations to set for both Service and Revision. name=value; you may provide this flag "+
+			"any number of times to set multiple annotations.")
+	p.markFlagMakesRevision("annotation")
+	command.Flags().StringArrayVarP(&p.Labels, "label", "l", []string{},
+		"Labels to set for both Service and Revision. name=value; you may provide this flag "+
+			"any number of times to set multiple labels.")
+	p.markFlagMakesRevision("label")
 }
 
 // Apply mutates the given service according to the flags in the command.
diff --git a/pkg/kn/commands/source/container/create.go b/pkg/kn/commands/source/container/create.go
index 7e76e133..d7f81751 100644
--- a/pkg/kn/commands/source/container/create.go
+++ b/pkg/kn/commands/source/container/create.go
@@ -85,6 +85,7 @@ func NewContainerCreateCommand(p *commands.KnParams) *cobra.Command {
 	}
 	commands.AddNamespaceFlags(cmd.Flags(), false)
 	podFlags.AddFlags(cmd.Flags())
+	podFlags.AddUpdateFlags(cmd.Flags())
 	sinkFlags.Add(cmd)
 	cmd.MarkFlagRequired("image")
 	cmd.MarkFlagRequired("sink")
diff --git a/pkg/kn/commands/source/container/update.go b/pkg/kn/commands/source/container/update.go
index f82f3ecb..e0e1816c 100644
--- a/pkg/kn/commands/source/container/update.go
+++ b/pkg/kn/commands/source/container/update.go
@@ -94,6 +94,7 @@ func NewContainerUpdateCommand(p *commands.KnParams) *cobra.Command {
 	}
 	commands.AddNamespaceFlags(cmd.Flags(), false)
 	podFlags.AddFlags(cmd.Flags())
+	podFlags.AddUpdateFlags(cmd.Flags())
 	sinkFlags.Add(cmd)
 	return cmd
 }
diff --git a/pkg/kn/flags/podspec.go b/pkg/kn/flags/podspec.go
index 0dee6dc4..bc7b6f32 100644
--- a/pkg/kn/flags/podspec.go
+++ b/pkg/kn/flags/podspec.go
@@ -72,23 +72,15 @@ func (s *uniqueStringArg) Type() string {
 
 func (s *uniqueStringArg) String() string { return string(*s) }
 
-//AddFlags will add PodSpec related flags to FlagSet
-func (p *PodSpecFlags) AddFlags(flagset *pflag.FlagSet) []string {
-
+//AddUpdateFlags will add PodSpec flags related to environment variable to FlagSet of update command
+func (p *PodSpecFlags) AddUpdateFlags(flagset *pflag.FlagSet) []string {
 	flagNames := []string{}
-
-	flagset.VarP(&p.Image, "image", "", "Image to run.")
-	flagNames = append(flagNames, "image")
-
 	flagset.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-).")
 	flagNames = append(flagNames, "env")
 
-	flagset.StringVarP(&p.EnvFile, "env-file", "", "", "Path to a file containing environment variables (e.g. --env-file=/home/knative/service1/env).")
-	flagNames = append(flagNames, "env-file")
-
 	flagset.StringArrayVarP(&p.EnvValueFrom, "env-value-from", "", []string{},
 		"Add environment variable from a value of key in ConfigMap (prefix cm: or config-map:) or a Secret (prefix sc: or secret:). "+
 			"Example: --env-value-from NAME=cm:myconfigmap:key or --env-value-from NAME=secret:mysecret:key. "+
@@ -103,6 +95,43 @@ func (p *PodSpecFlags) AddFlags(flagset *pflag.FlagSet) []string {
 			"To unset a ConfigMap/Secret reference, append \"-\" to the name, e.g. --env-from cm:myconfigmap-.")
 	flagNames = append(flagNames, "env-from")
 
+	return flagNames
+}
+
+//AddCreateFlags will add PodSpec flags related to environment variable to FlagSet of create command
+func (p *PodSpecFlags) AddCreateFlags(flagset *pflag.FlagSet) []string {
+	flagNames := []string{}
+	flagset.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.")
+	flagNames = append(flagNames, "env")
+
+	flagset.StringArrayVarP(&p.EnvValueFrom, "env-value-from", "", []string{},
+		"Add environment variable from a value of key in ConfigMap (prefix cm: or config-map:) or a Secret (prefix sc: or secret:). "+
+			"Example: --env-value-from NAME=cm:myconfigmap:key or --env-value-from NAME=secret:mysecret:key. "+
+			"You can use this flag multiple times.")
+	flagNames = append(flagNames, "env-value-from")
+
+	flagset.StringArrayVarP(&p.EnvFrom, "env-from", "", []string{},
+		"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.")
+	flagNames = append(flagNames, "env-from")
+
+	return flagNames
+}
+
+//AddFlags will add PodSpec related flags to FlagSet
+func (p *PodSpecFlags) AddFlags(flagset *pflag.FlagSet) []string {
+
+	flagNames := []string{}
+
+	flagset.VarP(&p.Image, "image", "", "Image to run.")
+	flagNames = append(flagNames, "image")
+
+	flagset.StringVarP(&p.EnvFile, "env-file", "", "", "Path to a file containing environment variables (e.g. --env-file=/home/knative/service1/env).")
+	flagNames = append(flagNames, "env-file")
+
 	flagset.StringArrayVarP(&p.Mount, "mount", "", []string{},
 		"Mount a ConfigMap (prefix cm: or config-map:), a Secret (prefix secret: or sc:), or an existing Volume (without any prefix) on the specified directory. "+
 			"Example: --mount /mydir=cm:myconfigmap, --mount /mydir=secret:mysecret, or --mount /mydir=myvolume. "+


Test patch:
diff --git a/pkg/kn/flags/podspec_test.go b/pkg/kn/flags/podspec_test.go
index 42b1aea1..68a2ec2b 100644
--- a/pkg/kn/flags/podspec_test.go
+++ b/pkg/kn/flags/podspec_test.go
@@ -54,6 +54,7 @@ func TestPodSpecFlags(t *testing.T) {
 	}
 	testCmd.SetArgs(args)
 	flags.AddFlags(testCmd.Flags())
+	flags.AddCreateFlags(testCmd.Flags())
 	testCmd.Execute()
 }
 
@@ -130,6 +131,7 @@ func TestPodSpecResolve(t *testing.T) {
 	}
 	testCmd.SetArgs(inputArgs)
 	flags.AddFlags(testCmd.Flags())
+	flags.AddUpdateFlags(testCmd.Flags())
 	testCmd.Execute()
 }
 
@@ -251,6 +253,7 @@ containers:
 			}
 			testCmd.SetArgs(inputArgs)
 			flags.AddFlags(testCmd.Flags())
+			flags.AddUpdateFlags(testCmd.Flags())
 			testCmd.Execute()
 		})
 	}
@@ -282,6 +285,7 @@ func TestPodSpecResolveReturnError(t *testing.T) {
 
 	testCmd.SetArgs(inputArgs)
 	flags.AddFlags(testCmd.Flags())
+	flags.AddCreateFlags(testCmd.Flags())
 	testCmd.Execute()
 	out := outBuf.String()
 	assert.Assert(t, util.ContainsAll(out, "Invalid", "mount"))
@@ -326,5 +330,6 @@ func TestPodSpecResolveWithEnvFile(t *testing.T) {
 	}
 	testCmd.SetArgs(inputArgs)
 	flags.AddFlags(testCmd.Flags())
+	flags.AddUpdateFlags(testCmd.Flags())
 	testCmd.Execute()
 }