Add support to combine service create --filename with other options

[dsimansk]

Feature request

Per PR #913 review discussion and comment. 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 reflected.

err = editFlags.Apply(&service, nil, cmd)
if err != nil {
    return nil, err
}

[danielhelfand]

In the event a user defines both a serviceaccount property in the file and via a flag, which should be applied or should an error be thrown?

[rhuss]

IMO, the resolution should be:

• If something is defined on the command line it always takes precedence. A good use case is --namespace as the file applied might have been extracted from a resource from another cluster which has a different namespace setup. Also, anything that a user provides on the CLI should have an effect and should not be ignored.
• Except for the name which is the identifier. If provided on both sides and they are different, then an error should be thrown.

[duglin]

Not sure how I feel about it yet but why would name be different? Someone could be using someone else’s yaml as a common baseline, and it has a name , but they need a new name in their env. Erroring would force them to dup the file and get out of sync.

[rhuss]

tbh, I can't see any use case why someone would like to change the name of a service, as the name is used in Knative (and in K8s in general) as an identifier. I see this feature more like a variant of kubectl edit where the input comes from the command line. With kubectl edit you can't change the name either. Also, when we want to have the same functionality for kn service update (or kn service apply when it comes), we can't allow overwriting the name anyway.

If you have a good use case why one would need a new name in their env, maybe this use case is more appealing than having this kind of consistency across kn service create and kn service update.

Another idea was also to allow the yaml file to be an arbitrary list of something, and with the name given on the command line, the service definition could be picked from the list in the file. Not sure if this is a good idea though. But this would also require consistency of the names.

[rhuss]

I suggest to implement the overwrite of other options first and then finally discuss and decide what to do with a name collision. In the meantime, we can collect arguments pro and against having to allow a name overwrite.

[duglin]

I was only thinking of 'create' since that's what the title of the issue mentioned. In that case I can see someone hosting a yaml file in their repo that they use but then someone else wants to use it themselves, so they can either copy it (and manually keep it in sync) just so they can change the name, or we can allow them to override the name. This promotes reuse.

As for 'update', "name" should be ignored since you can't update a ksvc name.

[duglin]

Also, my statement about ignoring "name" on kn update applies regardless of whether we allow people to specify cmd line flags. Once a ksvc is created, if we allow people to update it via a file then the "name" field from the file would still need to be ignored. Or throw an error I guess, but either way, the same rules would apply for it appearing on the cmd line or in the file.... you can't change the name of ksvc once it's created.

The cmd line stuff should just be a convenience thing to allow people to avoid the pain of editing the file before running the command (this is just normal options processing... cmd line overrides env vars which overrides config files). I don't see this being any different. How the net (merged) result interacts with the server doesn't change.

[rhuss]

As for 'update', "name" should be ignored since you can't update a ksvc name.

But you need the name to find out which service to update ?

[duglin]

But you need to find out which service to update ?

doi! :-) My mind was stuck on kn service update --name foo --file .... - so yep, w/o --name on cmd line you'll need to use the name from the file. But if --name is there, I'd suggest we just ignore the one in the file - similar to how the namespace field in a kube yaml will be ignored if you use -n on the cmd line

[duglin]

edited^^

[dsimansk]

I suggest to implement the overwrite of other options first and then finally discuss and decide what to do with a name collision. In the meantime, we can collect arguments pro and against having to allow a name overwrite.

I'm more inclined towards overriding the name from file when it's provided as cmd param to have a consistent policy towards flags that everything provided by the user on cmdline takes precedence over file input. Therefore single value params/flags like name,namespace,service-account will override the file's values, list flags like env will be merged and override on the same key etc.

Then there's one use case to handle YAML's --- separated list. That's where name param would be useful to select the desired service to create and throw not found error otherwise.