fix: resolve and validate the output format once, in the root command

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
+}

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"))
+}

pkg/constants/constants.go

@@ -1,5 +1,7 @@
 package constants
 
+import "strings"
+
 const (
 	ExecutableName = "octopus"
 )
@@ -77,6 +79,17 @@ const (
 	PromptCreateNew = "<Create New>"
 )
 
+// 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