cli: use Cobra's positional argument validation

Positional argument validation was added to spf13/cobra in
spf13/cobra#284, but our cli still
relies on custom validation in each of the "run" functions.
This validation is unstandardized and overly permissive in
certain cases. This change fixes this by replacing our custom
logic with validation expressed using Cobra's `PositionalArgs`
validators.

Release note: None

pkg/cli/cert.go

@@ -51,6 +51,7 @@ The certs directory is created if it does not exist.
 If the CA key exists and --allow-ca-key-reuse is true, the key is used.
 If the CA certificate exists and --overwrite is true, the new CA certificate is prepended to it.
 `,
+	Args: cobra.NoArgs,
 	RunE: MaybeDecorateGRPCError(runCreateCACert),
 }
 
@@ -84,6 +85,12 @@ Requires a CA cert in "<certs-dir>/ca.crt" and matching key in "--ca-key".
 If "ca.crt" contains more than one certificate, the first is used.
 Creation fails if the CA expiration time is before the desired certificate expiration.
 `,
+	Args: func(cmd *cobra.Command, args []string) error {
+		if len(args) == 0 {
+			return errors.Errorf("create-node requires at least one host name or address, none was specified")
+		}
+		return nil
+	},
 	RunE: MaybeDecorateGRPCError(runCreateNodeCert),
 }
 
@@ -119,6 +126,7 @@ Requires a CA cert in "<certs-dir>/ca.crt" and matching key in "--ca-key".
 If "ca.crt" contains more than one certificate, the first is used.
 Creation fails if the CA expiration time is before the desired certificate expiration.
 `,
+	Args: cobra.ExactArgs(1),
 	RunE: MaybeDecorateGRPCError(runCreateClientCert),
 }
 
@@ -127,10 +135,6 @@ Creation fails if the CA expiration time is before the desired certificate expir
 // TODO(marc): there is currently no way to specify which CA cert to use if more
 // than one if present.
 func runCreateClientCert(cmd *cobra.Command, args []string) error {
-	if len(args) != 1 {
-		return usageAndError(cmd)
-	}
-
 	var err error
 	var username string
 	if username, err = sql.NormalizeAndValidateUsername(args[0]); err != nil {
@@ -156,15 +160,12 @@ var listCertsCmd = &cobra.Command{
 	Long: `
 List certificates and keys found in the certificate directory.
 `,
+	Args: cobra.NoArgs,
 	RunE: MaybeDecorateGRPCError(runListCerts),
 }
 
 // runListCerts loads and lists all certs.
 func runListCerts(cmd *cobra.Command, args []string) error {
-	if len(args) != 0 {
-		return usageAndError(cmd)
-	}
-
 	cm, err := baseCfg.GetCertificateManager()
 	if err != nil {
 		return errors.Wrap(err, "could not get certificate manager")

pkg/cli/cli.go

@@ -81,6 +81,7 @@ var versionCmd = &cobra.Command{
 	Long: `
 Output build version information.
 `,
+	Args: cobra.NoArgs,
 	RunE: func(cmd *cobra.Command, args []string) error {
 		info := build.GetInfo()
 		tw := tabwriter.NewWriter(os.Stdout, 2, 1, 2, ' ', 0)

pkg/cli/cli_test.go

@@ -2061,17 +2061,19 @@ func TestJunkPositionalArguments(t *testing.T) {
 	defer c.cleanup()
 
 	for i, test := range []string{
-		"start junk",
-		"sql junk",
-		"gen man junk",
-		"gen autocomplete junk",
-		"gen example-data intro junk",
+		"start",
+		"sql",
+		"gen man",
+		"gen autocomplete",
+		"gen example-data intro",
 	} {
-		out, err := c.RunWithCapture(test)
+		const junk = "junk"
+		line := test + " " + junk
+		out, err := c.RunWithCapture(line)
 		if err != nil {
 			t.Fatal(errors.Wrap(err, strconv.Itoa(i)))
 		}
-		exp := test + "\ninvalid arguments\n"
+		exp := fmt.Sprintf("%s\nunknown command %q for \"cockroach %s\"\n", line, junk, test)
 		if exp != out {
 			t.Errorf("expected:\n%s\ngot:\n%s", exp, out)
 		}

pkg/cli/debug.go

@@ -60,6 +60,7 @@ var debugKeysCmd = &cobra.Command{
 	Long: `
 Pretty-prints all keys in a store.
 `,
+	Args: cobra.ExactArgs(1),
 	RunE: MaybeDecorateGRPCError(runDebugKeys),
 }
 
@@ -138,10 +139,6 @@ func runDebugKeys(cmd *cobra.Command, args []string) error {
 	stopper := stop.NewStopper()
 	defer stopper.Stop(context.Background())
 
-	if len(args) != 1 {
-		return errors.New("one argument required: dir")
-	}
-
 	db, err := openExistingStore(args[0], stopper, true /* readOnly */)
 	if err != nil {
 		return err
@@ -156,24 +153,21 @@ func runDebugKeys(cmd *cobra.Command, args []string) error {
 }
 
 var debugRangeDataCmd = &cobra.Command{
-	Use:   "range-data [directory] range-id",
+	Use:   "range-data [directory] [range id]",
 	Short: "dump all the data in a range",
 	Long: `
 Pretty-prints all keys and values in a range. By default, includes unreplicated
 state like the raft HardState. With --replicated, only includes data covered by
  the consistency checker.
 `,
+	Args: cobra.ExactArgs(2),
 	RunE: MaybeDecorateGRPCError(runDebugRangeData),
 }
 
 func runDebugRangeData(cmd *cobra.Command, args []string) error {
 	stopper := stop.NewStopper()
 	defer stopper.Stop(context.Background())
 
-	if len(args) != 2 {
-		return errors.New("two arguments required: dir range_id")
-	}
-
 	db, err := openExistingStore(args[0], stopper, true /* readOnly */)
 	if err != nil {
 		return err
@@ -213,6 +207,7 @@ var debugRangeDescriptorsCmd = &cobra.Command{
 	Long: `
 Prints all range descriptors in a store with a history of changes.
 `,
+	Args: cobra.ExactArgs(1),
 	RunE: MaybeDecorateGRPCError(runDebugRangeDescriptors),
 }
 
@@ -424,10 +419,6 @@ func runDebugRangeDescriptors(cmd *cobra.Command, args []string) error {
 	stopper := stop.NewStopper()
 	defer stopper.Stop(context.Background())
 
-	if len(args) != 1 {
-		return errors.New("one argument required: dir")
-	}
-
 	db, err := openExistingStore(args[0], stopper, true /* readOnly */)
 	if err != nil {
 		return err
@@ -448,6 +439,7 @@ Decode a hexadecimal-encoded key and pretty-print it. For example:
 	$ decode-key BB89F902ADB43000151C2D1ED07DE6C009
 	/Table/51/1/44938288/1521140384.514565824,0
 `,
+	Args: cobra.ArbitraryArgs,
 	RunE: func(cmd *cobra.Command, args []string) error {
 		for _, arg := range args {
 			b, err := hex.DecodeString(arg)
@@ -470,6 +462,7 @@ var debugRaftLogCmd = &cobra.Command{
 	Long: `
 Prints all log entries in a store for the given range.
 `,
+	Args: cobra.ExactArgs(2),
 	RunE: MaybeDecorateGRPCError(runDebugRaftLog),
 }
 
@@ -528,10 +521,6 @@ func runDebugRaftLog(cmd *cobra.Command, args []string) error {
 	stopper := stop.NewStopper()
 	defer stopper.Stop(context.Background())
 
-	if len(args) != 2 {
-		return errors.New("two arguments required: dir range_id")
-	}
-
 	db, err := openExistingStore(args[0], stopper, true /* readOnly */)
 	if err != nil {
 		return err
@@ -560,6 +549,7 @@ ranges individually.
 
 Uses a hard-coded GC policy with a 24 hour TTL for old versions.
 `,
+	Args: cobra.RangeArgs(1, 2),
 	RunE: MaybeDecorateGRPCError(runDebugGCCmd),
 }
 
@@ -568,18 +558,11 @@ func runDebugGCCmd(cmd *cobra.Command, args []string) error {
 	defer stopper.Stop(context.Background())
 
 	var rangeID roachpb.RangeID
-	switch len(args) {
-
-	}
-	switch len(args) {
-	case 2:
+	if len(args) == 2 {
 		var err error
 		if rangeID, err = parseRangeID(args[1]); err != nil {
 			return err
 		}
-	case 1:
-	default:
-		return errors.New("arguments: dir [range_id]")
 	}
 
 	db, err := openExistingStore(args[0], stopper, true /* readOnly */)
@@ -650,6 +633,7 @@ Capable of detecting the following errors:
 * Raft logs that are inconsistent with their metadata
 * MVCC stats that are inconsistent with the data within the range
 `,
+	Args: cobra.ExactArgs(1),
 	RunE: MaybeDecorateGRPCError(runDebugCheckStoreCmd),
 }
 
@@ -666,10 +650,6 @@ func runDebugCheckStoreCmd(cmd *cobra.Command, args []string) error {
 
 	ctx := context.Background()
 
-	if len(args) != 1 {
-		return errors.New("one required argument: dir")
-	}
-
 	db, err := openExistingStore(args[0], stopper, true /* readOnly */)
 	if err != nil {
 		return err
@@ -812,6 +792,7 @@ var debugEnvCmd = &cobra.Command{
 	Long: `
 Output environment variables that influence configuration.
 `,
+	Args: cobra.NoArgs,
 	Run: func(cmd *cobra.Command, args []string) {
 		env := envutil.GetEnvReport()
 		fmt.Print(env)
@@ -824,17 +805,14 @@ var debugCompactCmd = &cobra.Command{
 	Long: `
 Compact the sstables in a store.
 `,
+	Args: cobra.ExactArgs(1),
 	RunE: MaybeDecorateGRPCError(runDebugCompact),
 }
 
 func runDebugCompact(cmd *cobra.Command, args []string) error {
 	stopper := stop.NewStopper()
 	defer stopper.Stop(context.Background())
 
-	if len(args) != 1 {
-		return errors.New("one argument is required")
-	}
-
 	db, err := openExistingStore(args[0], stopper, false /* readOnly */)
 	if err != nil {
 		return err
@@ -886,17 +864,14 @@ total files and 14 files that are 129 MiB in size.
 The suffixes K, M, G and T are used for terseness to represent KiB, MiB, GiB
 and TiB.
 `,
+	Args: cobra.ExactArgs(1),
 	RunE: MaybeDecorateGRPCError(runDebugSSTables),
 }
 
 func runDebugSSTables(cmd *cobra.Command, args []string) error {
 	stopper := stop.NewStopper()
 	defer stopper.Stop(context.Background())
 
-	if len(args) != 1 {
-		return errors.New("one argument is required")
-	}
-
 	db, err := openExistingStore(args[0], stopper, true /* readOnly */)
 	if err != nil {
 		return err
@@ -915,6 +890,7 @@ Pretty-prints the values in a node's gossip instance.
 Can connect to a running server to get the values or can be provided with
 a JSON file captured from a node's /_status/gossip/ debug endpoint.
 `,
+	Args: cobra.ExactArgs(1),
 	RunE: MaybeDecorateGRPCError(runDebugGossipValues),
 }
 
@@ -1020,6 +996,7 @@ var debugSyncTestCmd = &cobra.Command{
 	Short: "Run a performance test for WAL sync speed",
 	Long: `
 `,
+	Args:   cobra.MaximumNArgs(1),
 	Hidden: true,
 	RunE:   MaybeDecorateGRPCError(runDebugSyncTest),
 }
@@ -1032,9 +1009,6 @@ var syncTestOpts = synctest.Options{
 
 func runDebugSyncTest(cmd *cobra.Command, args []string) error {
 	syncTestOpts.Dir = "./testdb"
-	if len(args) > 1 {
-		return fmt.Errorf("too many arguments")
-	}
 	if len(args) == 1 {
 		syncTestOpts.Dir = args[0]
 	}

pkg/cli/demo.go

@@ -34,6 +34,7 @@ Start an in-memory, standalone, single-node CockroachDB instance, and open an
 interactive SQL prompt to it.
 `,
 	Example: `  cockroach demo`,
+	Args:    cobra.NoArgs,
 	RunE:    MaybeShoutError(MaybeDecorateGRPCError(runDemo)),
 }
 

pkg/cli/dump.go

@@ -41,14 +41,11 @@ var dumpCmd = &cobra.Command{
 Dump SQL tables of a cockroach database. If the table name
 is omitted, dump all tables in the database.
 `,
+	Args: cobra.MinimumNArgs(1),
 	RunE: MaybeDecorateGRPCError(runDump),
 }
 
 func runDump(cmd *cobra.Command, args []string) error {
-	if len(args) < 1 {
-		return usageAndError(cmd)
-	}
-
 	conn, err := getPasswordAndMakeSQLClient("cockroach dump")
 	if err != nil {
 		return err

pkg/cli/error.go

@@ -111,10 +111,3 @@ func MaybeShoutError(
 		return err
 	}
 }
-
-func usageAndError(cmd *cobra.Command) error {
-	if err := cmd.Usage(); err != nil {
-		return err
-	}
-	return errors.New("invalid arguments")
-}

pkg/cli/examples.go

@@ -41,10 +41,8 @@ func init() {
 		genExampleCmd := &cobra.Command{
 			Use:   meta.Name,
 			Short: meta.Description,
+			Args:  cobra.NoArgs,
 			RunE: func(cmd *cobra.Command, args []string) error {
-				if len(args) > 0 {
-					return usageAndError(cmd)
-				}
 				runGenExamplesCmd(gen)
 				return nil
 			},

pkg/cli/gen.go

@@ -35,14 +35,11 @@ By default, this places man pages into the "man/man1" directory under the curren
 Use "--path=PATH" to override the output directory. For example, to install man pages globally on
 many Unix-like systems, use "--path=/usr/local/share/man/man1".
 `,
+	Args: cobra.NoArgs,
 	RunE: MaybeDecorateGRPCError(runGenManCmd),
 }
 
 func runGenManCmd(cmd *cobra.Command, args []string) error {
-	if len(args) > 0 {
-		return usageAndError(cmd)
-	}
-
 	info := build.GetInfo()
 	header := &doc.GenManHeader{
 		Section: "1",
@@ -90,13 +87,11 @@ override the file location.
 Note that for the generated file to work on OS X, you'll need to install Homebrew's bash-completion
 package (or an equivalent) and follow the post-install instructions.
 `,
+	Args: cobra.NoArgs,
 	RunE: MaybeDecorateGRPCError(runGenAutocompleteCmd),
 }
 
 func runGenAutocompleteCmd(cmd *cobra.Command, args []string) error {
-	if len(args) > 0 {
-		return usageAndError(cmd)
-	}
 	if err := cmd.Root().GenBashCompletionFile(autoCompletePath); err != nil {
 		return err
 	}

pkg/cli/haproxy.go

@@ -45,6 +45,7 @@ The addresses used are those advertized by the nodes themselves. Make sure hapro
 can resolve the hostnames in the configuration file, either by using full-qualified names, or
 running haproxy in the same network.
 `,
+	Args: cobra.NoArgs,
 	RunE: MaybeDecorateGRPCError(runGenHAProxyCmd),
 }
 
@@ -84,10 +85,6 @@ func nodeStatusesToNodeInfos(statuses []status.NodeStatus) []haProxyNodeInfo {
 }
 
 func runGenHAProxyCmd(cmd *cobra.Command, args []string) error {
-	if len(args) > 0 {
-		return usageAndError(cmd)
-	}
-
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()