From b9171d01dcdbcc5657d6ab04fbc301c3af0c9630 Mon Sep 17 00:00:00 2001 From: Keepers <104464746+ryanfkeepers@users.noreply.github.com> Date: Fri, 29 Jul 2022 13:47:08 -0600 Subject: [PATCH] remove help menu output from non-cli errors (#438) Cobra's RunE causes every error response to display the CLI help menu for the current command, after printing the original error itself. This made error outputs difficult to read, and was generally unhelpful in most cases. The exit func in Print now prints the error and closes the CLI without showing the help menu. --- src/cli/backup/exchange.go | 27 +++++++++-------- src/cli/cli.go | 7 ++--- src/cli/print/print.go | 59 ++++++++++++++++++++++++++++++++++--- src/cli/print/print_test.go | 47 +++++++++++++++++++++++++++++ src/cli/repo/s3.go | 27 ++++++++--------- src/cli/restore/exchange.go | 17 +++++------ 6 files changed, 140 insertions(+), 44 deletions(-) create mode 100644 src/cli/print/print_test.go diff --git a/src/cli/backup/exchange.go b/src/cli/backup/exchange.go index e6fa23db1..bc83cae1f 100644 --- a/src/cli/backup/exchange.go +++ b/src/cli/backup/exchange.go @@ -8,6 +8,7 @@ import ( "github.com/alcionai/corso/cli/config" "github.com/alcionai/corso/cli/options" "github.com/alcionai/corso/cli/print" + . "github.com/alcionai/corso/cli/print" "github.com/alcionai/corso/cli/utils" "github.com/alcionai/corso/pkg/logger" "github.com/alcionai/corso/pkg/repository" @@ -128,12 +129,12 @@ func createExchangeCmd(cmd *cobra.Command, args []string) error { s, acct, err := config.GetStorageAndAccount(true, nil) if err != nil { - return err + return Only(err) } m365, err := acct.M365Config() if err != nil { - return errors.Wrap(err, "Failed to parse m365 account config") + return Only(errors.Wrap(err, "Failed to parse m365 account config")) } logger.Ctx(ctx).Debugw( @@ -152,12 +153,12 @@ func createExchangeCmd(cmd *cobra.Command, args []string) error { bo, err := r.NewBackup(ctx, sel, options.Control()) if err != nil { - return errors.Wrap(err, "Failed to initialize Exchange backup") + return Only(errors.Wrap(err, "Failed to initialize Exchange backup")) } err = bo.Run(ctx) if err != nil { - return errors.Wrap(err, "Failed to run Exchange backup") + return Only(errors.Wrap(err, "Failed to run Exchange backup")) } bu, err := r.Backup(ctx, bo.Results.BackupID) @@ -226,12 +227,12 @@ func listExchangeCmd(cmd *cobra.Command, args []string) error { s, acct, err := config.GetStorageAndAccount(true, nil) if err != nil { - return err + return Only(err) } m365, err := acct.M365Config() if err != nil { - return errors.Wrap(err, "Failed to parse m365 account config") + return Only(errors.Wrap(err, "Failed to parse m365 account config")) } logger.Ctx(ctx).Debugw( @@ -240,13 +241,13 @@ func listExchangeCmd(cmd *cobra.Command, args []string) error { r, err := repository.Connect(ctx, acct, s) if err != nil { - return errors.Wrapf(err, "Failed to connect to the %s repository", s.Provider) + return Only(errors.Wrapf(err, "Failed to connect to the %s repository", s.Provider)) } defer utils.CloseRepo(ctx, r) rps, err := r.Backups(ctx) if err != nil { - return errors.Wrap(err, "Failed to list backups in the repository") + return Only(errors.Wrap(err, "Failed to list backups in the repository")) } print.Backups(rps) @@ -288,12 +289,12 @@ func detailsExchangeCmd(cmd *cobra.Command, args []string) error { s, acct, err := config.GetStorageAndAccount(true, nil) if err != nil { - return err + return Only(err) } m365, err := acct.M365Config() if err != nil { - return errors.Wrap(err, "Failed to parse m365 account config") + return Only(errors.Wrap(err, "Failed to parse m365 account config")) } logger.Ctx(ctx).Debugw( @@ -302,13 +303,13 @@ func detailsExchangeCmd(cmd *cobra.Command, args []string) error { r, err := repository.Connect(ctx, acct, s) if err != nil { - return errors.Wrapf(err, "Failed to connect to the %s repository", s.Provider) + return Only(errors.Wrapf(err, "Failed to connect to the %s repository", s.Provider)) } defer utils.CloseRepo(ctx, r) d, _, err := r.BackupDetails(ctx, backupID) if err != nil { - return errors.Wrap(err, "Failed to get backup details in the repository") + return Only(errors.Wrap(err, "Failed to get backup details in the repository")) } sel := selectors.NewExchangeRestore() @@ -334,7 +335,7 @@ func detailsExchangeCmd(cmd *cobra.Command, args []string) error { ds := sel.Reduce(d) if len(ds.Entries) == 0 { - return errors.New("nothing to display: no items in the backup match the provided selectors") + return Only(errors.New("nothing to display: no items in the backup match the provided selectors")) } print.Entries(ds.Entries) diff --git a/src/cli/cli.go b/src/cli/cli.go index a7e43fcea..9649ccb34 100644 --- a/src/cli/cli.go +++ b/src/cli/cli.go @@ -2,7 +2,6 @@ package cli import ( "context" - "fmt" "os" "github.com/spf13/cobra" @@ -40,7 +39,7 @@ func initConfig() { cobra.CheckErr(err) if err := viper.ReadInConfig(); err == nil { - fmt.Println("Using config file:", viper.ConfigFileUsed()) + print.Info("Using config file:", viper.ConfigFileUsed()) } } @@ -48,7 +47,7 @@ func initConfig() { // Produces the same output as `corso --help`. func handleCorsoCmd(cmd *cobra.Command, args []string) error { if version { - fmt.Printf("Corso\nversion:\tpre-alpha\n") + print.Infof("Corso\nversion:\tpre-alpha\n") return nil } return cmd.Help() @@ -58,6 +57,7 @@ func handleCorsoCmd(cmd *cobra.Command, args []string) error { func Handle() { corsoCmd.Flags().BoolP("version", "v", version, "current version info") corsoCmd.PersistentFlags().StringVar(&cfgFile, "config-file", "", "config file (default is $HOME/.corso)") + print.SetRootCommand(corsoCmd) print.AddOutputFlag(corsoCmd) corsoCmd.CompletionOptions.DisableDefaultCmd = true @@ -72,7 +72,6 @@ func Handle() { }() if err := corsoCmd.ExecuteContext(ctx); err != nil { - fmt.Println(err) os.Exit(1) } } diff --git a/src/cli/print/print.go b/src/cli/print/print.go index 1c67a74dd..2dbf440de 100644 --- a/src/cli/print/print.go +++ b/src/cli/print/print.go @@ -3,7 +3,7 @@ package print import ( "encoding/json" "fmt" - "os" + "io" "github.com/alcionai/corso/pkg/backup" "github.com/alcionai/corso/pkg/backup/details" @@ -17,6 +17,12 @@ var ( outputAsJSONDebug bool ) +var rootCmd = &cobra.Command{} + +func SetRootCommand(root *cobra.Command) { + rootCmd = root +} + // adds the --output flag to the provided command. func AddOutputFlag(parent *cobra.Command) { fs := parent.PersistentFlags() @@ -25,6 +31,49 @@ func AddOutputFlag(parent *cobra.Command) { cobra.CheckErr(fs.MarkHidden("json-debug")) } +// --------------------------------------------------------------------------------------------------------- +// Helper funcs +// --------------------------------------------------------------------------------------------------------- + +// Only tells the CLI to only display this error, preventing the usage +// (ie, help) menu from displaying as well. +func Only(e error) error { + rootCmd.SilenceUsage = true + return e +} + +// Info prints the strings to cobra's error writer (stdErr by default) +// if s is nil, prints nothing. +func Info(s ...any) { + info(rootCmd.ErrOrStderr(), s...) +} + +// info is the testable core of Info() +func info(w io.Writer, s ...any) { + if len(s) == 0 { + return + } + fmt.Fprint(w, s...) +} + +// Info prints the formatted strings to cobra's error writer (stdErr by default) +// if t is empty, prints nothing. +func Infof(t string, s ...any) { + infof(rootCmd.ErrOrStderr(), t, s...) +} + +// infof is the testable core of Infof() +func infof(w io.Writer, t string, s ...any) { + if len(t) == 0 { + return + } + fmt.Fprintf(w, t, s...) +} + +// --------------------------------------------------------------------------------------------------------- +// Output control for backup list/details +// --------------------------------------------------------------------------------------------------------- + type Printable interface { // reduces the struct to a minimized format for easier human consumption MinimumPrintable() any @@ -98,7 +147,7 @@ func outputTable(ps []Printable) { t.Rows = append(t.Rows, p.Values()) } _ = t.WriteTable( - os.Stdout, + rootCmd.OutOrStdout(), &table.Config{ ShowIndex: false, Color: false, @@ -134,8 +183,10 @@ func outputJSONArr(ps []Printable, debug bool) { func printJSON(a any) { bs, err := json.Marshal(a) if err != nil { - fmt.Fprintf(os.Stderr, "error formatting results to json: %v\n", err) + fmt.Fprintf(rootCmd.OutOrStderr(), "error formatting results to json: %v\n", err) return } - fmt.Println(string(pretty.Pretty(bs))) + fmt.Fprintln( + rootCmd.OutOrStdout(), + string(pretty.Pretty(bs))) } diff --git a/src/cli/print/print_test.go b/src/cli/print/print_test.go new file mode 100644 index 000000000..20dd31d0c --- /dev/null +++ b/src/cli/print/print_test.go @@ -0,0 +1,47 @@ +package print + +import ( + "bytes" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/suite" +) + +type PrintUnitSuite struct { + suite.Suite +} + +func TestPrintUnitSuite(t *testing.T) { + suite.Run(t, new(PrintUnitSuite)) +} + +func (suite *PrintUnitSuite) TestOnly() { + t := suite.T() + c := &cobra.Command{} + oldRoot := rootCmd + defer SetRootCommand(oldRoot) + SetRootCommand(c) + assert.NoError(t, Only(nil)) + assert.True(t, c.SilenceUsage) +} + +func (suite *PrintUnitSuite) TestInfo() { + t := suite.T() + var b bytes.Buffer + msg := "I have seen the fnords!" + info(&b, msg) + assert.Contains(t, b.String(), msg) +} + +func (suite *PrintUnitSuite) TestInfof() { + t := suite.T() + var b bytes.Buffer + msg := "I have seen the fnords!" + msg2 := "smarf" + infof(&b, msg, msg2) + bs := b.String() + assert.Contains(t, bs, msg) + assert.Contains(t, bs, msg2) +} diff --git a/src/cli/repo/s3.go b/src/cli/repo/s3.go index 11fbb7d1c..38590ac57 100644 --- a/src/cli/repo/s3.go +++ b/src/cli/repo/s3.go @@ -1,13 +1,12 @@ package repo import ( - "fmt" - "github.com/pkg/errors" "github.com/spf13/cobra" "github.com/spf13/pflag" "github.com/alcionai/corso/cli/config" + . "github.com/alcionai/corso/cli/print" "github.com/alcionai/corso/cli/utils" "github.com/alcionai/corso/internal/kopia" "github.com/alcionai/corso/pkg/account" @@ -72,16 +71,16 @@ func initS3Cmd(cmd *cobra.Command, args []string) error { s, a, err := config.GetStorageAndAccount(false, s3Overrides()) if err != nil { - return err + return Only(err) } s3Cfg, err := s.S3Config() if err != nil { - return errors.Wrap(err, "Retrieving s3 configuration") + return Only(errors.Wrap(err, "Retrieving s3 configuration")) } m365, err := a.M365Config() if err != nil { - return errors.Wrap(err, "Failed to parse m365 account config") + return Only(errors.Wrap(err, "Failed to parse m365 account config")) } log.Debugw( @@ -97,14 +96,14 @@ func initS3Cmd(cmd *cobra.Command, args []string) error { if succeedIfExists && kopia.IsRepoAlreadyExistsError(err) { return nil } - return errors.Wrap(err, "Failed to initialize a new S3 repository") + return Only(errors.Wrap(err, "Failed to initialize a new S3 repository")) } defer utils.CloseRepo(ctx, r) - fmt.Printf("Initialized a S3 repository within bucket %s.\n", s3Cfg.Bucket) + Infof("Initialized a S3 repository within bucket %s.\n", s3Cfg.Bucket) if err = config.WriteRepoConfig(s3Cfg, m365); err != nil { - return errors.Wrap(err, "Failed to write repository configuration") + return Only(errors.Wrap(err, "Failed to write repository configuration")) } return nil } @@ -129,15 +128,15 @@ func connectS3Cmd(cmd *cobra.Command, args []string) error { s, a, err := config.GetStorageAndAccount(true, s3Overrides()) if err != nil { - return err + return Only(err) } s3Cfg, err := s.S3Config() if err != nil { - return errors.Wrap(err, "Retrieving s3 configuration") + return Only(errors.Wrap(err, "Retrieving s3 configuration")) } m365, err := a.M365Config() if err != nil { - return errors.Wrap(err, "Failed to parse m365 account config") + return Only(errors.Wrap(err, "Failed to parse m365 account config")) } log.Debugw( @@ -150,14 +149,14 @@ func connectS3Cmd(cmd *cobra.Command, args []string) error { r, err := repository.Connect(ctx, a, s) if err != nil { - return errors.Wrap(err, "Failed to connect to the S3 repository") + return Only(errors.Wrap(err, "Failed to connect to the S3 repository")) } defer utils.CloseRepo(ctx, r) - fmt.Printf("Connected to S3 bucket %s.\n", s3Cfg.Bucket) + Infof("Connected to S3 bucket %s.\n", s3Cfg.Bucket) if err = config.WriteRepoConfig(s3Cfg, m365); err != nil { - return errors.Wrap(err, "Failed to write repository configuration") + return Only(errors.Wrap(err, "Failed to write repository configuration")) } return nil } diff --git a/src/cli/restore/exchange.go b/src/cli/restore/exchange.go index 0ba3c4041..9fa7d338e 100644 --- a/src/cli/restore/exchange.go +++ b/src/cli/restore/exchange.go @@ -1,14 +1,13 @@ package restore import ( - "fmt" - "github.com/pkg/errors" "github.com/spf13/cobra" "github.com/spf13/pflag" "github.com/alcionai/corso/cli/config" "github.com/alcionai/corso/cli/options" + . "github.com/alcionai/corso/cli/print" "github.com/alcionai/corso/cli/utils" "github.com/alcionai/corso/pkg/logger" "github.com/alcionai/corso/pkg/repository" @@ -108,12 +107,12 @@ func restoreExchangeCmd(cmd *cobra.Command, args []string) error { s, a, err := config.GetStorageAndAccount(true, nil) if err != nil { - return err + return Only(err) } m365, err := a.M365Config() if err != nil { - return errors.Wrap(err, "Failed to parse m365 account config") + return Only(errors.Wrap(err, "Failed to parse m365 account config")) } logger.Ctx(ctx).Debugw( @@ -125,7 +124,7 @@ func restoreExchangeCmd(cmd *cobra.Command, args []string) error { r, err := repository.Connect(ctx, a, s) if err != nil { - return errors.Wrapf(err, "Failed to connect to the %s repository", s.Provider) + return Only(errors.Wrapf(err, "Failed to connect to the %s repository", s.Provider)) } defer utils.CloseRepo(ctx, r) @@ -152,14 +151,14 @@ func restoreExchangeCmd(cmd *cobra.Command, args []string) error { ro, err := r.NewRestore(ctx, backupID, sel.Selector, options.Control()) if err != nil { - return errors.Wrap(err, "Failed to initialize Exchange restore") + return Only(errors.Wrap(err, "Failed to initialize Exchange restore")) } if err := ro.Run(ctx); err != nil { - return errors.Wrap(err, "Failed to run Exchange restore") + return Only(errors.Wrap(err, "Failed to run Exchange restore")) } - fmt.Printf("Restored Exchange in %s for user %s.\n", s.Provider, user) + Infof("Restored Exchange in %s for user %s.\n", s.Provider, user) return nil } @@ -274,7 +273,7 @@ func validateExchangeRestoreFlags( return nil } if lu == 0 { - return errors.New("requires one or more --user ids, the wildcard --user *, or the --all flag.") + return errors.New("requires one or more --user ids, the wildcard --user *, or the --all flag") } if lc > 0 && lcf == 0 { return errors.New("one or more --contact-folder ids or the wildcard --contact-folder * must be included to specify a --contact")