Skip to content

Commit 82177a3

Browse files
nieblaranieblara
committed
addressing pr feedback
1 parent 7a8c18c commit 82177a3

5 files changed

Lines changed: 92 additions & 56 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
### ⚠ BREAKING CHANGES
66

7-
* When stdout is not a TTY, the default `--output` format is now **json** instead of plaintext. Scripts that assumed plaintext when output was piped or redirected should set `LD_OUTPUT=plaintext`, run `ldcli config --set output plaintext`, or pass `--output plaintext` (or `--output json` explicitly if you want JSON regardless of TTY).
7+
* When stdout is not a TTY, the default `--output` format is now **json** instead of plaintext. Scripts that assumed plaintext when output was piped or redirected should set `LD_OUTPUT=plaintext`, run `ldcli config --set output plaintext`, or pass `--output plaintext` (or `--output json` explicitly if you want JSON regardless of TTY). You can also set **`FORCE_TTY`** or **`LD_FORCE_TTY`** to any non-empty value to keep plaintext as the default when stdout is not a TTY, without changing the saved `output` setting.
88

99
## [2.2.0](https://github.com/launchdarkly/ldcli/compare/v2.1.0...v2.2.0) (2026-02-20)
1010

‎README.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,9 @@ When you do not pass `--output` or `--json`, the default format depends on wheth
9696

9797
To force the plaintext default even when stdout is not a TTY, set either **`FORCE_TTY`** or **`LD_FORCE_TTY`** to any non-empty value (similar to tools that use `NO_COLOR`). That only affects the default; explicit `--output`, `--json`, `LD_OUTPUT`, and the `output` setting in your config file still apply.
9898

99-
Effective output is resolved in this order: **`--json`** and **`--output`** flags, then **`LD_OUTPUT`**, then the **`output`** value from your config file, then the TTY-based default above.
99+
**`LD_OUTPUT`** is the same setting as `output` in the config file, exposed as an environment variable (see the `LD_` prefix above). It is not new with TTY detection; the test suite locks in that it overrides the non-TTY JSON default when set to `plaintext`.
100+
101+
Effective output is resolved in this order: **`--json`** (if set, wins over `--output` when both are present), then **`--output`**, then **`LD_OUTPUT`**, then the **`output`** value from your config file, then the TTY-based default above.
100102

101103
## Commands
102104

‎cmd/cmdtest.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ func CallCmd(
3535
"test",
3636
false,
3737
func() bool { return true },
38+
nil,
3839
)
3940
cmd := rootCmd.Cmd()
4041
require.NoError(t, err)

‎cmd/root.go‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -85,23 +85,37 @@ func init() {
8585
// forceTTYDefaultOutput is true when FORCE_TTY or LD_FORCE_TTY is non-empty, so the default
8686
// --output is plaintext even if stdout is not a TTY (similar to NO_COLOR). Explicit --output,
8787
// --json, LD_OUTPUT, and config file values still take precedence via Viper/Cobra after parse.
88-
func forceTTYDefaultOutput() bool {
89-
return os.Getenv("FORCE_TTY") != "" || os.Getenv("LD_FORCE_TTY") != ""
88+
//
89+
// getenv, if non-nil, is used only for those two keys (tests can inject without mutating
90+
// process environment). If nil, os.Getenv is used.
91+
func forceTTYDefaultOutput(getenv func(string) string) bool {
92+
lookup := getenv
93+
if lookup == nil {
94+
lookup = os.Getenv
95+
}
96+
return lookup("FORCE_TTY") != "" || lookup("LD_FORCE_TTY") != ""
9097
}
9198

9299
// NewRootCommand constructs the ldcli root command tree.
93100
//
94-
// isTerminal should reflect whether stdout is a TTY (see Execute). For nil or a function that
95-
// returns false, the default --output is json unless forceTTYDefaultOutput applies—intended for
96-
// tests and embeddings; production should always pass a non-nil detector.
101+
// isTerminal must be non-nil; it should reflect whether stdout is a TTY (see Execute). When it
102+
// returns false, the default --output is json unless forceTTYDefaultOutput applies.
103+
//
104+
// getenv is optional: when non-nil, it is used to read FORCE_TTY and LD_FORCE_TTY only; when
105+
// nil, os.Getenv is used. Viper still reads LD_OUTPUT and other LD_ vars from the real
106+
// environment.
97107
func NewRootCommand(
98108
configService config.Service,
99109
analyticsTrackerFn analytics.TrackerFn,
100110
clients APIClients,
101111
version string,
102112
useConfigFile bool,
103113
isTerminal func() bool,
114+
getenv func(string) string,
104115
) (*RootCmd, error) {
116+
if isTerminal == nil {
117+
return nil, errors.New("NewRootCommand: isTerminal must not be nil")
118+
}
105119
cmd := &cobra.Command{
106120
Use: "ldcli",
107121
Short: "LaunchDarkly CLI",
@@ -205,7 +219,7 @@ func NewRootCommand(
205219
// When stdout is not a TTY (e.g. piped, CI, agent), default to JSON unless FORCE_TTY or
206220
// LD_FORCE_TTY is set (any non-empty value), like NO_COLOR.
207221
defaultOutput := "plaintext"
208-
if !forceTTYDefaultOutput() && (isTerminal == nil || !isTerminal()) {
222+
if !forceTTYDefaultOutput(getenv) && !isTerminal() {
209223
defaultOutput = "json"
210224
}
211225

@@ -274,6 +288,7 @@ func Execute(version string) {
274288
version,
275289
true,
276290
func() bool { return term.IsTerminal(int(os.Stdout.Fd())) },
291+
nil,
277292
)
278293
if err != nil {
279294
log.Fatal(err)

‎cmd/root_test.go‎

Lines changed: 66 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,8 @@ func TestOutputFlags(t *testing.T) {
108108
})
109109
}
110110

111-
func newRootCmdWithTerminal(t *testing.T, isTerminal func() bool) *cmd.RootCmd {
111+
// getenv is passed to NewRootCommand for FORCE_TTY / LD_FORCE_TTY only; nil uses os.Getenv.
112+
func newRootCmdWithTerminal(t *testing.T, isTerminal func() bool, getenv func(string) string) *cmd.RootCmd {
112113
t.Helper()
113114
rootCmd, err := cmd.NewRootCommand(
114115
config.NewService(&resources.MockClient{}),
@@ -117,12 +118,19 @@ func newRootCmdWithTerminal(t *testing.T, isTerminal func() bool) *cmd.RootCmd {
117118
"test",
118119
false,
119120
isTerminal,
121+
getenv,
120122
)
121123
require.NoError(t, err)
122124
return rootCmd
123125
}
124126

125127
func execNonTTYCmd(t *testing.T, mockClient *resources.MockClient, extraArgs ...string) []byte {
128+
t.Helper()
129+
return execNonTTYCmdGetenv(t, mockClient, nil, extraArgs...)
130+
}
131+
132+
// getenv is forwarded to NewRootCommand for FORCE_TTY / LD_FORCE_TTY only (nil uses os.Getenv).
133+
func execNonTTYCmdGetenv(t *testing.T, mockClient *resources.MockClient, getenv func(string) string, extraArgs ...string) []byte {
126134
t.Helper()
127135
rootCmd, err := cmd.NewRootCommand(
128136
config.NewService(&resources.MockClient{}),
@@ -131,6 +139,7 @@ func execNonTTYCmd(t *testing.T, mockClient *resources.MockClient, extraArgs ...
131139
"test",
132140
false,
133141
func() bool { return false },
142+
getenv,
134143
)
135144
require.NoError(t, err)
136145

@@ -161,117 +170,127 @@ func TestOutputDefaultsAndOverrides(t *testing.T) {
161170
}
162171

163172
t.Run("non-TTY defaults to json output", func(t *testing.T) {
164-
t.Setenv("FORCE_TTY", "")
165-
t.Setenv("LD_FORCE_TTY", "")
166-
167-
rootCmd := newRootCmdWithTerminal(t, func() bool { return false })
173+
rootCmd := newRootCmdWithTerminal(t, func() bool { return false }, func(string) string { return "" })
168174

169175
f := rootCmd.Cmd().PersistentFlags().Lookup(cliflags.OutputFlag)
170176
require.NotNil(t, f)
171177
assert.Equal(t, "json", f.DefValue)
172178
})
173179

174180
t.Run("TTY defaults to plaintext output", func(t *testing.T) {
175-
t.Setenv("FORCE_TTY", "")
176-
t.Setenv("LD_FORCE_TTY", "")
177-
178-
rootCmd := newRootCmdWithTerminal(t, func() bool { return true })
181+
rootCmd := newRootCmdWithTerminal(t, func() bool { return true }, func(string) string { return "" })
179182

180183
f := rootCmd.Cmd().PersistentFlags().Lookup(cliflags.OutputFlag)
181184
require.NotNil(t, f)
182185
assert.Equal(t, "plaintext", f.DefValue)
183186
})
184187

185-
t.Run("nil isTerminal defaults to json output", func(t *testing.T) {
186-
t.Setenv("FORCE_TTY", "")
187-
t.Setenv("LD_FORCE_TTY", "")
188-
189-
rootCmd := newRootCmdWithTerminal(t, nil)
190-
191-
f := rootCmd.Cmd().PersistentFlags().Lookup(cliflags.OutputFlag)
192-
require.NotNil(t, f)
193-
assert.Equal(t, "json", f.DefValue)
194-
})
195-
196188
t.Run("explicit --output plaintext overrides non-TTY", func(t *testing.T) {
197-
t.Setenv("FORCE_TTY", "")
198-
t.Setenv("LD_FORCE_TTY", "")
199-
200189
out := execNonTTYCmd(t, mockClient, "--output", "plaintext")
201190
assert.Contains(t, string(out), "Successfully updated")
202191
})
203192

204193
t.Run("non-TTY without explicit flag returns JSON", func(t *testing.T) {
205-
t.Setenv("FORCE_TTY", "")
206-
t.Setenv("LD_FORCE_TTY", "")
207-
208194
out := execNonTTYCmd(t, mockClient)
209195
assert.Contains(t, string(out), `"key"`)
210196
assert.NotContains(t, string(out), "Successfully updated")
211197
})
212198

213199
t.Run("--json overrides non-TTY default", func(t *testing.T) {
214-
t.Setenv("FORCE_TTY", "")
215-
t.Setenv("LD_FORCE_TTY", "")
216-
217200
out := execNonTTYCmd(t, mockClient, "--json")
218201
assert.Contains(t, string(out), `"key"`)
219202
assert.NotContains(t, string(out), "Successfully updated")
220203
})
221204

222205
t.Run("LD_OUTPUT=plaintext overrides non-TTY default", func(t *testing.T) {
223206
t.Setenv("LD_OUTPUT", "plaintext")
224-
t.Setenv("FORCE_TTY", "")
225-
t.Setenv("LD_FORCE_TTY", "")
226207

227208
out := execNonTTYCmd(t, mockClient)
228209
assert.Contains(t, string(out), "Successfully updated")
229210
assert.Contains(t, string(out), "test-name (test-key)")
230211
})
231212

232213
t.Run("FORCE_TTY=0 yields plaintext DefValue when non-TTY", func(t *testing.T) {
233-
t.Setenv("FORCE_TTY", "0")
234-
t.Setenv("LD_FORCE_TTY", "")
235-
236-
rootCmd := newRootCmdWithTerminal(t, func() bool { return false })
214+
getenv := func(k string) string {
215+
if k == "FORCE_TTY" {
216+
return "0"
217+
}
218+
return ""
219+
}
220+
rootCmd := newRootCmdWithTerminal(t, func() bool { return false }, getenv)
237221

238222
f := rootCmd.Cmd().PersistentFlags().Lookup(cliflags.OutputFlag)
239223
require.NotNil(t, f)
240224
assert.Equal(t, "plaintext", f.DefValue)
241225
})
242226

243227
t.Run("FORCE_TTY=1 yields plaintext DefValue when non-TTY", func(t *testing.T) {
244-
t.Setenv("FORCE_TTY", "1")
245-
t.Setenv("LD_FORCE_TTY", "")
246-
247-
rootCmd := newRootCmdWithTerminal(t, func() bool { return false })
228+
getenv := func(k string) string {
229+
if k == "FORCE_TTY" {
230+
return "1"
231+
}
232+
return ""
233+
}
234+
rootCmd := newRootCmdWithTerminal(t, func() bool { return false }, getenv)
248235

249236
f := rootCmd.Cmd().PersistentFlags().Lookup(cliflags.OutputFlag)
250237
require.NotNil(t, f)
251238
assert.Equal(t, "plaintext", f.DefValue)
252239
})
253240

254241
t.Run("LD_FORCE_TTY=1 yields plaintext DefValue when non-TTY", func(t *testing.T) {
255-
t.Setenv("FORCE_TTY", "")
256-
t.Setenv("LD_FORCE_TTY", "1")
257-
258-
rootCmd := newRootCmdWithTerminal(t, func() bool { return false })
242+
getenv := func(k string) string {
243+
if k == "LD_FORCE_TTY" {
244+
return "1"
245+
}
246+
return ""
247+
}
248+
rootCmd := newRootCmdWithTerminal(t, func() bool { return false }, getenv)
259249

260250
f := rootCmd.Cmd().PersistentFlags().Lookup(cliflags.OutputFlag)
261251
require.NotNil(t, f)
262252
assert.Equal(t, "plaintext", f.DefValue)
263253
})
264254

265255
t.Run("LD_FORCE_TTY=1 yields plaintext output when non-TTY", func(t *testing.T) {
266-
t.Setenv("FORCE_TTY", "")
267-
t.Setenv("LD_FORCE_TTY", "1")
256+
getenv := func(k string) string {
257+
if k == "LD_FORCE_TTY" {
258+
return "1"
259+
}
260+
return ""
261+
}
262+
out := execNonTTYCmdGetenv(t, mockClient, getenv)
263+
assert.Contains(t, string(out), "Successfully updated")
264+
assert.Contains(t, string(out), "test-name (test-key)")
265+
})
268266

269-
out := execNonTTYCmd(t, mockClient)
267+
t.Run("FORCE_TTY=1 yields plaintext output when non-TTY", func(t *testing.T) {
268+
getenv := func(k string) string {
269+
if k == "FORCE_TTY" {
270+
return "1"
271+
}
272+
return ""
273+
}
274+
out := execNonTTYCmdGetenv(t, mockClient, getenv)
270275
assert.Contains(t, string(out), "Successfully updated")
271276
assert.Contains(t, string(out), "test-name (test-key)")
272277
})
273278
}
274279

280+
func TestNewRootCommandNilIsTerminalRejects(t *testing.T) {
281+
_, err := cmd.NewRootCommand(
282+
config.NewService(&resources.MockClient{}),
283+
analytics.NoopClientFn{}.Tracker(),
284+
cmd.APIClients{},
285+
"test",
286+
false,
287+
nil,
288+
nil,
289+
)
290+
require.Error(t, err)
291+
assert.Contains(t, err.Error(), "isTerminal")
292+
}
293+
275294
func TestConfigOutputPrecedenceNonTTY(t *testing.T) {
276295
viper.Reset()
277296
t.Cleanup(viper.Reset)
@@ -282,8 +301,6 @@ func TestConfigOutputPrecedenceNonTTY(t *testing.T) {
282301

283302
cfgRoot := t.TempDir()
284303
t.Setenv("XDG_CONFIG_HOME", cfgRoot)
285-
t.Setenv("FORCE_TTY", "")
286-
t.Setenv("LD_FORCE_TTY", "")
287304

288305
ldcliDir := filepath.Join(cfgRoot, "ldcli")
289306
require.NoError(t, os.MkdirAll(ldcliDir, 0o755))
@@ -296,6 +313,7 @@ func TestConfigOutputPrecedenceNonTTY(t *testing.T) {
296313
"test",
297314
true,
298315
func() bool { return false },
316+
nil,
299317
)
300318
require.NoError(t, err)
301319

0 commit comments

Comments
 (0)