fix: remove NoArgs positional arguments validation

[yashvardhan-kukreja]

fixes #3466

~/Projects/GoProjects/kind issue-3466/no-args-fix                                                                                                                                                   05:26:35
❯ ./bin/kind clusters get > /dev/null && echo good || echo bad
ERROR: unknown command "clusters" for "kind"
bad

~/Projects/GoProjects/kind issue-3466/no-args-fix                                                                                                                                                   05:27:00
❯ ./bin/kind get clusters > /dev/null && echo good || echo bad
enabling experimental podman provider
good

~/Projects/GoProjects/kind issue-3466/no-args-fix                                                                                                                                                   05:27:06
❯ ./bin/kind get clusters dfkni > /dev/null && echo good || echo bad
ERROR: unknown command "dfkni" for "kind get clusters"
bad

~/Projects/GoProjects/kind issue-3466/no-args-fix                                                                                                                                                   05:27:14
❯ ./bin/kind create cluster sds d sd> /dev/null && echo good || echo bad
ERROR: unknown command "sds" for "kind create cluster"
bad

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yashvardhan-kukreja
Once this PR has been reviewed and has the lgtm label, please assign bentheelder for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @yashvardhan-kukreja. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[aojea]

what happen now if I pass arguments that are not going to be used?

[yashvardhan-kukreja]

what happen now if I pass arguments that are not going to be used?

Can you give a more specific example which might be going through your mind?

Meanwhile I did try providing unusable args/flag (all of these correlate to "bad" with the present release of KinD)

❯ ./bin/kind get clusters -v3 --a3 -b3 > /dev/null && echo good || echo bad 
ERROR: unknown flag: --a3
bad

❯ kind get clusters -v3 --a3 -b3 > /dev/null && echo good || echo bad
ERROR: unknown flag: --a3
bad

❯ ./bin/kind get clust clusters > /dev/null && echo good || echo bad
ERROR: Subcommand is required
bad

❯ ./bin/kind get clusters clust > /dev/null && echo good || echo bad 
ERROR: unknown command "clust" for "kind get clusters"
bad

[aojea]

I'm not familiar enough with cobra, I was wondering what change on behavior is causing the removal of this option

[yashvardhan-kukreja]

Sure, let me elaborate on what I found when diggin originally into this issue, which ultimately led to the solution this PR suggests.

Cobra calls Find method here when executing the commands - Ref

Here you can see the Find method is ignoring the validation which we can benefit from i.e. the legacyArgs validation which enforces the fact that any parent-less subcommand (i.e. wrong subcommand) is immediately returned as an error.

A more graceful way would have been to already have a PositionalArgument function written (like cobra.NoArgs) exactly matching our requirements but that doesn't seem to exist at the moment. Moreover, we could have chosen to write our own such function but it would've been an overkill in my opinion.

Finally, it would have been much nicer if cobra returned an error here instead of just returning a nil error alongside logging out the help when a wrong set of command are provided.

(It almost looks like a bug/mistake to me that they added a return cmd, nil. I would've expected to return an error there and leave to the invoker to compare it against flag.ErrHelp to handle it on their own).

[yashvardhan-kukreja]

I was wondering what change on behavior is causing the removal of this option

The only change I noticed was the change in how things were logged when wrong subcommands were provided.

Previously, kind had to reach here to eventually face an error log the information accordingly.

But after this PR, any wrong subcommands would fail much before i.e. here only

Also, Any non-leaf commands provided with unknown arguments or no arguments will lead to a different error

For example:

Old behaviour of KinD

❯ kind get clust x > /dev/null && echo good || echo bad
ERROR: unknown command "clust" for "kind get"
bad

New behaviour of KinD

❯ ./bin/kind get clust x > /dev/null && echo good || echo bad
ERROR: Subcommand is required
bad

And, any leaf subcommands like clusters (having no other children subcommands) will fail in the same manner as before.

For example
Old behaviour of KinD

❯ kind get clusters cl    
ERROR: unknown command "cl" for "kind get clusters"

New behaviour of KinD

❯ ./bin/kind get clusters cl   
ERROR: unknown command "cl" for "kind get clusters"

[yashvardhan-kukreja]

@aojea mind taking a look at this once you get free?

[stmcginnis]

In this case, being less explicit does seem to be the better option so we get the appropriate non-zero return code.

/lgtm