Skip to content

fix(utils): a confirmation could be typed, read, and thrown away - #252

Draft
Denis-hamon wants to merge 16 commits into
ovh:feat/baremetal-install-discoveryfrom
Denis-hamon:fix/confirmation-prompt-io
Draft

fix(utils): a confirmation could be typed, read, and thrown away#252
Denis-hamon wants to merge 16 commits into
ovh:feat/baremetal-install-discoveryfrom
Denis-hamon:fix/confirmation-prompt-io

Conversation

@Denis-hamon

Copy link
Copy Markdown
Contributor

Stacked on #250. Only the last commit belongs to this pull request.

A confirmation could be typed, read, and thrown away

bufio.Reader.ReadString returns the text it read and an error when the
stream ends without a delimiter. Both confirmation helpers dropped the text on
any error:

answer, err := reader.ReadString('\n')
if err != nil {
    return false          // discards `answer`, even when it is "yes"
}

So an operator who typed an answer and pressed Ctrl-D instead of Enter was
refused, silently, while the guard was holding their answer. Anything actually
typed now counts. Only an empty read is a failure to answer, and that one says
so instead of looking like a decline nobody made.

This is not a way past the guard. A pipe is not an interactive terminal, so
the unattended path refuses before it ever reads. The change only affects
somebody at a keyboard who ended their line with Ctrl-D.

The prompt itself was untestable

confirmInput was injectable so a test could type; the writer was hardcoded to
os.Stderr, so no test could read the question. That asymmetry has already
cost something concrete: a test added in #249 that asserted a server's display
name appeared in the confirmation had to be moved to a service-level unit test,
because the prompt is written outside the channel cmd.Execute captures.

confirmOutput is now injectable alongside it. The wording an operator reads
before erasing a disk is the part worth pinning, and five tests now pin it: it
names the risk, it shows the default, it says which name to type, and it does
not end in a newline — which would push the answer onto the line below the
.

Not routed through internal/display, and the code says why

Review suggested these prompts should go through internal/display rather than
fmt.Fprintf. Measured against what that package does, it would break the
prompt five ways: every path ends in fmt.Println on stdout, adds a
newline after the , JSON-encodes the message under -o json, overwrites the
ResultString global, and — for a warning or an error — calls ExitFunc,
which ends the process. A question is none of those things, and one that
terminated before reading the answer would not be a question.

The layering instinct is right; the target is not. That reasoning is now a
comment above the variables rather than tribal knowledge.

Verification

go test ./..., go vet, make wasm, docgen — all green.

Five sabotages, five red, each on the test meant to catch it:

sabotage test that fell
restore if err != nil { return false } HonoursAnAnswerEndedByEOF ×2
drop the unreadable-answer message SaysSoWhenNothingCouldBeRead
end the prompt with a newline AsksBeforeItReads
drop the warning from the prompt AsksBeforeItReads
stop showing the name to type AsksForTheExactName

Denis-hamon and others added 3 commits August 18, 2026 17:02
…to change

`vps service-info edit myvps --renew-period 12` used to send this:

    {"renew":{"automatic":false,"deleteAtExpiration":false,"forced":false,
              "manualPayment":false,"period":12}}

The renewal settings are booleans bound to a shared struct carrying no
`omitempty`, so every one of them was marshalled at its zero value and won the
merge against the fetched resource. Changing the renewal period therefore also
switched automatic renewal off — on a service that had been renewing itself for
years, without a word in the output saying so.

Webhosting already built its payload from `cmd.Flags().Changed`, and did not
have the defect. This promotes that builder into `common`, so `vps` gets it too
and the next `service-info edit` cannot reintroduce the bug by reaching for the
struct.

Reading `Changed` rather than the values keeps `--renew-automatic=false`
working: pflag records a flag as changed whatever value it was given, so an
explicit false is still sent while an absent flag stays absent. Both cases are
covered by a test, and each test was checked against the failure it exists to
catch.

The shared mutable `ServiceInfoSpec` goes away with the last thing that read
it, and the five flag registrations repeated across four commands become one
call, which also settles the two spellings of the period's help text.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
…ayer

Declaring cobra flags is the command layer's job, and internal/services/common
was the only service package doing it — the shared flag helpers all live in
internal/cmd. Raised in review of this PR.

The registration moves; the table does not. Both halves need the flag name —
one to declare it, the other to read whether the operator set it — and that
name is the only thing tying them together. Splitting it into two copies would
mean a rename could touch one side and leave the other silently no longer
sending a setting, which is the exact failure this PR exists to fix. So
common.ServiceInfoRenewFlags becomes the exported description, internal/cmd
registers from it, and the payload builder keeps reading it.

Checked by renaming an entry in that table and watching the service-info tests
fall: the two halves still move together.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bufio reports end of file alongside the text it did read. Discarding the text
whenever an error came back meant somebody who typed an answer and pressed
Ctrl-D instead of Enter was refused in silence — the guard had their answer and
dropped it. Anything actually typed now counts; only an empty read is a failure
to answer, and that one says so rather than looking like a decline nobody made.

This is not a way past the guard: a pipe is not an interactive terminal, so
that path refuses before it ever reads.

The writer becomes injectable alongside the reader. The wording an operator
sees before erasing a disk is the part worth pinning, and it was the one part
no test could reach: a P5 test that tried to assert it had to be moved
elsewhere because the prompt is written outside the channel cmd.Execute
captures. Five tests now cover the question itself — that it names the risk,
shows the default, and does not end in a newline that would push the answer
onto the next line.

Deliberately not routed through internal/display, and the comment says why:
every path through that package ends on stdout, JSON-encodes the message under
-o json, overwrites a global, and — for a warning or an error — calls ExitFunc.
A question is none of those, and one that ended the process before reading the
answer would not be a question.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Denis-hamon
Denis-hamon requested a review from a team as a code owner August 19, 2026 10:02
@Denis-hamon
Denis-hamon marked this pull request as draft August 20, 2026 13:09
Denis-hamon and others added 3 commits August 20, 2026 16:14
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
The first sweep replaced only the exact spelling. A test that checks a name can
be copied back without reproducing its case carried the uppercase form of a
real hostname, which therefore survived — and, once its neighbour was
replaced, made that test fail. A real value does not stop being one because one
of its spellings was removed.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
@Denis-hamon
Denis-hamon changed the base branch from main to feat/baremetal-install-discovery August 20, 2026 15:18
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
ovh#243 moved the Cobra registration of the renewal flags out of
internal/services/common and into the command layer, and exported the
descriptor table so both halves keep reading one list. That commit landed after
this branch had already taken its parent, so this branch still carried the
version where a service package declares flags — and being the more recent side
of the merge, it would have won and quietly undone the refactor.

Merged rather than rebased: nothing is rewritten, so the review threads on this
PR stay attached to their lines.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
A --dry-run already carries the parameters in its message, and a
log.Println sitting just above the branch repeated the same JSON behind
a Go timestamp no other command in this CLI emits:

    🔍 Dry run: nothing was sent. This would have been posted to …
    { "operatingSystem": "debian12_64" }
    2026/08/23 23:47:22 Final parameters:
    { "operatingSystem": "debian12_64" }

The log line moves below the branch. A real run still logs what it is
about to send, which is what it was for; a dry run logs nothing, because
it sends nothing.

The line goes to stderr, so no assertion on stdout could ever have seen
it — which is why it survived every green run. Two tests now redirect the
logger: one that a dry run does not log, and its positive control that a
real run still does, so deleting the line outright would not pass.

Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Mechanical, not a change of behaviour. Placing them just after
TestBaremetalReinstallDryRun put them in the one region of
baremetal_test.go that 23 downstream branches also append to, and in the
import block they all touch as well. Merging the parent into those
branches failed 23 times out of 23, every one of them on adjacency
rather than on a disagreement.

baremetal_test.go goes back to what it was before the previous commit,
so this branch now leaves that file untouched. A new file can only clash
with a file of the same name, and nothing else carries this one.

Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant