-
Notifications
You must be signed in to change notification settings - Fork 5.9k
cmd/prompt: use ReadString instead of Fscanln, fix default value, and remove uses github.com/AlecAivazis/survey/v2 #13900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,10 +17,11 @@ | |
| package prompt | ||
|
|
||
| import ( | ||
| "bufio" | ||
| "fmt" | ||
| "io" | ||
| "strings" | ||
|
|
||
| "github.com/AlecAivazis/survey/v2" | ||
| "github.com/docker/cli/cli/streams" | ||
|
|
||
| "github.com/docker/compose/v5/pkg/utils" | ||
|
|
@@ -74,17 +75,7 @@ func (s streamsFileReader) Fd() uintptr { | |
|
|
||
| // Confirm asks for yes or no input | ||
| func (u User) Confirm(message string, defaultValue bool) (bool, error) { | ||
| qs := &survey.Confirm{ | ||
| Message: message, | ||
| Default: defaultValue, | ||
| } | ||
| var b bool | ||
| err := survey.AskOne(qs, &b, func(options *survey.AskOptions) error { | ||
| options.Stdio.In = u.stdin | ||
| options.Stdio.Out = u.stdout | ||
| return nil | ||
| }) | ||
| return b, err | ||
| return confirm(u.stdin, u.stdout, message, defaultValue) | ||
| } | ||
|
|
||
| // Pipe - aggregates prompt methods | ||
|
|
@@ -95,8 +86,19 @@ type Pipe struct { | |
|
|
||
| // Confirm asks for yes or no input | ||
| func (u Pipe) Confirm(message string, defaultValue bool) (bool, error) { | ||
| _, _ = fmt.Fprint(u.stdout, message) | ||
| var answer string | ||
| _, _ = fmt.Fscanln(u.stdin, &answer) | ||
| return confirm(u.stdin, u.stdout, message, defaultValue) | ||
| } | ||
|
|
||
| func confirm(stdin io.Reader, stdout io.Writer, message string, defaultValue bool) (bool, error) { | ||
| _, _ = fmt.Fprint(stdout, message) | ||
| answer, err := bufio.NewReader(stdin).ReadString('\n') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW] A new
In the current codebase A safer pattern is to create the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pre-existing issue; same as #13900 (comment)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right that the The That said, if this doesn't align with the project's priorities for this refactor, happy to defer it. Just wanted to flag the behaviour change. |
||
| if err != nil && err != io.EOF { | ||
| return false, err | ||
| } | ||
|
|
||
| answer = strings.TrimSpace(answer) | ||
| if answer == "" { | ||
| return defaultValue, nil | ||
| } | ||
| return utils.StringToBool(answer), nil | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM] Unrecognized user input silently returns
// pkg/utils/stringutils.go
func StringToBool(s string) bool {
s = strings.ToLower(strings.TrimSpace(s))
if s == "y" { return true }
b, _ := strconv.ParseBool(s) // error silently ignored
return b
}Before this PR, Consider validating the trimmed answer against an explicit set of accepted strings (e.g.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an existing issue; at least it was already inconsistent; if a TTY is present, it would use "survey", which would handle invalid input, but the other implementation didn't. It's probably fine as-is for y/n prompts, but we could improve this (as a follow-up)?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point — thanks for the context. Since this PR unifies the TTY and non-TTY paths and the silent-false behavior was already present in the non-TTY case, it's not a regression. Agree that improving validation as a follow-up makes sense rather than blocking this PR.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM] Unrecognised user input silently returns The new unified // pkg/utils/stringutils.go
func StringToBool(s string) bool {
s = strings.ToLower(strings.TrimSpace(s))
if s == "y" { return true }
b, _ := strconv.ParseBool(s) // error silently discarded
return b
}The old Consider one of:
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LOW]
bufio.NewReadercreated per call may silently discard buffered stdin bytes on repeatedConfirmcallsbufio.NewReader(stdin)wraps the underlyingio.Readerin a new 4096-byte buffer on every call toconfirm(). If the user types a response faster than the read consumes it, thebufio.Readermay buffer bytes beyond the newline delimiter. When the reader goes out of scope at the end ofconfirm(), those buffered-ahead bytes are lost; the nextconfirm()call creates a freshbufio.Readerfrom the (already-advanced) underlying reader and misses them.Current call-sites each make only a single
Confirmcall, so this does not trigger today. However, the design is fragile: any future caller that prompts more than once on the same stdin (e.g. a loop, or two sequential prompts in the same command handler) would silently drop input.Consider storing a single
*bufio.Readeron theUserandPipestructs (or accepting one as a constructor parameter) so the buffer is shared across calls rather than recreated each time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a pre-existing issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right — this pattern was already present before this PR. Since it's a latent issue that doesn't affect current usage and would require a small refactor to fix, it makes sense to handle it separately rather than blocking this change.
Feel free to ignore this comment for now, or we can track it as a potential future improvement if you think it's worth addressing.