Skip to content

Commit 9747414

Browse files
authored
Use cmdio.IsPromptSupported() more widely (#3857)
## Changes All prompting logic under `cmd/` now uses a single `IsPromptSupported()` function instead of ad-hoc TTY checks: ### Before Callers checked various combinations: - `IsInTTY(ctx) && IsOutTTY(ctx)` - `!IsInTTY(ctx)` - `IsOutTTY(ctx) && IsInTTY(ctx) && !IsGitBash(ctx)` ### After All callers use: ```go if cmdio.IsPromptSupported(ctx) { // prompt user } else { // return error or use non-interactive fallback } ``` ### Behavior Changes Prompting now requires ALL of: - Interactive mode (no dumb terminal, no NO_COLOR) - stdin to be a TTY (for reading input) - stdout to be a TTY (for showing prompts) - Not running in Git Bash on Windows This prevents prompting in edge cases that weren't properly handled before (e.g., TTY stdin with piped stdout). ## Why Higher level capability checks mean we can more easily swap out what we do for terminal I/O. ## Tests Existing tests pass. Manually ran `configure` and `auth login` commands.
1 parent ad5bab5 commit 9747414

File tree

10 files changed

+61
-67
lines changed

10 files changed

+61
-67
lines changed

cmd/auth/auth.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ GCP: https://docs.gcp.databricks.com/dev-tools/auth/index.html`,
3535
}
3636

3737
func promptForHost(ctx context.Context) (string, error) {
38-
if !cmdio.IsInTTY(ctx) {
38+
if !cmdio.IsPromptSupported(ctx) {
3939
return "", errors.New("the command is being run in a non-interactive environment, please specify a host using --host")
4040
}
4141

@@ -45,7 +45,7 @@ func promptForHost(ctx context.Context) (string, error) {
4545
}
4646

4747
func promptForAccountID(ctx context.Context) (string, error) {
48-
if !cmdio.IsInTTY(ctx) {
48+
if !cmdio.IsPromptSupported(ctx) {
4949
return "", errors.New("the command is being run in a non-interactive environment, please specify an account ID using --account-id")
5050
}
5151

cmd/auth/login.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import (
2424
)
2525

2626
func promptForProfile(ctx context.Context, defaultValue string) (string, error) {
27-
if !cmdio.IsInTTY(ctx) {
27+
if !cmdio.IsPromptSupported(ctx) {
2828
return "", nil
2929
}
3030

cmd/auth/login_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ func TestSetHostDoesNotFailWithNoDatabrickscfg(t *testing.T) {
3333
func TestSetHost(t *testing.T) {
3434
var authArguments auth.AuthArguments
3535
t.Setenv("DATABRICKS_CONFIG_FILE", "./testdata/.databrickscfg")
36-
ctx, _ := cmdio.SetupTest(context.Background())
36+
ctx, _ := cmdio.SetupTest(context.Background(), cmdio.TestOptions{})
3737

3838
profile1 := loadTestProfile(t, ctx, "profile-1")
3939
profile2 := loadTestProfile(t, ctx, "profile-2")
@@ -76,7 +76,7 @@ func TestSetHost(t *testing.T) {
7676
func TestSetAccountId(t *testing.T) {
7777
var authArguments auth.AuthArguments
7878
t.Setenv("DATABRICKS_CONFIG_FILE", "./testdata/.databrickscfg")
79-
ctx, _ := cmdio.SetupTest(context.Background())
79+
ctx, _ := cmdio.SetupTest(context.Background(), cmdio.TestOptions{})
8080

8181
accountProfile := loadTestProfile(t, ctx, "account-profile")
8282

cmd/configure/configure.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ The host must be specified with the --host flag or the DATABRICKS_HOST environme
128128
}
129129

130130
ctx := cmd.Context()
131-
if cmdio.IsInTTY(ctx) && cmdio.IsOutTTY(ctx) {
131+
if cmdio.IsPromptSupported(ctx) {
132132
err = configureInteractive(cmd, &flags, &cfg)
133133
} else {
134134
err = configureNonInteractive(cmd, &flags, &cfg)

cmd/root/auth_test.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ func expectPrompts(t *testing.T, fn promptFn, config *config.Config) {
3939
// Channel to pass errors from the prompting function back to the test.
4040
errch := make(chan error, 1)
4141

42-
ctx, io := cmdio.SetupTest(ctx)
42+
ctx, io := cmdio.SetupTest(ctx, cmdio.TestOptions{PromptSupported: true})
4343
go func() {
4444
defer close(errch)
4545
defer cancel()
@@ -61,7 +61,7 @@ func expectReturns(t *testing.T, fn promptFn, config *config.Config) {
6161
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
6262
defer cancel()
6363

64-
ctx, _ = cmdio.SetupTest(ctx)
64+
ctx, _ = cmdio.SetupTest(ctx, cmdio.TestOptions{PromptSupported: true})
6565
client, err := fn(ctx, config, true)
6666
require.NoError(t, err)
6767
require.NotNil(t, client)
@@ -224,7 +224,7 @@ func TestMustAccountClientWorksWithDatabricksCfg(t *testing.T) {
224224
func TestMustAccountClientWorksWithNoDatabricksCfgButEnvironmentVariables(t *testing.T) {
225225
testutil.CleanupEnvironment(t)
226226

227-
ctx, tt := cmdio.SetupTest(context.Background())
227+
ctx, tt := cmdio.SetupTest(context.Background(), cmdio.TestOptions{PromptSupported: true})
228228
t.Cleanup(tt.Done)
229229
cmd := New(ctx)
230230
t.Setenv("DATABRICKS_HOST", "https://accounts.azuredatabricks.net/")
@@ -238,7 +238,7 @@ func TestMustAccountClientWorksWithNoDatabricksCfgButEnvironmentVariables(t *tes
238238
func TestMustAccountClientErrorsWithNoDatabricksCfg(t *testing.T) {
239239
testutil.CleanupEnvironment(t)
240240

241-
ctx, tt := cmdio.SetupTest(context.Background())
241+
ctx, tt := cmdio.SetupTest(context.Background(), cmdio.TestOptions{PromptSupported: true})
242242
t.Cleanup(tt.Done)
243243
cmd := New(ctx)
244244

@@ -261,7 +261,7 @@ func TestMustAnyClientCanCreateWorkspaceClient(t *testing.T) {
261261
0o755)
262262
require.NoError(t, err)
263263

264-
ctx, tt := cmdio.SetupTest(context.Background())
264+
ctx, tt := cmdio.SetupTest(context.Background(), cmdio.TestOptions{PromptSupported: true})
265265
t.Cleanup(tt.Done)
266266
cmd := New(ctx)
267267

@@ -290,7 +290,7 @@ func TestMustAnyClientCanCreateAccountClient(t *testing.T) {
290290
0o755)
291291
require.NoError(t, err)
292292

293-
ctx, tt := cmdio.SetupTest(context.Background())
293+
ctx, tt := cmdio.SetupTest(context.Background(), cmdio.TestOptions{PromptSupported: true})
294294
t.Cleanup(tt.Done)
295295
cmd := New(ctx)
296296

@@ -314,7 +314,7 @@ func TestMustAnyClientWithEmptyDatabricksCfg(t *testing.T) {
314314
0o755)
315315
require.NoError(t, err)
316316

317-
ctx, tt := cmdio.SetupTest(context.Background())
317+
ctx, tt := cmdio.SetupTest(context.Background(), cmdio.TestOptions{PromptSupported: true})
318318
t.Cleanup(tt.Done)
319319
cmd := New(ctx)
320320

cmd/workspace/secrets/put_secret.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"encoding/base64"
55
"errors"
66
"io"
7-
"os"
87

98
"github.com/databricks/cli/cmd/root"
109
"github.com/databricks/cli/libs/cmdctx"
@@ -117,9 +116,9 @@ func newPutSecret() *cobra.Command {
117116
}
118117

119118
func promptSecret(cmd *cobra.Command) ([]byte, error) {
120-
// If stdin is a TTY, prompt for the secret.
121-
if !cmdio.IsInTTY(cmd.Context()) {
122-
return io.ReadAll(os.Stdin)
119+
// If prompting is not supported, read secret from stdin.
120+
if !cmdio.IsPromptSupported(cmd.Context()) {
121+
return io.ReadAll(cmd.InOrStdin())
123122
}
124123

125124
value, err := cmdio.Secret(cmd.Context(), "Please enter your secret value")

libs/cmdio/io.go

Lines changed: 27 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ type cmdIO struct {
2323
// states if we are in the interactive mode
2424
// e.g. if stdout is a terminal
2525
interactive bool
26+
prompt bool
2627
outputFormat flags.Output
2728
headerTemplate string
2829
template string
@@ -37,8 +38,21 @@ func NewIO(ctx context.Context, outputFormat flags.Output, in io.Reader, out, er
3738
if f, ok := err.(*os.File); ok && !dumb {
3839
dumb = !isatty.IsTerminal(f.Fd()) && !isatty.IsCygwinTerminal(f.Fd())
3940
}
41+
42+
// Interactive mode is the opposite of "dumb" mode.
43+
// TODO(@pietern): Clean this up later. Don't want to change more logic in this PR.
44+
interactive := !dumb
45+
46+
// Prompting requires:
47+
// - "interactive" mode (i.e. the terminal to not be dumb or use NO_COLOR (because promptui uses both)
48+
// - stdin to be a TTY (for reading input)
49+
// - stdout to be a TTY (for showing prompts)
50+
// - not to be running in Git Bash on Windows
51+
prompt := interactive && IsTTY(in) && IsTTY(out) && !isGitBash(ctx)
52+
4053
return &cmdIO{
41-
interactive: !dumb,
54+
interactive: interactive,
55+
prompt: prompt,
4256
outputFormat: outputFormat,
4357
headerTemplate: headerTemplate,
4458
template: template,
@@ -53,57 +67,28 @@ func IsInteractive(ctx context.Context) bool {
5367
return c.interactive
5468
}
5569

56-
// IsTTY detects if io.Writer is a terminal.
57-
func IsTTY(w any) bool {
58-
f, ok := w.(*os.File)
59-
if !ok {
60-
return false
61-
}
62-
fd := f.Fd()
63-
return isatty.IsTerminal(fd) || isatty.IsCygwinTerminal(fd)
64-
}
65-
66-
// IsInTTY detects if the input reader is a terminal.
67-
func IsInTTY(ctx context.Context) bool {
68-
c := fromContext(ctx)
69-
return IsTTY(c.in)
70-
}
71-
72-
// IsOutTTY detects if the output writer is a terminal.
73-
func IsOutTTY(ctx context.Context) bool {
74-
c := fromContext(ctx)
75-
return IsTTY(c.out)
76-
}
77-
78-
// IsErrTTY detects if the error writer is a terminal.
79-
func IsErrTTY(ctx context.Context) bool {
70+
func IsPromptSupported(ctx context.Context) bool {
8071
c := fromContext(ctx)
81-
return IsTTY(c.err)
72+
return c.prompt
8273
}
8374

84-
// IsTTY detects if stdout is a terminal. It assumes that stderr is terminal as well
85-
func (c *cmdIO) IsTTY() bool {
86-
f, ok := c.out.(*os.File)
75+
// IsTTY detects if io.Writer is a terminal.
76+
func IsTTY(w any) bool {
77+
f, ok := w.(*os.File)
8778
if !ok {
8879
return false
8980
}
9081
fd := f.Fd()
9182
return isatty.IsTerminal(fd) || isatty.IsCygwinTerminal(fd)
9283
}
9384

94-
func IsPromptSupported(ctx context.Context) bool {
95-
// We do not allow prompting in non-interactive mode and in Git Bash on Windows.
96-
// Likely due to fact that Git Bash does not (correctly support ANSI escape sequences,
97-
// we cannot use promptui package there.
98-
// See known issues:
99-
// - https://github.com/manifoldco/promptui/issues/208
100-
// - https://github.com/chzyer/readline/issues/191
101-
// We also do not allow prompting in non-interactive mode,
102-
// because it's not possible to read from stdin in non-interactive mode.
103-
return (IsInteractive(ctx) || (IsOutTTY(ctx) && IsInTTY(ctx))) && !IsGitBash(ctx)
104-
}
105-
106-
func IsGitBash(ctx context.Context) bool {
85+
// We do not allow prompting in non-interactive mode and in Git Bash on Windows.
86+
// Likely due to fact that Git Bash does not (correctly support ANSI escape sequences,
87+
// we cannot use promptui package there.
88+
// See known issues:
89+
// - https://github.com/manifoldco/promptui/issues/208
90+
// - https://github.com/chzyer/readline/issues/191
91+
func isGitBash(ctx context.Context) bool {
10792
// Check if the MSYSTEM environment variable is set to "MINGW64"
10893
msystem := env.Get(ctx, "MSYSTEM")
10994
if strings.EqualFold(msystem, "MINGW64") {

libs/cmdio/io_test.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,12 @@ import (
88
"github.com/stretchr/testify/assert"
99
)
1010

11-
func TestIsPromptSupportedFalseForGitBash(t *testing.T) {
11+
func TestIsGitBash(t *testing.T) {
1212
ctx := context.Background()
13-
ctx, _ = SetupTest(ctx)
14-
15-
assert.True(t, IsPromptSupported(ctx))
13+
assert.False(t, isGitBash(ctx))
1614

1715
ctx = env.Set(ctx, "MSYSTEM", "MINGW64")
1816
ctx = env.Set(ctx, "TERM", "xterm")
1917
ctx = env.Set(ctx, "PS1", "\\[\033]0;$TITLEPREFIX:$PWD\007\\]\n\\[\033[32m\\]\\u@\\h \\[\033[35m\\]$MSYSTEM \\[\033[33m\\]\\w\\[\033[36m\\]`__git_ps1`\\[\033[0m\\]\n$")
20-
assert.False(t, IsPromptSupported(ctx))
18+
assert.True(t, isGitBash(ctx))
2119
}

libs/cmdio/testing.go

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,26 @@ type Test struct {
1414
Stderr *bufio.Reader
1515
}
1616

17-
func SetupTest(ctx context.Context) (context.Context, *Test) {
17+
type TestOptions struct {
18+
// PromptSupported indicates whether IsPromptSupported should return true
19+
// for the test context. If false (default), prompting will fail as it would
20+
// in a non-interactive environment (e.g., CI).
21+
PromptSupported bool
22+
}
23+
24+
// SetupTest creates a cmdio context with pipes for stdin/stdout/stderr.
25+
// This is useful for testing interactive I/O operations.
26+
//
27+
// By default, IsPromptSupported returns false for the test context because
28+
// pipes are not TTYs. To test prompt logic, pass TestOptions{PromptSupported: true}.
29+
func SetupTest(ctx context.Context, opts TestOptions) (context.Context, *Test) {
1830
rin, win := io.Pipe()
1931
rout, wout := io.Pipe()
2032
rerr, werr := io.Pipe()
2133

2234
cmdio := &cmdIO{
2335
interactive: true,
36+
prompt: opts.PromptSupported,
2437
in: rin,
2538
out: wout,
2639
err: werr,

libs/template/config.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -259,8 +259,7 @@ func (c *config) promptForValues(r *renderer) error {
259259
// Prompt user for any missing config values. Assign default values if
260260
// terminal is not TTY
261261
func (c *config) promptOrAssignDefaultValues(r *renderer) error {
262-
// TODO: replace with IsPromptSupported call (requires fixing TestAccBundleInitErrorOnUnknownFields test)
263-
if cmdio.IsOutTTY(c.ctx) && cmdio.IsInTTY(c.ctx) && !cmdio.IsGitBash(c.ctx) {
262+
if cmdio.IsPromptSupported(c.ctx) {
264263
return c.promptForValues(r)
265264
}
266265
log.Debugf(c.ctx, "Terminal is not TTY. Assigning default values to template input parameters")

0 commit comments

Comments
 (0)