Merge pull request vitessio#7451 from tinyspeck/am_vtctldclient_errors

Provide named function for squashing usage errors; start using it

go.mod

@@ -80,6 +80,7 @@ require (
 	github.com/sjmudd/stopwatch v0.0.0-20170613150411-f380bf8a9be1
 	github.com/smartystreets/goconvey v1.6.4 // indirect
 	github.com/spf13/cobra v0.0.5
+	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.4.0
 	github.com/tchap/go-patricia v0.0.0-20160729071656-dd168db6051b
 	github.com/tebeka/selenium v0.9.9

go/cmd/vtctldclient/cli/cobra.go

@@ -0,0 +1,32 @@
+/*
+Copyright 2021 The Vitess Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cli
+
+import "github.com/spf13/cobra"
+
+// FinishedParsing transitions a cobra.Command from treating RunE errors as
+// usage errors to treating them just as normal runtime errors that should be
+// propagated up to the root command's Execute method without also printing the
+// subcommand's usage text on stderr. A subcommand should call this function
+// from its RunE function when it has finished processing its flags and is
+// moving into the pure "business logic" of its entrypoint.
+//
+// Package vitess.io/vitess/go/cmd/vtctldclient/internal/command has more
+// details on why this exists.
+func FinishedParsing(cmd *cobra.Command) {
+	cmd.SilenceUsage = true
+}

go/cmd/vtctldclient/internal/command/backups.go

@@ -22,6 +22,7 @@ import (
 
 	"github.com/spf13/cobra"
 
+	"vitess.io/vitess/go/cmd/vtctldclient/cli"
 	vtctldatapb "vitess.io/vitess/go/vt/proto/vtctldata"
 )
 
@@ -33,6 +34,8 @@ var GetBackups = &cobra.Command{
 }
 
 func commandGetBackups(cmd *cobra.Command, args []string) error {
+	cli.FinishedParsing(cmd)
+
 	keyspace := cmd.Flags().Arg(0)
 	shard := cmd.Flags().Arg(1)
 

go/cmd/vtctldclient/internal/command/cells.go

@@ -49,6 +49,8 @@ var (
 )
 
 func commandGetCellInfoNames(cmd *cobra.Command, args []string) error {
+	cli.FinishedParsing(cmd)
+
 	resp, err := client.GetCellInfoNames(commandCtx, &vtctldatapb.GetCellInfoNamesRequest{})
 	if err != nil {
 		return err
@@ -60,9 +62,11 @@ func commandGetCellInfoNames(cmd *cobra.Command, args []string) error {
 }
 
 func commandGetCellInfo(cmd *cobra.Command, args []string) error {
+	cli.FinishedParsing(cmd)
+
 	cell := cmd.Flags().Arg(0)
-	resp, err := client.GetCellInfo(commandCtx, &vtctldatapb.GetCellInfoRequest{Cell: cell})
 
+	resp, err := client.GetCellInfo(commandCtx, &vtctldatapb.GetCellInfoRequest{Cell: cell})
 	if err != nil {
 		return err
 	}
@@ -78,6 +82,8 @@ func commandGetCellInfo(cmd *cobra.Command, args []string) error {
 }
 
 func commandGetCellsAliases(cmd *cobra.Command, args []string) error {
+	cli.FinishedParsing(cmd)
+
 	resp, err := client.GetCellsAliases(commandCtx, &vtctldatapb.GetCellsAliasesRequest{})
 	if err != nil {
 		return err

go/cmd/vtctldclient/internal/command/doc.go

@@ -107,5 +107,38 @@ A good pattern we have found is to do the following:
 
 		Root.AddCommand(GetTablet)
 	}
+
+A note on RunE and SilenceUsage:
+
+We prefer using RunE over Run for the entrypoint to our subcommands, because it
+allows us return errors back up to the vtctldclient main function and do error
+handling, logging, and exit-code management once, in one place, rather than on a
+per-command basis. However, cobra treats errors returned from a command's RunE
+as usage errors, and therefore will print the command's full usage text to
+stderr when RunE returns non-nil, in addition to propagating that error back up
+to the result of the root command's Execute() method. This is decidedly not what
+we want. There is no plan to address this in cobra v1. [1]
+
+The suggested workaround for this issue is to set SilenceUsage: true, either on
+the root command or on every subcommand individually. This also does not work
+for vtctldclient, because not every flag can be parsed during pflag.Parse time,
+and for certain flags (mutually exclusive options, optional flags that require
+other flags to be set with them, etc) we do additional parsing and validation of
+flags in an individual subcommand. We want errors during this phase to be
+treated as usage errors, so setting SilenceUsage=true before this point would
+not cause usage text to be printed for us.
+
+So, for us, we want to individually set cmd.SilenceUsage = true at *particular
+points* in each command, dependending on whether that command needs to do
+an additional parse & validation pass. In most cases, the command does not need
+to post-validate its options, and can set cmd.SilencUsage = true as their first
+line. We feel, though, that a line that reads "SilenceUsage = true" to be
+potentially confusing in how it reads. A maintainer without sufficient context
+may read this and say "Silence usage? We don't want that" and remove the lines,
+so we provide a wrapper function that communicates intent, cli.FinishedParsing,
+that each subcommand should call when they have transitioned from the parsing &
+validation phase of their entrypoint to the actual logic.
+
+[1]: https://github.com/spf13/cobra/issues/340
 */
 package command

go/cmd/vtctldclient/internal/command/keyspaces.go

@@ -121,6 +121,8 @@ func commandCreateKeyspace(cmd *cobra.Command, args []string) error {
 		snapshotTime = logutil.TimeToProto(t)
 	}
 
+	cli.FinishedParsing(cmd)
+
 	req := &vtctldatapb.CreateKeyspaceRequest{
 		Name:               name,
 		Force:              createKeyspaceOptions.Force,
@@ -164,12 +166,14 @@ var deleteKeyspaceOptions = struct {
 }{}
 
 func commandDeleteKeyspace(cmd *cobra.Command, args []string) error {
-	ks := cmd.Flags().Arg(0)
+	cli.FinishedParsing(cmd)
 
+	ks := cmd.Flags().Arg(0)
 	_, err := client.DeleteKeyspace(commandCtx, &vtctldatapb.DeleteKeyspaceRequest{
 		Keyspace:  ks,
 		Recursive: deleteKeyspaceOptions.Recursive,
 	})
+
 	if err != nil {
 		return fmt.Errorf("DeleteKeyspace(%v) error: %w; please check the topo", ks, err)
 	}
@@ -180,6 +184,8 @@ func commandDeleteKeyspace(cmd *cobra.Command, args []string) error {
 }
 
 func commandFindAllShardsInKeyspace(cmd *cobra.Command, args []string) error {
+	cli.FinishedParsing(cmd)
+
 	ks := cmd.Flags().Arg(0)
 	resp, err := client.FindAllShardsInKeyspace(commandCtx, &vtctldatapb.FindAllShardsInKeyspaceRequest{
 		Keyspace: ks,
@@ -199,6 +205,8 @@ func commandFindAllShardsInKeyspace(cmd *cobra.Command, args []string) error {
 }
 
 func commandGetKeyspace(cmd *cobra.Command, args []string) error {
+	cli.FinishedParsing(cmd)
+
 	ks := cmd.Flags().Arg(0)
 	resp, err := client.GetKeyspace(commandCtx, &vtctldatapb.GetKeyspaceRequest{
 		Keyspace: ks,
@@ -214,6 +222,8 @@ func commandGetKeyspace(cmd *cobra.Command, args []string) error {
 }
 
 func commandGetKeyspaces(cmd *cobra.Command, args []string) error {
+	cli.FinishedParsing(cmd)
+
 	resp, err := client.GetKeyspaces(commandCtx, &vtctldatapb.GetKeyspacesRequest{})
 	if err != nil {
 		return err
@@ -235,6 +245,8 @@ var removeKeyspaceCellOptions = struct {
 }{}
 
 func commandRemoveKeyspaceCell(cmd *cobra.Command, args []string) error {
+	cli.FinishedParsing(cmd)
+
 	keyspace := cmd.Flags().Arg(0)
 	cell := cmd.Flags().Arg(1)
 

go/cmd/vtctldclient/internal/command/schema.go

@@ -54,6 +54,8 @@ func commandGetSchema(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	cli.FinishedParsing(cmd)
+
 	resp, err := client.GetSchema(commandCtx, &vtctldatapb.GetSchemaRequest{
 		TabletAlias:    alias,
 		Tables:         getSchemaOptions.Tables,

go/cmd/vtctldclient/internal/command/shards.go

@@ -65,6 +65,8 @@ func commandCreateShard(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	cli.FinishedParsing(cmd)
+
 	resp, err := client.CreateShard(commandCtx, &vtctldatapb.CreateShardRequest{
 		Keyspace:      keyspace,
 		ShardName:     shard,
@@ -96,6 +98,8 @@ func commandDeleteShards(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	cli.FinishedParsing(cmd)
+
 	_, err = client.DeleteShards(commandCtx, &vtctldatapb.DeleteShardsRequest{
 		Shards:        shards,
 		EvenIfServing: deleteShardsOptions.EvenIfServing,
@@ -117,6 +121,8 @@ func commandGetShard(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	cli.FinishedParsing(cmd)
+
 	resp, err := client.GetShard(commandCtx, &vtctldatapb.GetShardRequest{
 		Keyspace:  keyspace,
 		ShardName: shard,
@@ -146,6 +152,8 @@ func commandRemoveShardCell(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	cli.FinishedParsing(cmd)
+
 	cell := cmd.Flags().Arg(1)
 
 	_, err = client.RemoveShardCell(commandCtx, &vtctldatapb.RemoveShardCellRequest{

go/cmd/vtctldclient/internal/command/tablets.go

@@ -73,6 +73,8 @@ func commandChangeTabletType(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	cli.FinishedParsing(cmd)
+
 	resp, err := client.ChangeTabletType(commandCtx, &vtctldatapb.ChangeTabletTypeRequest{
 		TabletAlias: alias,
 		DbType:      newType,
@@ -102,6 +104,8 @@ func commandDeleteTablets(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	cli.FinishedParsing(cmd)
+
 	_, err = client.DeleteTablets(commandCtx, &vtctldatapb.DeleteTabletsRequest{
 		TabletAliases: aliases,
 		AllowPrimary:  deleteTabletsOptions.AllowPrimary,
@@ -123,6 +127,8 @@ func commandGetTablet(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	cli.FinishedParsing(cmd)
+
 	resp, err := client.GetTablet(commandCtx, &vtctldatapb.GetTabletRequest{TabletAlias: alias})
 	if err != nil {
 		return err
@@ -159,6 +165,8 @@ func commandGetTablets(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("--shard (= %s) cannot be passed without also passing --keyspace", getTabletsOptions.Shard)
 	}
 
+	cli.FinishedParsing(cmd)
+
 	resp, err := client.GetTablets(commandCtx, &vtctldatapb.GetTabletsRequest{
 		Cells:    getTabletsOptions.Cells,
 		Keyspace: getTabletsOptions.Keyspace,

go/cmd/vtctldclient/internal/command/vschemas.go

@@ -42,6 +42,8 @@ var (
 )
 
 func commandGetSrvVSchema(cmd *cobra.Command, args []string) error {
+	cli.FinishedParsing(cmd)
+
 	cell := cmd.Flags().Arg(0)
 
 	resp, err := client.GetSrvVSchema(commandCtx, &vtctldatapb.GetSrvVSchemaRequest{
@@ -62,6 +64,8 @@ func commandGetSrvVSchema(cmd *cobra.Command, args []string) error {
 }
 
 func commandGetVSchema(cmd *cobra.Command, args []string) error {
+	cli.FinishedParsing(cmd)
+
 	keyspace := cmd.Flags().Arg(0)
 
 	resp, err := client.GetVSchema(commandCtx, &vtctldatapb.GetVSchemaRequest{