feat: better error messages on malformed option value (#798)

Previously, if an option was configured with the wrong type (e.g.
`string` was configured with an `int` value) or other parsing errors
like malformed duration/boolean values occured, `Option[T].Get()` would
panic. It now returns an error that can properly be displayed to the
user and falls back to the default value.

go.mod

@@ -14,6 +14,7 @@ require (
 	github.com/guptarohit/asciigraph v0.7.1
 	github.com/hetznercloud/hcloud-go/v2 v2.10.2
 	github.com/jedib0t/go-pretty/v6 v6.5.9
+	github.com/spf13/cast v1.6.0
 	github.com/spf13/cobra v1.8.1
 	github.com/spf13/pflag v1.0.5
 	github.com/spf13/viper v1.19.0
@@ -46,7 +47,6 @@ require (
 	github.com/sagikazarmark/slog-shim v0.1.0 // indirect
 	github.com/sourcegraph/conc v0.3.0 // indirect
 	github.com/spf13/afero v1.11.0 // indirect
-	github.com/spf13/cast v1.6.0 // indirect
 	github.com/subosito/gotenv v1.6.0 // indirect
 	go.uber.org/atomic v1.9.0 // indirect
 	go.uber.org/multierr v1.9.0 // indirect

internal/cli/root.go

@@ -25,7 +25,11 @@ func NewRootCommand(s state.State) *cobra.Command {
 	cmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
 		var err error
 		out := os.Stdout
-		if quiet := config.OptionQuiet.Get(s.Config()); quiet {
+		quiet, err := config.OptionQuiet.Get(s.Config())
+		if err != nil {
+			return err
+		}
+		if quiet {
 			out, err = os.Open(os.DevNull)
 			if err != nil {
 				return err

internal/cmd/base/create.go

@@ -43,7 +43,10 @@ func (cc *CreateCmd) CobraCommand(s state.State) *cobra.Command {
 	cmd.RunE = func(cmd *cobra.Command, args []string) error {
 		outputFlags := output.FlagsForCommand(cmd)
 
-		quiet := config.OptionQuiet.Get(s.Config())
+		quiet, err := config.OptionQuiet.Get(s.Config())
+		if err != nil {
+			return err
+		}
 
 		isSchema := outputFlags.IsSet("json") || outputFlags.IsSet("yaml")
 		if isSchema && !quiet {

internal/cmd/config/get.go

@@ -44,7 +44,11 @@ func runGet(s state.State, cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("unknown key: %s", key)
 	}
 
-	val := opt.GetAsAny(s.Config())
+	val, err := opt.GetAsAny(s.Config())
+	if err != nil {
+		return err
+	}
+
 	if opt.HasFlags(config.OptionFlagSensitive) && !allowSensitive {
 		return fmt.Errorf("'%s' is sensitive. use --allow-sensitive to show the value", key)
 	}

internal/cmd/config/list.go

@@ -51,7 +51,11 @@ func runList(s state.State, cmd *cobra.Command, _ []string) error {
 
 	var options []option
 	for name, opt := range config.Options {
-		val := opt.GetAsAny(s.Config())
+		val, err := opt.GetAsAny(s.Config())
+		if err != nil {
+			return err
+		}
+
 		if opt.HasFlags(config.OptionFlagSensitive) && !allowSensitive {
 			val = "[redacted]"
 		}

internal/cmd/server/create.go

@@ -356,7 +356,10 @@ func createOptsFromFlags(
 	}
 
 	if !flags.Changed("ssh-key") && config.OptionDefaultSSHKeys.Changed(s.Config()) {
-		sshKeys = config.OptionDefaultSSHKeys.Get(s.Config())
+		sshKeys, err = config.OptionDefaultSSHKeys.Get(s.Config())
+		if err != nil {
+			return
+		}
 	}
 
 	for _, sshKeyIDOrName := range sshKeys {

internal/cmd/util/util.go

@@ -13,6 +13,7 @@ import (
 	"time"
 
 	"github.com/goccy/go-yaml"
+	"github.com/spf13/cast"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 )
@@ -302,3 +303,27 @@ func ParseBoolLenient(s string) (bool, error) {
 		return false, fmt.Errorf("invalid boolean value: %s", s)
 	}
 }
+
+// ToBoolE converts the provided value to a bool. It is more lenient than [cast.ToBoolE] (see [ParseBoolLenient]).
+func ToBoolE(val any) (bool, error) {
+	switch v := val.(type) {
+	case bool:
+		return v, nil
+	case string:
+		return ParseBoolLenient(v)
+	default:
+		return cast.ToBoolE(val)
+	}
+}
+
+// ToStringSliceDelimited is like [AnyToStringSlice] but also accepts a string that is comma-separated.
+func ToStringSliceDelimited(val any) []string {
+	switch v := val.(type) {
+	case []string:
+		return v
+	case string:
+		return strings.Split(v, ",")
+	default:
+		return AnyToStringSlice(val)
+	}
+}

internal/cmd/util/util_test.go

@@ -380,3 +380,36 @@ func TestParseBoolLenient(t *testing.T) {
 	b, err = util.ParseBoolLenient("")
 	assert.EqualError(t, err, "invalid boolean value: ")
 }
+
+func TestBoolFromAny(t *testing.T) {
+	b, err := util.ToBoolE(true)
+	assert.NoError(t, err)
+	assert.True(t, b)
+	b, err = util.ToBoolE("true")
+	assert.NoError(t, err)
+	assert.True(t, b)
+	b, err = util.ToBoolE("false")
+	assert.NoError(t, err)
+	assert.False(t, b)
+	b, err = util.ToBoolE("yes")
+	assert.NoError(t, err)
+	assert.True(t, b)
+	b, err = util.ToBoolE("no")
+	assert.NoError(t, err)
+	assert.False(t, b)
+	b, err = util.ToBoolE(1)
+	assert.NoError(t, err)
+	assert.True(t, b)
+	b, err = util.ToBoolE(0)
+	assert.NoError(t, err)
+	assert.False(t, b)
+	_, err = util.ToBoolE("invalid")
+	assert.EqualError(t, err, "invalid boolean value: invalid")
+}
+
+func TestToStringSliceDelimited(t *testing.T) {
+	assert.Equal(t, []string{"a", "b", "c"}, util.ToStringSliceDelimited([]string{"a", "b", "c"}))
+	assert.Equal(t, []string{"a", "b", "c"}, util.ToStringSliceDelimited([]any{"a", "b", "c"}))
+	assert.Equal(t, []string{"a", "b", "c"}, util.ToStringSliceDelimited("a,b,c"))
+	assert.Equal(t, []string{"0", "1", "2"}, util.ToStringSliceDelimited([]int{0, 1, 2}))
+}

internal/state/config/config.go

@@ -107,7 +107,11 @@ func (cfg *config) Read(f any) error {
 		err      error
 	)
 
-	cfg.path = OptionConfig.Get(cfg)
+	cfg.path, err = OptionConfig.Get(cfg)
+	if err != nil {
+		return err
+	}
+
 	cfgPath, ok := f.(string)
 	if cfgPath != "" && ok {
 		cfg.path = cfgPath
@@ -149,7 +153,10 @@ func (cfg *config) Read(f any) error {
 	}
 
 	// read active context from viper
-	activeContext := OptionContext.Get(cfg)
+	activeContext, err := OptionContext.Get(cfg)
+	if err != nil {
+		return err
+	}
 
 	cfg.contexts = cfg.schema.Contexts
 	for i, ctx := range cfg.contexts {

internal/state/config/options.go

@@ -3,10 +3,10 @@ package config
 import (
 	"fmt"
 	"slices"
-	"strconv"
 	"strings"
 	"time"
 
+	"github.com/spf13/cast"
 	"github.com/spf13/pflag"
 
 	"github.com/hetznercloud/cli/internal/cmd/util"
@@ -48,7 +48,7 @@ type IOption interface {
 	// HasFlags returns true if the option has all the provided flags set
 	HasFlags(src OptionFlag) bool
 	// GetAsAny reads the option value from the config and returns it as an any
-	GetAsAny(c Config) any
+	GetAsAny(c Config) (any, error)
 	// OverrideAny sets the option value in the config to the provided any value
 	OverrideAny(c Config, v any)
 	// Changed returns true if the option has been changed from the default
@@ -152,41 +152,46 @@ type Option[T any] struct {
 	overrides   *overrides
 }
 
-func (o *Option[T]) Get(c Config) T {
+func (o *Option[T]) Get(c Config) (T, error) {
+	// val is the option value that we obtain from viper.
+	// Since viper uses multiple configuration sources (env, config, etc.) we need
+	// to be able to convert the value to the desired type.
 	val := c.Viper().Get(o.Name)
-	if val == nil {
-		return o.Default
+	if util.IsNil(val) {
+		return o.Default, nil
 	}
+
+	// t is a dummy variable to get the desired type of the option
 	var t T
+
 	switch any(t).(type) {
 	case time.Duration:
-		if v, ok := val.(string); ok {
-			d, err := time.ParseDuration(v)
-			if err != nil {
-				panic(err)
-			}
-			val = d
-		}
-		if v, ok := val.(int64); ok {
-			val = time.Duration(v)
+		// we can use the cast package included with viper here
+		d, err := cast.ToDurationE(val)
+		if err != nil {
+			return o.Default, fmt.Errorf("%s: %s", o.Name, err)
 		}
+		val = d
+
 	case bool:
-		if v, ok := val.(string); ok {
-			b, err := strconv.ParseBool(v)
-			if err != nil {
-				panic(err)
-			}
-			val = b
+		b, err := util.ToBoolE(val)
+		if err != nil {
+			return o.Default, fmt.Errorf("%s: %s", o.Name, err)
 		}
+		val = b
+
 	case []string:
-		if v, ok := val.([]any); ok {
-			val = util.ToStringSlice(v)
-		}
+		val = util.ToStringSliceDelimited(val)
+
+	case string:
+		val = fmt.Sprint(val)
 	}
-	return val.(T)
+
+	// now that val has the desired dynamic type, we can safely cast the static type to T
+	return val.(T), nil
 }
 
-func (o *Option[T]) GetAsAny(c Config) any {
+func (o *Option[T]) GetAsAny(c Config) (any, error) {
 	return o.Get(c)
 }
 
@@ -283,6 +288,7 @@ func (o *Option[T]) Parse(values []string) (any, error) {
 		if err != nil {
 			return nil, fmt.Errorf("invalid duration value: %s", value)
 		}
+
 	case []string:
 		newVal := values[:]
 		slices.Sort(newVal)

internal/state/helpers.go

@@ -15,7 +15,10 @@ func Wrap(s State, f func(State, *cobra.Command, []string) error) func(*cobra.Co
 }
 
 func (c *state) EnsureToken(_ *cobra.Command, _ []string) error {
-	token := config.OptionToken.Get(c.config)
+	token, err := config.OptionToken.Get(c.config)
+	if err != nil {
+		return err
+	}
 	if token == "" {
 		return errors.New("no active context or token (see `hcloud context --help`)")
 	}

internal/state/state.go

@@ -37,7 +37,11 @@ func New(cfg config.Config) (State, error) {
 		term:    terminal.DefaultTerminal{},
 	}
 
-	s.client = s.newClient()
+	var err error
+	s.client, err = s.newClient()
+	if err != nil {
+		return nil, err
+	}
 	return s, nil
 }
 
@@ -53,27 +57,50 @@ func (c *state) Terminal() terminal.Terminal {
 	return c.term
 }
 
-func (c *state) newClient() hcapi2.Client {
+func (c *state) newClient() (hcapi2.Client, error) {
+	tok, err := config.OptionToken.Get(c.config)
+	if err != nil {
+		return nil, err
+	}
+
 	opts := []hcloud.ClientOption{
-		hcloud.WithToken(config.OptionToken.Get(c.config)),
+		hcloud.WithToken(tok),
 		hcloud.WithApplication("hcloud-cli", version.Version),
 	}
 
-	if ep := config.OptionEndpoint.Get(c.config); ep != "" {
+	if ep, err := config.OptionEndpoint.Get(c.config); err == nil && ep != "" {
 		opts = append(opts, hcloud.WithEndpoint(ep))
+	} else if err != nil {
+		return nil, err
 	}
-	if config.OptionDebug.Get(c.config) {
-		if filePath := config.OptionDebugFile.Get(c.config); filePath == "" {
+
+	debug, err := config.OptionDebug.Get(c.config)
+	if err != nil {
+		return nil, err
+	}
+
+	if debug {
+		filePath, err := config.OptionDebugFile.Get(c.config)
+		if err != nil {
+			return nil, err
+		}
+
+		if filePath == "" {
 			opts = append(opts, hcloud.WithDebugWriter(os.Stderr))
 		} else {
 			writer, _ := os.Create(filePath)
 			opts = append(opts, hcloud.WithDebugWriter(writer))
 		}
 	}
-	pollInterval := config.OptionPollInterval.Get(c.config)
+
+	pollInterval, err := config.OptionPollInterval.Get(c.config)
+	if err != nil {
+		return nil, err
+	}
+
 	if pollInterval > 0 {
 		opts = append(opts, hcloud.WithBackoffFunc(hcloud.ConstantBackoff(pollInterval)))
 	}
 
-	return hcapi2.NewClient(opts...)
+	return hcapi2.NewClient(opts...), nil
 }