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
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 |
There was a problem hiding this comment.
That's already handled by callers, which include it, so we just print verbatim; e.g.;
msg := "\nDo you want to proceed with these variables? [Y/n]: "
There was a problem hiding this comment.
You're right, thanks for the clarification. I see that both call sites in options.go already include the hint in their message strings (line 166 uses [Y/n] and line 290 uses [y/N]), so the verbatim printing is intentional and there's no regression here.
I should have checked all the callers before flagging this. Apologies for the noise!
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 😂 |
bed0960 to
e61e69a
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The PR correctly removes the archived github.com/AlecAivazis/survey/v2 dependency and replaces it with golang.org/x/term. The core Confirm implementation is structurally sound: SetRawTerminal / defer RestoreTerminal is safe, term.NewTerminal usage is idiomatic, the error propagation path is correct, and go.mod properly promotes golang.org/x/term from indirect to direct. The new tests cover the happy path, invalid-input retry, and Ctrl+C interruption.
Lower-confidence findings (not posted inline)
- [medium → verified low]
cmd/prompt/prompt.go:94— Ctrl+C handling relies onterm.ReadLinereturningio.EOFon byte0x03; ifgolang.org/x/termchanges this behaviour in a future release the loop would spin indefinitely until the TTY is closed. Currently working as intended in v0.45.0 — the verifier assessed this as speculative/low, score 40/100 (🟠 weak). No action required now; worth a comment in the code or a version note ingo.modif the team wants to guard against it long-term.
Yeah, not fixing that; we have tests, and they shouldn't break behavior. |
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 mostly provided terminal input handling around a simple yes/no question. Replace the interactive confirmation implementation with golang.org/x/term while keeping the exported prompt types and existing non-terminal prompt behavior unchanged. Interactive prompts continue to use raw terminal input so Ctrl+C is handled by the prompt instead of requiring a second interrupt to terminate the process. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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 mostly provided terminal input handling around a simple
yes/no question.
Replace the interactive confirmation implementation with
golang.org/x/term while keeping the exported prompt types and existing
non-terminal prompt behavior unchanged. Interactive prompts continue to
use raw terminal input so Ctrl+C is handled by the prompt instead of
requiring a second interrupt to terminate the process.
What I did
Related issue
(not mandatory) A picture of a cute animal, if possible in relation to what you did