From 9e59fc6924b4d1c5f565721cc0fc539a2ce33206 Mon Sep 17 00:00:00 2001 From: Sam Kleinman Date: Fri, 11 Feb 2022 11:30:16 -0500 Subject: [PATCH] libs/cli: clean up package (#7806) --- cmd/tendermint/commands/completion.go | 46 +++++++++++ cmd/tendermint/commands/root_test.go | 33 ++++++-- cmd/tendermint/main.go | 4 +- libs/cli/helper.go | 114 +++++--------------------- libs/cli/setup.go | 98 ++++------------------ libs/cli/setup_test.go | 90 +++++++++++++++++--- 6 files changed, 188 insertions(+), 197 deletions(-) create mode 100644 cmd/tendermint/commands/completion.go diff --git a/cmd/tendermint/commands/completion.go b/cmd/tendermint/commands/completion.go new file mode 100644 index 000000000..d2c81f0af --- /dev/null +++ b/cmd/tendermint/commands/completion.go @@ -0,0 +1,46 @@ +package commands + +import ( + "fmt" + + "github.com/spf13/cobra" +) + +// NewCompletionCmd returns a cobra.Command that generates bash and zsh +// completion scripts for the given root command. If hidden is true, the +// command will not show up in the root command's list of available commands. +func NewCompletionCmd(rootCmd *cobra.Command, hidden bool) *cobra.Command { + flagZsh := "zsh" + cmd := &cobra.Command{ + Use: "completion", + Short: "Generate shell completion scripts", + Long: fmt.Sprintf(`Generate Bash and Zsh completion scripts and print them to STDOUT. + +Once saved to file, a completion script can be loaded in the shell's +current session as shown: + + $ . <(%s completion) + +To configure your bash shell to load completions for each session add to +your $HOME/.bashrc or $HOME/.profile the following instruction: + + . <(%s completion) +`, rootCmd.Use, rootCmd.Use), + RunE: func(cmd *cobra.Command, _ []string) error { + zsh, err := cmd.Flags().GetBool(flagZsh) + if err != nil { + return err + } + if zsh { + return rootCmd.GenZshCompletion(cmd.OutOrStdout()) + } + return rootCmd.GenBashCompletion(cmd.OutOrStdout()) + }, + Hidden: hidden, + Args: cobra.NoArgs, + } + + cmd.Flags().Bool(flagZsh, false, "Generate Zsh completion script") + + return cmd +} diff --git a/cmd/tendermint/commands/root_test.go b/cmd/tendermint/commands/root_test.go index 2eade59b6..54f96f907 100644 --- a/cmd/tendermint/commands/root_test.go +++ b/cmd/tendermint/commands/root_test.go @@ -1,6 +1,7 @@ package commands import ( + "context" "fmt" "os" "path/filepath" @@ -17,6 +18,17 @@ import ( tmos "github.com/tendermint/tendermint/libs/os" ) +// writeConfigVals writes a toml file with the given values. +// It returns an error if writing was impossible. +func writeConfigVals(dir string, vals map[string]string) error { + data := "" + for k, v := range vals { + data += fmt.Sprintf("%s = \"%s\"\n", k, v) + } + cfile := filepath.Join(dir, "config.toml") + return os.WriteFile(cfile, []byte(data), 0600) +} + // clearConfig clears env vars, the given root dir, and resets viper. func clearConfig(t *testing.T, dir string) *cfg.Config { t.Helper() @@ -41,7 +53,7 @@ func testRootCmd(conf *cfg.Config) *cobra.Command { return cmd } -func testSetup(t *testing.T, conf *cfg.Config, args []string, env map[string]string) error { +func testSetup(ctx context.Context, t *testing.T, conf *cfg.Config, args []string, env map[string]string) error { t.Helper() cmd := testRootCmd(conf) @@ -49,7 +61,7 @@ func testSetup(t *testing.T, conf *cfg.Config, args []string, env map[string]str // run with the args and env args = append([]string{cmd.Use}, args...) - return cli.RunWithArgs(cmd, args, env) + return cli.RunWithArgs(ctx, cmd, args, env) } func TestRootHome(t *testing.T) { @@ -65,11 +77,14 @@ func TestRootHome(t *testing.T) { {nil, map[string]string{"TMHOME": newRoot}, newRoot}, } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + for i, tc := range cases { t.Run(fmt.Sprint(i), func(t *testing.T) { conf := clearConfig(t, tc.root) - err := testSetup(t, conf, tc.args, tc.env) + err := testSetup(ctx, t, conf, tc.args, tc.env) require.NoError(t, err) require.Equal(t, tc.root, conf.RootDir) @@ -99,11 +114,14 @@ func TestRootFlagsEnv(t *testing.T) { {nil, map[string]string{"TM_LOG_LEVEL": "debug"}, "debug"}, // right env } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + for i, tc := range cases { t.Run(fmt.Sprint(i), func(t *testing.T) { conf := clearConfig(t, defaultDir) - err := testSetup(t, conf, tc.args, tc.env) + err := testSetup(ctx, t, conf, tc.args, tc.env) require.NoError(t, err) assert.Equal(t, tc.logLevel, conf.LogLevel) @@ -113,6 +131,9 @@ func TestRootFlagsEnv(t *testing.T) { } func TestRootConfig(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + // write non-default config nonDefaultLogLvl := "debug" cvals := map[string]string{ @@ -142,14 +163,14 @@ func TestRootConfig(t *testing.T) { // write the non-defaults to a different path // TODO: support writing sub configs so we can test that too - err = WriteConfigVals(configFilePath, cvals) + err = writeConfigVals(configFilePath, cvals) require.NoError(t, err) cmd := testRootCmd(conf) // run with the args and env tc.args = append([]string{cmd.Use}, tc.args...) - err = cli.RunWithArgs(cmd, tc.args, tc.env) + err = cli.RunWithArgs(ctx, cmd, tc.args, tc.env) require.NoError(t, err) require.Equal(t, tc.logLvl, conf.LogLevel) diff --git a/cmd/tendermint/main.go b/cmd/tendermint/main.go index 6d2391dde..91ee89bea 100644 --- a/cmd/tendermint/main.go +++ b/cmd/tendermint/main.go @@ -44,7 +44,7 @@ func main() { commands.MakeRollbackStateCommand(conf), commands.MakeKeyMigrateCommand(conf, logger), debug.DebugCmd, - cli.NewCompletionCmd(rcmd, true), + commands.NewCompletionCmd(rcmd, true), ) // NOTE: @@ -60,7 +60,7 @@ func main() { // Create & start node rcmd.AddCommand(commands.NewRunNodeCmd(nodeFunc, conf, logger)) - if err := rcmd.ExecuteContext(ctx); err != nil { + if err := cli.RunWithTrace(ctx, rcmd); err != nil { panic(err) } } diff --git a/libs/cli/helper.go b/libs/cli/helper.go index 76f3c9043..6f723ac02 100644 --- a/libs/cli/helper.go +++ b/libs/cli/helper.go @@ -1,29 +1,20 @@ package cli import ( - "bytes" + "context" "fmt" - "io" "os" - "path/filepath" + "runtime" "github.com/spf13/cobra" + "github.com/spf13/viper" ) -// WriteConfigVals writes a toml file with the given values. -// It returns an error if writing was impossible. -func WriteConfigVals(dir string, vals map[string]string) error { - data := "" - for k, v := range vals { - data += fmt.Sprintf("%s = \"%s\"\n", k, v) - } - cfile := filepath.Join(dir, "config.toml") - return os.WriteFile(cfile, []byte(data), 0600) -} - // RunWithArgs executes the given command with the specified command line args // and environmental variables set. It returns any error returned from cmd.Execute() -func RunWithArgs(cmd Executable, args []string, env map[string]string) error { +// +// This is only used in testing. +func RunWithArgs(ctx context.Context, cmd *cobra.Command, args []string, env map[string]string) error { oargs := os.Args oenv := map[string]string{} // defer returns the environment back to normal @@ -46,85 +37,24 @@ func RunWithArgs(cmd Executable, args []string, env map[string]string) error { } // and finally run the command - return cmd.Execute() + return RunWithTrace(ctx, cmd) } -// RunCaptureWithArgs executes the given command with the specified command -// line args and environmental variables set. It returns string fields -// representing output written to stdout and stderr, additionally any error -// from cmd.Execute() is also returned -func RunCaptureWithArgs(cmd Executable, args []string, env map[string]string) (stdout, stderr string, err error) { - oldout, olderr := os.Stdout, os.Stderr // keep backup of the real stdout - rOut, wOut, _ := os.Pipe() - rErr, wErr, _ := os.Pipe() - os.Stdout, os.Stderr = wOut, wErr - defer func() { - os.Stdout, os.Stderr = oldout, olderr // restoring the real stdout - }() - - // copy the output in a separate goroutine so printing can't block indefinitely - copyStd := func(reader *os.File) *(chan string) { - stdC := make(chan string) - go func() { - var buf bytes.Buffer - // io.Copy will end when we call reader.Close() below - io.Copy(&buf, reader) //nolint:errcheck //ignore error - select { - case <-cmd.Context().Done(): - case stdC <- buf.String(): - } - }() - return &stdC - } - outC := copyStd(rOut) - errC := copyStd(rErr) - - // now run the command - err = RunWithArgs(cmd, args, env) - - // and grab the stdout to return - wOut.Close() - wErr.Close() - stdout = <-*outC - stderr = <-*errC - return stdout, stderr, err -} - -// NewCompletionCmd returns a cobra.Command that generates bash and zsh -// completion scripts for the given root command. If hidden is true, the -// command will not show up in the root command's list of available commands. -func NewCompletionCmd(rootCmd *cobra.Command, hidden bool) *cobra.Command { - flagZsh := "zsh" - cmd := &cobra.Command{ - Use: "completion", - Short: "Generate shell completion scripts", - Long: fmt.Sprintf(`Generate Bash and Zsh completion scripts and print them to STDOUT. - -Once saved to file, a completion script can be loaded in the shell's -current session as shown: - - $ . <(%s completion) - -To configure your bash shell to load completions for each session add to -your $HOME/.bashrc or $HOME/.profile the following instruction: +func RunWithTrace(ctx context.Context, cmd *cobra.Command) error { + cmd.SilenceUsage = true + cmd.SilenceErrors = true + + if err := cmd.ExecuteContext(ctx); err != nil { + if viper.GetBool(TraceFlag) { + const size = 64 << 10 + buf := make([]byte, size) + buf = buf[:runtime.Stack(buf, false)] + fmt.Fprintf(os.Stderr, "ERROR: %v\n%s\n", err, buf) + } else { + fmt.Fprintf(os.Stderr, "ERROR: %v\n", err) + } - . <(%s completion) -`, rootCmd.Use, rootCmd.Use), - RunE: func(cmd *cobra.Command, _ []string) error { - zsh, err := cmd.Flags().GetBool(flagZsh) - if err != nil { - return err - } - if zsh { - return rootCmd.GenZshCompletion(cmd.OutOrStdout()) - } - return rootCmd.GenBashCompletion(cmd.OutOrStdout()) - }, - Hidden: hidden, - Args: cobra.NoArgs, + return err } - - cmd.Flags().Bool(flagZsh, false, "Generate Zsh completion script") - - return cmd + return nil } diff --git a/libs/cli/setup.go b/libs/cli/setup.go index be69c30af..54ea90358 100644 --- a/libs/cli/setup.go +++ b/libs/cli/setup.go @@ -1,11 +1,8 @@ package cli import ( - "context" - "fmt" "os" "path/filepath" - "runtime" "strings" "github.com/spf13/cobra" @@ -13,52 +10,27 @@ import ( ) const ( - HomeFlag = "home" - TraceFlag = "trace" - OutputFlag = "output" - EncodingFlag = "encoding" + HomeFlag = "home" + TraceFlag = "trace" + OutputFlag = "output" // used in the cli ) -// Executable is the minimal interface to *corba.Command, so we can -// wrap if desired before the test -type Executable interface { - Execute() error - Context() context.Context -} - // PrepareBaseCmd is meant for tendermint and other servers -func PrepareBaseCmd(cmd *cobra.Command, envPrefix, defaultHome string) Executor { +func PrepareBaseCmd(cmd *cobra.Command, envPrefix, defaultHome string) *cobra.Command { + // the primary caller of this command is in the SDK and + // returning the cobra.Command object avoids breaking that + // code. In the long term, the SDK could avoid this entirely. cobra.OnInitialize(func() { InitEnv(envPrefix) }) cmd.PersistentFlags().StringP(HomeFlag, "", defaultHome, "directory for config and data") cmd.PersistentFlags().Bool(TraceFlag, false, "print out full stack trace on errors") cmd.PersistentPreRunE = concatCobraCmdFuncs(BindFlagsLoadViper, cmd.PersistentPreRunE) - return Executor{cmd, os.Exit} -} - -// PrepareMainCmd is meant for client side libs that want some more flags -// -// This adds --encoding (hex, btc, base64) and --output (text, json) to -// the command. These only really make sense in interactive commands. -func PrepareMainCmd(cmd *cobra.Command, envPrefix, defaultHome string) Executor { - cmd.PersistentFlags().StringP(EncodingFlag, "e", "hex", "Binary encoding (hex|b64|btc)") - cmd.PersistentFlags().StringP(OutputFlag, "o", "text", "Output format (text|json)") - cmd.PersistentPreRunE = concatCobraCmdFuncs(validateOutput, cmd.PersistentPreRunE) - return PrepareBaseCmd(cmd, envPrefix, defaultHome) + return cmd } // InitEnv sets to use ENV variables if set. func InitEnv(prefix string) { - copyEnvVars(prefix) - - // env variables with TM prefix (eg. TM_ROOT) - viper.SetEnvPrefix(prefix) - viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_", "-", "_")) - viper.AutomaticEnv() -} - -// This copies all variables like TMROOT to TM_ROOT, -// so we can support both formats for the user -func copyEnvVars(prefix string) { + // This copies all variables like TMROOT to TM_ROOT, + // so we can support both formats for the user prefix = strings.ToUpper(prefix) ps := prefix + "_" for _, e := range os.Environ() { @@ -71,42 +43,11 @@ func copyEnvVars(prefix string) { } } } -} - -// Executor wraps the cobra Command with a nicer Execute method -type Executor struct { - *cobra.Command - Exit func(int) // this is os.Exit by default, override in tests -} -type ExitCoder interface { - ExitCode() int -} - -// execute adds all child commands to the root command sets flags appropriately. -// This is called by main.main(). It only needs to happen once to the rootCmd. -func (e Executor) Execute() error { - e.SilenceUsage = true - e.SilenceErrors = true - err := e.Command.Execute() - if err != nil { - if viper.GetBool(TraceFlag) { - const size = 64 << 10 - buf := make([]byte, size) - buf = buf[:runtime.Stack(buf, false)] - fmt.Fprintf(os.Stderr, "ERROR: %v\n%s\n", err, buf) - } else { - fmt.Fprintf(os.Stderr, "ERROR: %v\n", err) - } - - // return error code 1 by default, can override it with a special error type - exitCode := 1 - if ec, ok := err.(ExitCoder); ok { - exitCode = ec.ExitCode() - } - e.Exit(exitCode) - } - return err + // env variables with TM prefix (eg. TM_ROOT) + viper.SetEnvPrefix(prefix) + viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_", "-", "_")) + viper.AutomaticEnv() } type cobraCmdFunc func(cmd *cobra.Command, args []string) error @@ -149,14 +90,3 @@ func BindFlagsLoadViper(cmd *cobra.Command, args []string) error { } return nil } - -func validateOutput(cmd *cobra.Command, args []string) error { - // validate output format - output := viper.GetString(OutputFlag) - switch output { - case "text", "json": - default: - return fmt.Errorf("unsupported output format: %s", output) - } - return nil -} diff --git a/libs/cli/setup_test.go b/libs/cli/setup_test.go index bc62481af..4c23c722a 100644 --- a/libs/cli/setup_test.go +++ b/libs/cli/setup_test.go @@ -1,8 +1,12 @@ package cli import ( + "bytes" + "context" "fmt" + "io" "os" + "path/filepath" "strconv" "strings" "testing" @@ -14,6 +18,9 @@ import ( ) func TestSetupEnv(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + cases := []struct { args []string env map[string]string @@ -44,11 +51,10 @@ func TestSetupEnv(t *testing.T) { } demo.Flags().String("foobar", "", "Some test value from config") cmd := PrepareBaseCmd(demo, "DEMO", "/qwerty/asdfgh") // some missing dir.. - cmd.Exit = func(int) {} viper.Reset() args := append([]string{cmd.Use}, tc.args...) - err := RunWithArgs(cmd, args, tc.env) + err := RunWithArgs(ctx, cmd, args, tc.env) require.NoError(t, err, i) assert.Equal(t, tc.expected, foo, i) } @@ -61,12 +67,27 @@ func tempDir(t *testing.T) string { return cdir } +// writeConfigVals writes a toml file with the given values. +// It returns an error if writing was impossible. +func writeConfigVals(dir string, vals map[string]string) error { + lines := make([]string, 0, len(vals)) + for k, v := range vals { + lines = append(lines, fmt.Sprintf("%s = %q", k, v)) + } + data := strings.Join(lines, "\n") + cfile := filepath.Join(dir, "config.toml") + return os.WriteFile(cfile, []byte(data), 0600) +} + func TestSetupConfig(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + // we pre-create two config files we can refer to in the rest of // the test cases. cval1 := "fubble" conf1 := tempDir(t) - err := WriteConfigVals(conf1, map[string]string{"boo": cval1}) + err := writeConfigVals(conf1, map[string]string{"boo": cval1}) require.NoError(t, err) cases := []struct { @@ -103,11 +124,10 @@ func TestSetupConfig(t *testing.T) { boo.Flags().String("boo", "", "Some test value from config") boo.Flags().String("two-words", "", "Check out env handling -") cmd := PrepareBaseCmd(boo, "RD", "/qwerty/asdfgh") // some missing dir... - cmd.Exit = func(int) {} viper.Reset() args := append([]string{cmd.Use}, tc.args...) - err := RunWithArgs(cmd, args, tc.env) + err := RunWithArgs(ctx, cmd, args, tc.env) require.NoError(t, err, i) assert.Equal(t, tc.expected, foo, i) assert.Equal(t, tc.expectedTwo, two, i) @@ -121,15 +141,18 @@ type DemoConfig struct { } func TestSetupUnmarshal(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + // we pre-create two config files we can refer to in the rest of // the test cases. cval1, cval2 := "someone", "else" conf1 := tempDir(t) - err := WriteConfigVals(conf1, map[string]string{"name": cval1}) + err := writeConfigVals(conf1, map[string]string{"name": cval1}) require.NoError(t, err) // even with some ignored fields, should be no problem conf2 := tempDir(t) - err = WriteConfigVals(conf2, map[string]string{"name": cval2, "foo": "bar"}) + err = writeConfigVals(conf2, map[string]string{"name": cval2, "foo": "bar"}) require.NoError(t, err) // unused is not declared on a flag and remains from base @@ -182,17 +205,19 @@ func TestSetupUnmarshal(t *testing.T) { // from the default config here marsh.Flags().Int("age", base.Age, "Some test value from config") cmd := PrepareBaseCmd(marsh, "MR", "/qwerty/asdfgh") // some missing dir... - cmd.Exit = func(int) {} viper.Reset() args := append([]string{cmd.Use}, tc.args...) - err := RunWithArgs(cmd, args, tc.env) + err := RunWithArgs(ctx, cmd, args, tc.env) require.NoError(t, err, i) assert.Equal(t, tc.expected, cfg, i) } } func TestSetupTrace(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + cases := []struct { args []string env map[string]string @@ -215,18 +240,16 @@ func TestSetupTrace(t *testing.T) { }, } cmd := PrepareBaseCmd(trace, "DBG", "/qwerty/asdfgh") // some missing dir.. - cmd.Exit = func(int) {} viper.Reset() args := append([]string{cmd.Use}, tc.args...) - stdout, stderr, err := RunCaptureWithArgs(cmd, args, tc.env) + stdout, stderr, err := runCaptureWithArgs(ctx, cmd, args, tc.env) require.Error(t, err, i) require.Equal(t, "", stdout, i) require.NotEqual(t, "", stderr, i) msg := strings.Split(stderr, "\n") desired := fmt.Sprintf("ERROR: %s", tc.expected) - assert.Equal(t, desired, msg[0], i) - t.Log(msg) + assert.Equal(t, desired, msg[0], i, msg) if tc.long && assert.True(t, len(msg) > 2, i) { // the next line starts the stack trace... assert.Contains(t, stderr, "TestSetupTrace", i) @@ -234,3 +257,44 @@ func TestSetupTrace(t *testing.T) { } } } + +// runCaptureWithArgs executes the given command with the specified command +// line args and environmental variables set. It returns string fields +// representing output written to stdout and stderr, additionally any error +// from cmd.Execute() is also returned +func runCaptureWithArgs(ctx context.Context, cmd *cobra.Command, args []string, env map[string]string) (stdout, stderr string, err error) { + oldout, olderr := os.Stdout, os.Stderr // keep backup of the real stdout + rOut, wOut, _ := os.Pipe() + rErr, wErr, _ := os.Pipe() + os.Stdout, os.Stderr = wOut, wErr + defer func() { + os.Stdout, os.Stderr = oldout, olderr // restoring the real stdout + }() + + // copy the output in a separate goroutine so printing can't block indefinitely + copyStd := func(reader *os.File) *(chan string) { + stdC := make(chan string) + go func() { + var buf bytes.Buffer + // io.Copy will end when we call reader.Close() below + io.Copy(&buf, reader) //nolint:errcheck //ignore error + select { + case <-cmd.Context().Done(): + case stdC <- buf.String(): + } + }() + return &stdC + } + outC := copyStd(rOut) + errC := copyStd(rErr) + + // now run the command + err = RunWithArgs(ctx, cmd, args, env) + + // and grab the stdout to return + wOut.Close() + wErr.Close() + stdout = <-*outC + stderr = <-*errC + return stdout, stderr, err +}