rpk/cli: show help for incorrect number or invalid arguments

[r-vasquez]

Cover letter

This is a proposal to fix half of #300 by adding a wrapper to the validation of positional arguments using the MatchAll used in cobra latest release and adding the usage after the validation error.

Im doing it this way to avoid using the silenceUsage=false at root level and have control on the desired behavior for each error.

This will require to modify all commands so I'm requesting your feedback before proceeding with the rest of the commands.

before:

rpk api topic create
Error: requires at least 1 arg(s), only received 0

after

rpk api topic create
Error: requires at least 1 arg(s), only received 0


Usage:
  rpk topic create [TOPICS...] [flags]

Flags:
  -d, --dry                        dry run: validate the topic creation request; do not create topics
  -h, --help                       help for create
  -p, --partitions int32           Number of partitions to create per topic (default 1)
  -r, --replicas int16             Replication factor; if -1, this will be the broker's default.replication.factor (default -1)
  -c, --topic-config stringArray   key=value; Config parameters (repeatable; e.g. -c cleanup.policy=compact)

Global Flags:
      --brokers strings         Comma-separated list of broker ip:port pairs (e.g. --brokers '192.168.78.34:9092,192.168.78.35:9092,192.179.23.54:9092' ). Alternatively, you may set the REDPANDA_BROKERS environment variable with the comma-separated list of broker addresses.
      --config string           Redpanda config file, if not set the file will be searched for in the default locations
      --password string         SASL password to be used for authentication.
      --sasl-mechanism string   The authentication mechanism to use. Supported values: SCRAM-SHA-256, SCRAM-SHA-512.
      --tls-cert string         The certificate to be used for TLS authentication with the broker.
      --tls-enabled             Enable TLS for the Kafka API (not necessary if specifying custom certs).
      --tls-key string          The certificate key to be used for TLS authentication with the broker.
      --tls-truststore string   The truststore to be used for TLS communication with the broker.
      --user string             SASL user to be used for authentication.
  -v, --verbose                 Enable verbose logging (default: false).

Release notes

• none

[CLAassistant]

All committers have signed the CLA.

[0x5d]

@r-vasquez thank you for tackling this! I left a couple small requests.

[r-vasquez]

Finished adding wrapper function to all commands and also the other half of the fix for #300 in https://github.com/r-vasquez/redpanda/blob/2beb1ba512950d84022d7e086e1b50ced6542f12/src/go/rpk/pkg/cli/cmd/root.go#L77

Let me know your comments. Thanks!

[twmb]

I'm not sure about the reason for setting SilenceUsage = true. The commit that introduced this (bf914d5902) doesn't have context. I was wondering if this is related to how some commands parse flags within the command itself (i.e., handle flags that are invalid to cobra), but that's handled in some other way internally.

What do you think about removing any uses of SilenceUsage = true, and removing the MatchAll introduced here? (sorry for my late review, I've not been feeling well).

As an aside, I'm a little wary about hiding the more custom error message within a vendored MatchAll and trying to consistently use that everywhere. Were we to upgrade to cobra 1.3, I'd expect us to remove the vendored code, but it's not too obvious at first glance that the vendored code actually changes the error message printed as well. Looking at the PR that introduced MatchAll within cobra (spf13/cobra#896), the point of MatchAll is so that a person can use multiple positional validations: cobra.MatchAll(cobra.ExactArgs(3), cobra.OnlyValidArgs). All usages of MatchAll in this PR still keep the one single validation, which gives the impression when maintaining that (match all of one??) that the code isn't necessary, and somebody in the future may accidentally remove it for being extraneous.

So in summary: what do you think about removing SilenceUsage = true, and then removing MatchAll introduced here? I'm trying to get to the bottom of why SilenceUsage = true was introduced but I'm not currently seeing any benefits of that.

[r-vasquez]

Thanks for the review ! Looking to previous commit I found that SilenceUsage = true was helpful to avoid displaying usage whenever we get an error on commands that used runE (Even those that are not related to CLI usage). However that's not the case anymore.

I'll proceed with removing the silenceUsage at root level and leave the FlagErrorFunc to display usage.

[twmb]

I think FlagErrorFunc also needs to be removed now that SilenceUsage is false.

In cobra, FlagErrorFunc is called if flag parsing fails, in the execute logic: https://github.com/spf13/cobra/blob/e04ec725508c760e70263b031e5697c232d5c3fa/command.go#L782-L785. This bubbles up the error. On line 984, since we don't have SilenceErrors = true, this will print the error from FlagErrorFunc, which in this PR is the error and the usage (usage is new in this PR). Then on line 991, because SilenceUsage = true was removed, this will again print the usage.

If I run rpk group list -asdf, I get the following (output trimmed):

Error: unknown shorthand flag: 'a' in -asdf

Usage:
  ...usage and flags

Usage:
  ...repeat

To complicate things a bit further, the FlagErrorFunc is useful for any other usages where SilenceUsage = true remains. See rpk version,

$ rpk version -asdf
Error: unknown shorthand flag: 'a' in -asdf

Usage:
  ...usage printed once

The usage is printed once because SilenceUsage == true, and then this is falling back to the FlagErrorFunc added in this PR. ...I will say that cobra makes this not that intuitive. Anyway, I think SilenceUsage being removed on the root command and everywhere it currently exists is what's necessary:

$ rg SilenceUsage
pkg/cli/cmd/debug/bundle.go
258:		SilenceUsage: true,

pkg/cli/cmd/acl.go
44:		SilenceUsage: true,

pkg/cli/cmd/wasm/generate.go
36:		SilenceUsage: true,

pkg/cli/cmd/redpanda/stop.go
41:		SilenceUsage: true,

pkg/cli/cmd/redpanda/check.go
37:		SilenceUsage: true,

pkg/cli/cmd/version.go
24:		SilenceUsage: true,```

[twmb]

This lgtm now, thank you for bearing through the repeated revisions -- I certainly learned a few things about cobra during this review!

[0x5d]

Thanks! Just a couple suggestions regarding commit history.