diff --git a/go/cmd/vtctldclient/cli/cobra.go b/go/cmd/vtctldclient/cli/cobra.go new file mode 100644 index 00000000000..d3f43bddbfb --- /dev/null +++ b/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 +} diff --git a/go/cmd/vtctldclient/internal/command/backups.go b/go/cmd/vtctldclient/internal/command/backups.go index 3b1fea947d2..21d5673ef34 100644 --- a/go/cmd/vtctldclient/internal/command/backups.go +++ b/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) diff --git a/go/cmd/vtctldclient/internal/command/cells.go b/go/cmd/vtctldclient/internal/command/cells.go index e37709abc44..e04984f761b 100644 --- a/go/cmd/vtctldclient/internal/command/cells.go +++ b/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 diff --git a/go/cmd/vtctldclient/internal/command/doc.go b/go/cmd/vtctldclient/internal/command/doc.go index 57500b0fccd..b83db0ef0a4 100644 --- a/go/cmd/vtctldclient/internal/command/doc.go +++ b/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 diff --git a/go/cmd/vtctldclient/internal/command/keyspaces.go b/go/cmd/vtctldclient/internal/command/keyspaces.go index 0f3ec9fcc1d..c9e779358a1 100644 --- a/go/cmd/vtctldclient/internal/command/keyspaces.go +++ b/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) diff --git a/go/cmd/vtctldclient/internal/command/schema.go b/go/cmd/vtctldclient/internal/command/schema.go index da6493d156e..f1e438df308 100644 --- a/go/cmd/vtctldclient/internal/command/schema.go +++ b/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, diff --git a/go/cmd/vtctldclient/internal/command/shards.go b/go/cmd/vtctldclient/internal/command/shards.go index 029e9be97fd..62d8205fb23 100644 --- a/go/cmd/vtctldclient/internal/command/shards.go +++ b/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{ diff --git a/go/cmd/vtctldclient/internal/command/tablets.go b/go/cmd/vtctldclient/internal/command/tablets.go index 5b56ff38d32..dcf7e832d54 100644 --- a/go/cmd/vtctldclient/internal/command/tablets.go +++ b/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, diff --git a/go/cmd/vtctldclient/internal/command/vschemas.go b/go/cmd/vtctldclient/internal/command/vschemas.go index 1c5e848e6d2..0184b9b50ac 100644 --- a/go/cmd/vtctldclient/internal/command/vschemas.go +++ b/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{