cmd/prompt: remove uses of github.com/AlecAivazis/survey/v2 - #14161
cmd/prompt: remove uses of github.com/AlecAivazis/survey/v2#14161thaJeztah wants to merge 1 commit into
Conversation
The github.com/AlecAivazis/survey/v2 module was archived and is no longer maintained. Looking at the code, we didn't really use most of its features; current callers already include the confirmation hint in the prompt message, so survey only adds terminal rendering behaviour around a simple yes/no question. Using the local implementation keeps the exported prompt types intact while removing the dependency. Replace the terminal confirmation provided by survey with a small local line-based implementation. Keep the existing non-terminal prompt behavior unchanged. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR cleanly removes the archived survey/v2 dependency and replaces it with a minimal local line-based implementation. The overall approach is sound. Two behavioral regressions in the new confirm() function are worth addressing before merge.
| } | ||
|
|
||
| func confirm(stdin *bufio.Reader, stdout io.Writer, message string, defaultValue bool) (bool, error) { | ||
| _, _ = fmt.Fprint(stdout, message) |
There was a problem hiding this comment.
[medium] confirm() does not print [y/N] hint; behavioral regression from survey.Confirm
The new confirm() function writes message verbatim to stdout with no indication of valid responses or the current default:
_, _ = fmt.Fprint(stdout, message)survey.Confirm automatically appended (y/N) or (Y/n) (depending on defaultValue) to every prompt. The new implementation omits this entirely. Unless every caller already embeds the hint in its own message string, users running in a terminal will see a bare prompt with no cue that y/n are the expected inputs or which value is the default.
Consider appending the hint in confirm() itself so callers don't have to know about this detail:
| _, _ = fmt.Fprint(stdout, message) | |
| if defaultValue { | |
| _, _ = fmt.Fprintf(stdout, "%s [Y/n] ", message) | |
| } else { | |
| _, _ = fmt.Fprintf(stdout, "%s [y/N] ", message) | |
| } |
| Confidence | Score |
|---|---|
| 🟡 moderate | 70/100 |
| if answer == "" { | ||
| return defaultValue, nil | ||
| } | ||
| return utils.StringToBool(answer), nil |
There was a problem hiding this comment.
[medium] Invalid user input silently returns false instead of re-prompting
When the user types an unrecognised string (e.g. "nno", "yyes", "maybe"), utils.StringToBool returns false and the function returns without any error or re-prompt:
return utils.StringToBool(answer), nilStringToBool accepts "y", "yes", "true", "1" as true and maps everything else to false. survey.Confirm looped until it received a valid answer. The new single-pass read means a mistype on a destructive confirmation (e.g. "Remove all volumes?") silently becomes a denial (false), giving the user no feedback that their input was not understood.
Consider validating the answer and returning an error or looping on unrecognised input. A minimal fix:
result, ok := parseBool(answer)
if !ok {
return false, fmt.Errorf("invalid input %q: expected y/yes/n/no", answer)
}
return result, nil| Confidence | Score |
|---|---|
| 🟢 strong | 100/100 |
glours
left a comment
glours
left a comment
There was a problem hiding this comment.
I confirm the 2 catches made by @docker-agent and found a 3rd one
| func confirm(stdin *bufio.Reader, stdout io.Writer, message string, defaultValue bool) (bool, error) { | ||
| _, _ = fmt.Fprint(stdout, message) | ||
|
|
||
| answer, err := stdin.ReadString('\n') |
There was a problem hiding this comment.
This blocking read has no way to observe cancellation. Since AdaptCmd's SIGINT handler (cmd/compose/compose.go:112) now intercepts Ctrl+C to cancel the context instead of letting the OS kill the process, a single Ctrl+C at a prompt no longer aborts it, the read stays blocked here and a second Ctrl+C is needed to actually terminate.
survey's raw-mode reader handled this internally (ISIG disabled, Ctrl+C caught as a keypress).
|
Ah, yes, I need to look at this one; I thought I'd move this separate, but didn't look too closely yet 😂 |
3 participants
The github.com/AlecAivazis/survey/v2 module was archived and is no longer maintained. Looking at the code, we didn't really use most of its features; current callers already include the confirmation hint in the prompt message, so survey only adds terminal rendering behaviour around a simple yes/no question. Using the local implementation keeps the exported prompt types intact while removing the dependency.
Replace the terminal confirmation provided by survey with a small local line-based implementation. Keep the existing non-terminal prompt behavior unchanged.
What I did
Related issue
(not mandatory) A picture of a cute animal, if possible in relation to what you did