diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 05106062..fe80903e 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -1,6 +1,9 @@ package root import ( + "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 +30,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 +119,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 +133,28 @@ 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, err := resolveOutputFormat(cmdPFlags, noPrompt, configuredFormat) + if err != nil { + return usage.NewUsageError(err.Error(), cmd) + } + _ = cmdPFlags.Set(constants.FlagOutputFormat, outputFormat) + if spaceNameOrId := viper.GetString(constants.ConfigSpace); spaceNameOrId != "" { clientFactory.SetSpaceNameOrId(spaceNameOrId) } + return nil } cmd.RunE = func(cmd *cobra.Command, args []string) error { @@ -150,3 +167,32 @@ 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. +func resolveOutputFormat(flags *pflag.FlagSet, noPrompt bool, configuredFormat 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) + + switch { + case explicit: // take the flag as given + case configuredFormat != "": + outputFormat = configuredFormat + case noPrompt: + outputFormat = constants.OutputFormatBasic + default: + outputFormat = constants.OutputFormatTable + } + + outputFormat = strings.ToLower(strings.TrimSpace(outputFormat)) + if !constants.IsValidOutputFormat(outputFormat) { + return "", fmt.Errorf("unsupported output format %s. Valid values are 'json', 'table', 'basic'. Defaults to table", outputFormat) + } + return outputFormat, nil +} diff --git a/pkg/cmd/root/root_test.go b/pkg/cmd/root/root_test.go new file mode 100644 index 00000000..64491182 --- /dev/null +++ b/pkg/cmd/root/root_test.go @@ -0,0 +1,102 @@ +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, err := resolveOutputFormat(flags, test.noPrompt, test.configuredFormat) + + assert.NoError(t, err) + 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 + configuredFormat string + }{ + {name: "from the flag", flag: "xml"}, + {name: "from the legacy flag", legacyFlag: "yaml"}, + {name: "from the config file", configuredFormat: "csv"}, + } + + 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, test.configuredFormat) + + assert.ErrorContains(t, err, "unsupported output format") + }) + } +} + +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..82e13529 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -1,5 +1,7 @@ package constants +import "strings" + const ( ExecutableName = "octopus" ) @@ -77,6 +79,17 @@ 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 + } +} + // 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