kn service update should print an error message when invalid or no option is passed

[toVersus]

In what area(s)?

/kind good-first-issue
/kind feature

Describe the feature:

When users run kn service update command with invalid options, nothing is changed. But in current implementation, kn prints the following output:

$ ./kn service update hello --max-scale -100
Waiting for service 'hello' to become ready ... OK
Service 'hello' updated in namespace 'default'.

IMO, this is a little confusing behavior. Kn should print unchanged when it changed nothing as kubectl does. I think we can implement this feature by comparing the generation before and after updating the service resource and returning changed/unchanged state (boolean).

$ ./kn service update hello --max-scale -100
Service 'hello' unchanged in namespace 'default'.

Can I take this, if you can accept this proposal?

[rhuss]

Shouldn't we just print an error in this case ? like "-100 is an invalid value for max-scale".

Anything else would be surprising.

[rhuss]

And then the "unchanged" message is unnecessary (you perfectly can assume that the service is not changed if an error occurs)

[toVersus]

Shouldn't we just print an error in this case ?

Ah, maybe I mixed up things, so aren't there any strong opinions in current behavior, which silently keeps "unchanged" state if users input invalid values. Can we require to pass at least one option at anytime for avoiding the following case ?

$ ./kn service update hello
Waiting for service 'hello' to become ready ... OK
Service 'hello' updated in namespace 'default'.

[rhuss]

Good point. We should catch that and return an error for such a no-op.

Of course, when we update with a new value == old value, we should not worry about (and should also print the success message)

[zhanggbj]

Hi @toVersus @rhuss ,

If nobody's working on this, I would like to take it. Thanks!
From the above comments, I think two items need to fix. Please let me know if anything is unclear.

• Add input validation for kn service update flags, for e.g., if --max-scale -100 is specified, we should return an error.

• If no input for kn service update, we should return an error.

[rhuss]

@zhanggbj agreed on the action items.

/assign zhanggbj

[toVersus]

@zhanggbj Thanks for picking it up! I worked on the first item in #279. I would appreciate if you look into the PR before implementing the second action item.

[zhanggbj]

@toVersus Sure, no problem! I just noticed your PR, maybe you can refer this issue item in your PR comment so people can know you are working on it :)