diff --git a/pkg/cmd/config/set/set.go b/pkg/cmd/config/set/set.go index e27381af..c3067ba0 100644 --- a/pkg/cmd/config/set/set.go +++ b/pkg/cmd/config/set/set.go @@ -1,6 +1,7 @@ package set import ( + "errors" "fmt" "strconv" "strings" @@ -67,13 +68,21 @@ func setRun(isPromptEnabled bool, ask question.Asker, key string, value string) key = k } key = strings.ToLower(key) - if key == strings.ToLower(constants.ConfigNoPrompt) { + switch key { + case strings.ToLower(constants.ConfigNoPrompt): boolValue, err := strconv.ParseBool(value) if err != nil { return fmt.Errorf("the provided value %s is not valid for NoPrompt, please use true of false", value) } localViper.Set(key, boolValue) - } else { + case strings.ToLower(constants.ConfigOutputFormat): + // reject it here rather than let it sit in the config file poisoning every later command + value = strings.ToLower(strings.TrimSpace(value)) + if !constants.IsValidOutputFormat(value) { + return errors.New(constants.UnsupportedOutputFormatMessage(value)) + } + localViper.Set(key, value) + default: localViper.Set(key, value) } if err := localViper.WriteConfig(); err != nil { diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 05106062..69918f71 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -1,6 +1,10 @@ package root import ( + "errors" + "fmt" + "strings" + "github.com/OctopusDeploy/cli/pkg/apiclient" accountCmd "github.com/OctopusDeploy/cli/pkg/cmd/account" apiCmd "github.com/OctopusDeploy/cli/pkg/cmd/api" @@ -27,7 +31,9 @@ import ( "github.com/OctopusDeploy/cli/pkg/constants" "github.com/OctopusDeploy/cli/pkg/factory" "github.com/OctopusDeploy/cli/pkg/question" + "github.com/OctopusDeploy/cli/pkg/usage" "github.com/spf13/cobra" + "github.com/spf13/pflag" "github.com/spf13/viper" ) @@ -114,9 +120,9 @@ func NewCmdRoot(f factory.Factory, clientFactory apiclient.ClientFactory, askPro _ = viper.BindPFlag(constants.ConfigSpace, cmdPFlags.Lookup(constants.FlagSpace)) _ = viper.BindPFlag(constants.FlagEnableServiceMessages, cmdPFlags.Lookup(constants.FlagEnableServiceMessages)) // if we attempt to check the flags before Execute is called, cobra hasn't parsed anything yet, - // so we'll get bad values. PersistentPreRun is a convenient callback for setting up our + // so we'll get bad values. PersistentPreRunE is a convenient callback for setting up our // environment after parsing but before execution. - cmd.PersistentPreRun = func(_ *cobra.Command, _ []string) { + cmd.PersistentPreRunE = func(cmd *cobra.Command, _ []string) error { // map flag alias values for k, v := range flagAliases { for _, aliasName := range v { @@ -128,16 +134,33 @@ func NewCmdRoot(f factory.Factory, clientFactory apiclient.ClientFactory, askPro } } - if noPrompt := viper.GetBool(constants.ConfigNoPrompt); noPrompt { + noPrompt := viper.GetBool(constants.ConfigNoPrompt) + if noPrompt { askProvider.DisableInteractive() - if v, _ := cmdPFlags.GetString(constants.FlagOutputFormat); v == "" { - cmdPFlags.Set(constants.FlagOutputFormat, constants.OutputFormatBasic) - } } + // resolve the output format once, here, rather than leaving each command to work it + // out for itself; commands (and output.PrintResource / output.PrintArray) then just + // read the flag and can trust what they get. + configuredFormat := "" + if viper.InConfig(strings.ToLower(constants.ConfigOutputFormat)) { + configuredFormat = viper.GetString(constants.ConfigOutputFormat) + } + outputFormat, warning, err := resolveOutputFormat(cmdPFlags, noPrompt, configuredFormat) + if warning != "" { + cmd.PrintErrln(warning) + } + if err != nil { + return usage.NewUsageError(err.Error(), cmd) + } + // write through Value so the flag isn't marked as Changed; commands such as `task wait` + // read Changed() to mean "the user explicitly asked for a format" + _ = cmdPFlags.Lookup(constants.FlagOutputFormat).Value.Set(outputFormat) + if spaceNameOrId := viper.GetString(constants.ConfigSpace); spaceNameOrId != "" { clientFactory.SetSpaceNameOrId(spaceNameOrId) } + return nil } cmd.RunE = func(cmd *cobra.Command, args []string) error { @@ -150,3 +173,47 @@ func NewCmdRoot(f factory.Factory, clientFactory apiclient.ClientFactory, askPro return cmd } + +// resolveOutputFormat works out the output format a command should use, in precedence order: +// an explicit --output-format (or legacy --outputFormat) flag, then the OutputFormat config file +// setting, then basic when prompting is disabled, and finally table. +// +// Note the flag carries a non-empty default, so "did the caller ask for a format?" has to be +// answered with Changed() rather than by testing the value for emptiness. configuredFormat is +// the OutputFormat config file setting, or empty if the config file doesn't set one. +// +// An unusable value returns an error, except when it came from the config file, which we can +// only warn about; see below. +func resolveOutputFormat(flags *pflag.FlagSet, noPrompt bool, configuredFormat string) (string, string, error) { + // the legacy flag is copied onto the new one by value, which doesn't mark it as Changed + explicit := flags.Changed(constants.FlagOutputFormat) || flags.Changed(constants.FlagOutputFormatLegacy) + outputFormat, _ := flags.GetString(constants.FlagOutputFormat) + + // this runs for every command, so failing hard on a bad config file value would lock the + // user out of the whole CLI - `octopus config set OutputFormat table` included. Warn and + // carry on down the precedence chain instead, so the config is still fixable. + warning := "" + if configuredFormat != "" && !constants.IsValidOutputFormat(strings.TrimSpace(configuredFormat)) { + warning = fmt.Sprintf("Ignoring the %s config setting: %s", + constants.ConfigOutputFormat, constants.UnsupportedOutputFormatMessage(configuredFormat)) + configuredFormat = "" + } + + switch { + case explicit: // take the flag as given + case configuredFormat != "": + outputFormat = configuredFormat + // note noPrompt is bound to $CI as well as --no-prompt (see config.bindEnvironment), so + // this fires on essentially every CI pipeline, not just on an explicit --no-prompt + case noPrompt: + outputFormat = constants.OutputFormatBasic + default: + outputFormat = constants.OutputFormatTable + } + + outputFormat = strings.ToLower(strings.TrimSpace(outputFormat)) + if !constants.IsValidOutputFormat(outputFormat) { + return "", warning, errors.New(constants.UnsupportedOutputFormatMessage(outputFormat)) + } + return outputFormat, warning, nil +} diff --git a/pkg/cmd/root/root_test.go b/pkg/cmd/root/root_test.go new file mode 100644 index 00000000..d6b89e1f --- /dev/null +++ b/pkg/cmd/root/root_test.go @@ -0,0 +1,131 @@ +package root + +import ( + "testing" + + "github.com/OctopusDeploy/cli/pkg/constants" + "github.com/spf13/pflag" + "github.com/stretchr/testify/assert" +) + +// newOutputFormatFlags mirrors the way NewCmdRoot registers the output format flags, +// including the non-empty default which is what makes Changed() necessary. +func newOutputFormatFlags() *pflag.FlagSet { + flags := pflag.NewFlagSet("test", pflag.ContinueOnError) + flags.StringP(constants.FlagOutputFormat, "f", constants.OutputFormatTable, "") + flags.String(constants.FlagOutputFormatLegacy, "", "") + return flags +} + +func TestResolveOutputFormat(t *testing.T) { + tests := []struct { + name string + flag string // --output-format, empty means not supplied + legacyFlag string // --outputFormat, empty means not supplied + noPrompt bool + configuredFormat string + expected string + }{ + {name: "defaults to table", expected: constants.OutputFormatTable}, + {name: "explicit flag is honoured", flag: "json", expected: constants.OutputFormatJson}, + {name: "explicit flag is normalised", flag: " JSON ", expected: constants.OutputFormatJson}, + {name: "legacy flag is honoured", legacyFlag: "json", expected: constants.OutputFormatJson}, + {name: "config file setting is honoured", configuredFormat: "json", expected: constants.OutputFormatJson}, + {name: "flag beats config file", flag: "basic", configuredFormat: "json", expected: constants.OutputFormatBasic}, + {name: "legacy flag beats config file", legacyFlag: "basic", configuredFormat: "json", expected: constants.OutputFormatBasic}, + // the flag's non-empty default used to mask this, so --no-prompt never took effect + {name: "no-prompt falls back to basic", noPrompt: true, expected: constants.OutputFormatBasic}, + {name: "flag beats no-prompt", flag: "json", noPrompt: true, expected: constants.OutputFormatJson}, + {name: "explicitly requesting table beats no-prompt", flag: "table", noPrompt: true, expected: constants.OutputFormatTable}, + {name: "config file beats no-prompt", noPrompt: true, configuredFormat: "json", expected: constants.OutputFormatJson}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + flags := newOutputFormatFlags() + if test.flag != "" { + assert.NoError(t, flags.Set(constants.FlagOutputFormat, test.flag)) + } + if test.legacyFlag != "" { + assert.NoError(t, flags.Set(constants.FlagOutputFormatLegacy, test.legacyFlag)) + // NewCmdRoot copies the legacy value across without marking the new flag as Changed + assert.NoError(t, flags.Lookup(constants.FlagOutputFormat).Value.Set(test.legacyFlag)) + } + + actual, warning, err := resolveOutputFormat(flags, test.noPrompt, test.configuredFormat) + + assert.NoError(t, err) + assert.Empty(t, warning) + assert.Equal(t, test.expected, actual) + }) + } +} + +func TestResolveOutputFormat_RejectsUnsupportedFormats(t *testing.T) { + // commands that hand-roll their own format switch have no default case, so an unsupported + // format used to print nothing at all and exit 0 + tests := []struct { + name string + flag string + legacyFlag string + }{ + {name: "from the flag", flag: "xml"}, + {name: "from the legacy flag", legacyFlag: "yaml"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + flags := newOutputFormatFlags() + if test.flag != "" { + assert.NoError(t, flags.Set(constants.FlagOutputFormat, test.flag)) + } + if test.legacyFlag != "" { + assert.NoError(t, flags.Set(constants.FlagOutputFormatLegacy, test.legacyFlag)) + assert.NoError(t, flags.Lookup(constants.FlagOutputFormat).Value.Set(test.legacyFlag)) + } + + _, _, err := resolveOutputFormat(flags, false, "") + + assert.ErrorContains(t, err, "unsupported output format") + }) + } +} + +// an unsupported value in the config file must not be fatal: this runs ahead of every command, +// so failing hard would lock the user out of the `config set` that would fix it +func TestResolveOutputFormat_WarnsAndFallsBackForAnUnsupportedConfigFileValue(t *testing.T) { + flags := newOutputFormatFlags() + + actual, warning, err := resolveOutputFormat(flags, false, "csv") + + assert.NoError(t, err) + assert.Equal(t, constants.OutputFormatTable, actual) + assert.Contains(t, warning, "unsupported output format 'csv'") + assert.Contains(t, warning, constants.ConfigOutputFormat) +} + +func TestResolveOutputFormat_AnExplicitFlagStillWinsOverAnUnsupportedConfigFileValue(t *testing.T) { + flags := newOutputFormatFlags() + assert.NoError(t, flags.Set(constants.FlagOutputFormat, "json")) + + actual, warning, err := resolveOutputFormat(flags, false, "csv") + + assert.NoError(t, err) + assert.Equal(t, constants.OutputFormatJson, actual) + assert.NotEmpty(t, warning) +} + +func TestUnsupportedOutputFormatMessage(t *testing.T) { + assert.Equal(t, + "unsupported output format ''. Valid values are 'json', 'table', 'basic'", + constants.UnsupportedOutputFormatMessage("")) +} + +func TestIsValidOutputFormat(t *testing.T) { + assert.True(t, constants.IsValidOutputFormat(constants.OutputFormatJson)) + assert.True(t, constants.IsValidOutputFormat(constants.OutputFormatTable)) + assert.True(t, constants.IsValidOutputFormat(constants.OutputFormatBasic)) + assert.True(t, constants.IsValidOutputFormat("JSON"), "should be case-insensitive") + assert.False(t, constants.IsValidOutputFormat("")) + assert.False(t, constants.IsValidOutputFormat("xml")) +} diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index 39b2ccf0..13b60448 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -1,5 +1,10 @@ package constants +import ( + "fmt" + "strings" +) + const ( ExecutableName = "octopus" ) @@ -77,6 +82,24 @@ const ( PromptCreateNew = "" ) +// IsValidOutputFormat tells you whether outputFormat is one the CLI understands. +// The comparison is case-insensitive, matching the way commands render the format. +func IsValidOutputFormat(outputFormat string) bool { + switch strings.ToLower(outputFormat) { + case OutputFormatJson, OutputFormatTable, OutputFormatBasic: + return true + default: + return false + } +} + +// UnsupportedOutputFormatMessage is the message we give back when we're handed an output +// format we don't understand. It lives next to IsValidOutputFormat so the wording can't drift +// between the places that reject one. +func UnsupportedOutputFormatMessage(outputFormat string) string { + return fmt.Sprintf("unsupported output format '%s'. Valid values are 'json', 'table', 'basic'", outputFormat) +} + // IsProgrammaticOutputFormat tells you if it is acceptable for your command to // print miscellaneous output to stdout, such as progress messages. // If your command is capable of printing such things, you should check the output format diff --git a/pkg/output/print_array.go b/pkg/output/print_array.go index d7aa19c3..f0a8bd4a 100644 --- a/pkg/output/print_array.go +++ b/pkg/output/print_array.go @@ -3,7 +3,6 @@ package output import ( "encoding/json" "errors" - "fmt" "strings" "github.com/OctopusDeploy/cli/pkg/constants" @@ -64,9 +63,7 @@ func PrintArray[T any](items []T, cmd *cobra.Command, mappers Mappers[T]) error return t.Print() default: - return usage.NewUsageError( - fmt.Sprintf("unsupported output format %s. Valid values are 'json', 'table', 'basic'. Defaults to table", outputFormat), - cmd) + return usage.NewUsageError(constants.UnsupportedOutputFormatMessage(outputFormat), cmd) } return nil } diff --git a/pkg/output/print_resource.go b/pkg/output/print_resource.go index a58141a4..14462ca1 100644 --- a/pkg/output/print_resource.go +++ b/pkg/output/print_resource.go @@ -3,7 +3,6 @@ package output import ( "encoding/json" "errors" - "fmt" "strings" "github.com/OctopusDeploy/cli/pkg/constants" @@ -58,9 +57,7 @@ func PrintResource[T any](item T, cmd *cobra.Command, mappers Mappers[T]) error return t.Print() default: - return usage.NewUsageError( - fmt.Sprintf("unsupported output format %s. Valid values are 'json', 'table', 'basic'. Defaults to table", outputFormat), - cmd) + return usage.NewUsageError(constants.UnsupportedOutputFormatMessage(outputFormat), cmd) } return nil }