Kubectl: --show-labels shows up on "kubectl create [...]"

[ahmetb]

• (ideally) --show-labels should not show up at kubectl create service command('s help)
• and it should error out when it's silently ignored like it's done today

For example:

➜  ~ kubectl create service  loadbalancer --show-labels lb-1 --tcp=80:80
service "lb-1" created

this command should fail and say invalid flag "--show-labels" for "create" or something like that.

[wackxu]

Actually there are so many cases like this. When there are some unused flag but cobra command can recognize, for example https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/util/printing.go#L44-L52
when we use --show-all when we create service but do not specify the -output flag

[ahmetb]

It might be by-design. I'm not sure. I just wanted to bring up in case it is completely useless.

I recently opened an issue in the main repo (now fixed): if you specify --show-labels and incompatible printer (-o yaml) it now prints a validation error.

Similarly I think it should fail if --show-labels isn't applicable to the command.

[apelisse]

Yeah, create uses the printer library (because you can print what you just created). And the printer library registers all the parameters it knows about.

You can do (even if it's completely useless):

> kubectl create svc  clusterip bla --tcp=3432:2131 -o wide --show-labels
NAME      TYPE        CLUSTER-IP     EXTERNAL-IP   PORT(S)    AGE       SELECTOR   LABELS
bla       ClusterIP   10.67.244.97   <none>        3432/TCP   0s        app=bla    app=bla

[apelisse]

We could return an error if --show-labels is specified and output is empty though.

[Lion-Wei]

I can help to fix this. :-)

[apelisse]

Great Lion-Wei, Thanks for working on that! Feel free to ping me on chat or ask for anything. I can't assign to you, so I'll just assign to me to mark it assigned.

[apelisse]

Yeah, thanks, I forgot to do it ahah

[Lion-Wei]

@apelisse I pushed a pr to do this. In that pr, kubectl will give a warning in this cases like this issue mentioned.
I have tested it, works fun. But might be immature in realization. Can you help me take a review? Thanks.

[apelisse]

I'm not sure we have a consensus on what needs to be done, and it's slightly too premature to come-up with code yet.

@ahmetb, can you please re-iterate what you expect?

[ahmetb]

@apelisse I think the right thing to do here is to make sure --show-labels does not show up at kubectl create service --help and is not usable with the kubectl create service and other "create xxx" commands.

[Lion-Wei]

ehhh, Here is how I understand:
When we use kubectl create with --show-labels but with out any -o specify, the --show-labels flag will not work. like:

# k create -f /root/rs-who.yaml --show-labels
replicationcontroller "whoami" created

But this flag not completely useless, cause when we use kubectl create with -owide, add this flag can print lables info, like:

# k create -f /root/rs-who.yaml -owide --show-labels
NAME      DESIRED   CURRENT   READY     AGE       CONTAINERS   IMAGES               SELECTOR     LABELS
whoami    3         0         0         0s        whoami       cizixs/whoami:v0.5   app=whoami   app=whoami,env=dev

So IMO, we can't just disable this flag in kubectl create, we need identify cases that the --show-labels flag are useless, and tell the user somehow...

[Lion-Wei]

If I understand it wrong, please let me know, thanks... @ahmetb @apelisse

[ahmetb]

@Lion-Wei you're right. I wasn't aware of this. in this case we should fail when it does not apply. We recently made similar fix to kubectl where we errored if -o=... value is not compatible with --show-labels (e.g. -o=yaml --show-labels). You can dig that PR up and extract common code.

[Lion-Wei]

Ok, sounds good, I will do that.

[Lion-Wei]

@ahmetb Sorry to ask, but I can't find that pr anywhere, can you give me some information about it? Thanks.

[ahmetb]

https://github.com/kubernetes/kubernetes/pull/55371

[apelisse]

We should probably be more generic and move the verification to the printer code.

[Lion-Wei]

@apelisse I extracted printer verification of delete create and others to util/printing.go. I'm thinking maybe there are other cases that should validate, if so, we can add to that function too.

[apelisse]

I'm thinking that it might make more sense with the printers package itself. That's where the printer logic lives, that's what is supposed to know how to interpret the options, and which combinations of option make sense and which don't.
@smarterclayton who knows the most about printing.

[Lion-Wei]

I can take some time to learn this part code, give the whole logic of printer, then we can decide what to do. You think that will work?

[apelisse]

Yeah, that could. I think if you need to have a deep knowledge of printers, then something must be wrong. So don't get lost into too many details.

[Lion-Wei]

@apelisse Thanks for your advise, sorry take so lang.
Here is what I got from the code.

get

| options | NONE | wide | name | json/yaml | custom-columns | custom-columns-file | jsonpath(-file) | template/go-template/go-template-file |
| ----------------- | :--: | :--: | :--: | :-------: | :------------: | :-----------------: | :-------------: | :-----------------------------------: |
| show-labels | y | y | n | n | n | n | n | n |
| show-all | y | y | y | y | y | y | y | y |
| sort-by | y | y | y | y | y | y | y | y |
| no-headers | y | y | n | n | y | n | n | n |
| template | n | n | n | n | n | n | n | n |
| allow-missing-... | n | n | n | n | n | n | y | y |

create(sub)/apply/patch/set-last-applied/reconcile/expose/label/rollingupdate/run/taint/autoscale/set(sub)/

| options | NONE | wide | name | json/yaml | custom-columns | custom-columns-file | jsonpath(-file) | template/go-template/go-template-file |
| :---------------- | :--: | :--: | :--: | :-------: | :------------: | :-----------------: | :-------------: | :-----------------------------------: |
| show-labels | n | y | n | n | n | n | n | n |
| no-headers | n | y | n | n | n | n | n | n |
| allow-missing-... | n | n | n | n | n | n | y | y |
| sort-by | n | n | n | n | n | n | n | n |
| show-all | n | n | n | n | n | n | n | n |
| template | n | n | n | n | n | n | n | n |

By default, they will use PrintSuccess, which will ignore all print flags. And cause those command print obj one by one, the flag sort-by and show-all will be ignored.

Specially, when we use kubectl set *** with flag --dry-run, then will use PrintObject no matter what output option is, this maybe a little problem. For example:

# k set env rc/whoami LW=la --dry-run
NAME      DESIRED   CURRENT   READY     AGE
whoami    2         2         2         2h
# k set env rc/whoami LW=la 
replicationcontroller "whoami" env updated

delete/replace/scale

Only support out option NONE or name, don't support any output flag.

view

If use output option NONE or wide, will change to -oyaml, only accept flag allow-missing-template-keys.

When use -oname, might have a little problem:

# k config view -oname
<unknown>/<unknown>

# k config view -owide
--output wide is not available in kubectl config view; reset to default output format (yaml)
apiVersion: v1
...

# k config view 
apiVersion: v1
...

I think this should covered all situation.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close