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

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 {

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

pkg/cmd/root/root_test.go

@@ -0,0 +1,175 @@
+package root
+
+import (
+	"bytes"
+	"strings"
+	"testing"
+
+	"github.com/OctopusDeploy/cli/pkg/constants"
+	"github.com/spf13/pflag"
+	"github.com/spf13/viper"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// 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)
+}
+
+// the resolved format is written back through the flag's Value rather than FlagSet.Set,
+// because Set marks the flag Changed and the flag object is shared with every subcommand.
+// pkg/cmd/task/wait reads Changed(FlagOutputFormat) to mean "the user asked for a format",
+// so marking it here would silently switch a plain `octopus task wait <id>` off the legacy
+// progress formatter.
+func TestNewCmdRoot_PreRunDoesNotMarkTheOutputFormatFlagAsChanged(t *testing.T) {
+	viper.Reset()
+	t.Cleanup(viper.Reset)
+	cmd := NewCmdRoot(nil, nil, nil)
+
+	require.NoError(t, cmd.PersistentPreRunE(cmd, nil))
+
+	flags := cmd.PersistentFlags()
+	value, err := flags.GetString(constants.FlagOutputFormat)
+	require.NoError(t, err)
+	assert.Equal(t, constants.OutputFormatTable, value)
+	assert.False(t, flags.Changed(constants.FlagOutputFormat))
+	assert.False(t, flags.Changed(constants.FlagOutputFormatLegacy))
+}
+
+// the pre-run runs ahead of every command, so an unsupported value sitting in the config file
+// must not be fatal - that would lock the user out of the `config set` that fixes it
+func TestNewCmdRoot_PreRunWarnsRatherThanFailingForAnUnsupportedConfigFileValue(t *testing.T) {
+	viper.Reset()
+	t.Cleanup(viper.Reset)
+	cmd := NewCmdRoot(nil, nil, nil)
+	viper.SetConfigType("json")
+	require.NoError(t, viper.ReadConfig(strings.NewReader(`{"outputformat":"xml"}`)))
+	stderr := &bytes.Buffer{}
+	cmd.SetErr(stderr)
+
+	require.NoError(t, cmd.PersistentPreRunE(cmd, nil))
+
+	value, err := cmd.PersistentFlags().GetString(constants.FlagOutputFormat)
+	require.NoError(t, err)
+	assert.Equal(t, constants.OutputFormatTable, value)
+	// the warning goes to stderr so it can't corrupt `-f json` output on stdout
+	assert.Contains(t, stderr.String(), "unsupported output format 'xml'")
+}
+
+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"))
+}

pkg/constants/constants.go

@@ -1,5 +1,10 @@
 package constants
 
+import (
+	"fmt"
+	"strings"
+)
+
 const (
 	ExecutableName = "octopus"
 )
@@ -77,6 +82,24 @@ 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
+	}
+}
+
+// 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

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
 }