diff --git a/cli/cliflag/cliflag.go b/cli/cliflag/cliflag.go index ddb808b910..053ea948f0 100644 --- a/cli/cliflag/cliflag.go +++ b/cli/cliflag/cliflag.go @@ -17,9 +17,33 @@ import ( "strings" "time" + "github.com/spf13/cobra" "github.com/spf13/pflag" ) +// IsSetBool returns the value of the boolean flag if it is set. +// It returns false if the flag isn't set or if any error occurs attempting +// to parse the value of the flag. +func IsSetBool(cmd *cobra.Command, name string) bool { + val, ok := IsSet(cmd, name) + if !ok { + return false + } + + b, err := strconv.ParseBool(val) + return err == nil && b +} + +// IsSet returns the string value of the flag and whether it was set. +func IsSet(cmd *cobra.Command, name string) (string, bool) { + flag := cmd.Flag(name) + if flag == nil { + return "", false + } + + return flag.Value.String(), flag.Changed +} + // String sets a string flag on the given flag set. func String(flagset *pflag.FlagSet, name, shorthand, env, def, usage string) { v, ok := os.LookupEnv(env) @@ -67,6 +91,22 @@ func Uint8VarP(flagset *pflag.FlagSet, ptr *uint8, name string, shorthand string flagset.Uint8VarP(ptr, name, shorthand, uint8(vi64), fmtUsage(usage, env)) } +func Bool(flagset *pflag.FlagSet, name, shorthand, env string, def bool, usage string) { + val, ok := os.LookupEnv(env) + if !ok || val == "" { + flagset.BoolP(name, shorthand, def, fmtUsage(usage, env)) + return + } + + valb, err := strconv.ParseBool(val) + if err != nil { + flagset.BoolP(name, shorthand, def, fmtUsage(usage, env)) + return + } + + flagset.BoolP(name, shorthand, valb, fmtUsage(usage, env)) +} + // BoolVarP sets a bool flag on the given flag set. func BoolVarP(flagset *pflag.FlagSet, ptr *bool, name string, shorthand string, env string, def bool, usage string) { val, ok := os.LookupEnv(env) diff --git a/cli/login.go b/cli/login.go index 9f26a2e30e..9abc0b48d9 100644 --- a/cli/login.go +++ b/cli/login.go @@ -73,7 +73,9 @@ func login() *cobra.Command { // on a very old client. err = checkVersions(cmd, client) if err != nil { - return xerrors.Errorf("check versions: %w", err) + // Checking versions isn't a fatal error so we print a warning + // and proceed. + _, _ = fmt.Fprintln(cmd.ErrOrStderr(), cliui.Styles.Warn.Render(err.Error())) } hasInitialUser, err := client.HasFirstUser(cmd.Context()) diff --git a/cli/root.go b/cli/root.go index a7c1b90ad7..862f290d66 100644 --- a/cli/root.go +++ b/cli/root.go @@ -4,7 +4,6 @@ import ( "fmt" "net/url" "os" - "strconv" "strings" "text/template" "time" @@ -40,11 +39,12 @@ const ( varAgentURL = "agent-url" varGlobalConfig = "global-config" varNoOpen = "no-open" + varNoVersionCheck = "no-version-warning" varForceTty = "force-tty" + varVerbose = "verbose" notLoggedInMessage = "You are not logged in. Try logging in using 'coder login '." - noVersionCheckFlag = "no-version-warning" - envNoVersionCheck = "CODER_NO_VERSION_WARNING" + envNoVersionCheck = "CODER_NO_VERSION_WARNING" ) var ( @@ -58,8 +58,6 @@ func init() { } func Root() *cobra.Command { - var varSuppressVersion bool - cmd := &cobra.Command{ Use: "coder", SilenceErrors: true, @@ -68,7 +66,7 @@ func Root() *cobra.Command { `, PersistentPreRun: func(cmd *cobra.Command, args []string) { err := func() error { - if varSuppressVersion { + if cliflag.IsSetBool(cmd, varNoVersionCheck) { return nil } @@ -141,7 +139,7 @@ func Root() *cobra.Command { cmd.SetUsageTemplate(usageTemplate()) cmd.PersistentFlags().String(varURL, "", "Specify the URL to your deployment.") - cliflag.BoolVarP(cmd.PersistentFlags(), &varSuppressVersion, noVersionCheckFlag, "", envNoVersionCheck, false, "Suppress warning when client and server versions do not match.") + cliflag.Bool(cmd.PersistentFlags(), varNoVersionCheck, "", envNoVersionCheck, false, "Suppress warning when client and server versions do not match.") cliflag.String(cmd.PersistentFlags(), varToken, "", envSessionToken, "", fmt.Sprintf("Specify an authentication token. For security reasons setting %s is preferred.", envSessionToken)) cliflag.String(cmd.PersistentFlags(), varAgentToken, "", "CODER_AGENT_TOKEN", "", "Specify an agent authentication token.") _ = cmd.PersistentFlags().MarkHidden(varAgentToken) @@ -152,6 +150,7 @@ func Root() *cobra.Command { _ = cmd.PersistentFlags().MarkHidden(varForceTty) cmd.PersistentFlags().Bool(varNoOpen, false, "Block automatically opening URLs in the browser.") _ = cmd.PersistentFlags().MarkHidden(varNoOpen) + cliflag.Bool(cmd.PersistentFlags(), varVerbose, "v", "CODER_VERBOSE", false, "Enable verbose output") return cmd } @@ -427,12 +426,29 @@ func formatExamples(examples ...example) string { // FormatCobraError colorizes and adds "--help" docs to cobra commands. func FormatCobraError(err error, cmd *cobra.Command) string { helpErrMsg := fmt.Sprintf("Run '%s --help' for usage.", cmd.CommandPath()) - return cliui.Styles.Error.Render(err.Error() + "\n" + helpErrMsg) + + var ( + httpErr *codersdk.Error + output strings.Builder + ) + + if xerrors.As(err, &httpErr) { + _, _ = fmt.Fprintln(&output, httpErr.Friendly()) + } + + // If the httpErr is nil then we just have a regular error in which + // case we want to print out what's happening. + if httpErr == nil || cliflag.IsSetBool(cmd, varVerbose) { + _, _ = fmt.Fprintln(&output, err.Error()) + } + + _, _ = fmt.Fprint(&output, helpErrMsg) + + return cliui.Styles.Error.Render(output.String()) } func checkVersions(cmd *cobra.Command, client *codersdk.Client) error { - flag := cmd.Flag("no-version-warning") - if suppress, _ := strconv.ParseBool(flag.Value.String()); suppress { + if cliflag.IsSetBool(cmd, varNoVersionCheck) { return nil } diff --git a/cli/root_test.go b/cli/root_test.go index f920401c96..5dc97bb060 100644 --- a/cli/root_test.go +++ b/cli/root_test.go @@ -4,22 +4,115 @@ import ( "bytes" "testing" + "github.com/spf13/cobra" "github.com/stretchr/testify/require" + "golang.org/x/xerrors" "github.com/coder/coder/buildinfo" "github.com/coder/coder/cli" "github.com/coder/coder/cli/clitest" + "github.com/coder/coder/codersdk" ) func TestRoot(t *testing.T) { t.Run("FormatCobraError", func(t *testing.T) { t.Parallel() - cmd, _ := clitest.New(t, "delete") + t.Run("OK", func(t *testing.T) { + t.Parallel() - cmd, err := cmd.ExecuteC() - errStr := cli.FormatCobraError(err, cmd) - require.Contains(t, errStr, "Run 'coder delete --help' for usage.") + cmd, _ := clitest.New(t, "delete") + + cmd, err := cmd.ExecuteC() + errStr := cli.FormatCobraError(err, cmd) + require.Contains(t, errStr, "Run 'coder delete --help' for usage.") + }) + + t.Run("Verbose", func(t *testing.T) { + t.Parallel() + + // Test that the verbose error is masked without verbose flag. + t.Run("NoVerboseAPIError", func(t *testing.T) { + t.Parallel() + + cmd, _ := clitest.New(t) + + cmd.RunE = func(cmd *cobra.Command, args []string) error { + var err error = &codersdk.Error{ + Response: codersdk.Response{ + Message: "This is a message.", + }, + Helper: "Try this instead.", + } + + err = xerrors.Errorf("wrap me: %w", err) + + return err + } + + cmd, err := cmd.ExecuteC() + errStr := cli.FormatCobraError(err, cmd) + require.Contains(t, errStr, "This is a message. Try this instead.") + require.NotContains(t, errStr, err.Error()) + }) + + // Assert that a regular error is not masked when verbose is not + // specified. + t.Run("NoVerboseRegularError", func(t *testing.T) { + t.Parallel() + + cmd, _ := clitest.New(t) + + cmd.RunE = func(cmd *cobra.Command, args []string) error { + return xerrors.Errorf("this is a non-codersdk error: %w", xerrors.Errorf("a wrapped error")) + } + + cmd, err := cmd.ExecuteC() + errStr := cli.FormatCobraError(err, cmd) + require.Contains(t, errStr, err.Error()) + }) + + // Test that both the friendly error and the verbose error are + // displayed when verbose is passed. + t.Run("APIError", func(t *testing.T) { + t.Parallel() + + cmd, _ := clitest.New(t, "--verbose") + + cmd.RunE = func(cmd *cobra.Command, args []string) error { + var err error = &codersdk.Error{ + Response: codersdk.Response{ + Message: "This is a message.", + }, + Helper: "Try this instead.", + } + + err = xerrors.Errorf("wrap me: %w", err) + + return err + } + + cmd, err := cmd.ExecuteC() + errStr := cli.FormatCobraError(err, cmd) + require.Contains(t, errStr, "This is a message. Try this instead.") + require.Contains(t, errStr, err.Error()) + }) + + // Assert that a regular error is not masked when verbose specified. + t.Run("RegularError", func(t *testing.T) { + t.Parallel() + + cmd, _ := clitest.New(t, "--verbose") + + cmd.RunE = func(cmd *cobra.Command, args []string) error { + return xerrors.Errorf("this is a non-codersdk error: %w", xerrors.Errorf("a wrapped error")) + } + + cmd, err := cmd.ExecuteC() + errStr := cli.FormatCobraError(err, cmd) + require.Contains(t, errStr, err.Error()) + }) + }) }) t.Run("Version", func(t *testing.T) { diff --git a/codersdk/client.go b/codersdk/client.go index 06185bdbd7..1279d47705 100644 --- a/codersdk/client.go +++ b/codersdk/client.go @@ -185,6 +185,10 @@ func (e *Error) StatusCode() int { return e.statusCode } +func (e *Error) Friendly() string { + return fmt.Sprintf("%s. %s", strings.TrimSuffix(e.Message, "."), e.Helper) +} + func (e *Error) Error() string { var builder strings.Builder if e.method != "" && e.url != "" {