feat(baremetal): ask what the machine will take, instead of making you guess - #276
Open
Denis-hamon wants to merge 8 commits into
Open
Conversation
…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>
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>
…' into feat/baremetal-install-progress
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
1 task
…u guess Reinstalling a server needs answers that depend on each other, and the CLI made the operator find that out one command at a time: list the templates, copy a name into `list-partition-schemes`, read that answer, copy two names into the reinstall. Three of those four steps are clerical, and all of them have to happen BEFORE the reinstall — finding out at install time means the disks are already wiped. `--wizard` asks the questions in the only order the API allows, fetching each list for the answer just given, then hands the result to the reinstall that was already there: same confirmation, same --dry-run, same --wait. Measured on the test account (8 servers): - 31 to 37 templates per server, all `ovh`, no personal ones; - exactly ONE partition scheme, "default", for every template tried; - no hardware RAID controller: 403 on all of them. Three rules come straight out of those numbers. A question with one possible answer is not a question, so the scheme is settled and stated rather than asked. A personal template is labelled as such rather than merged into the OVH list, because the two name spaces overlap and the wrong pick installs the wrong system. And nothing invents a hardwareRaid block: with no controller it is illegal, and with one the CLI still cannot size a configuration, since install/hardwareRaidSize answers 404 on every server tried. The wizard says which of the two it is and points at --from-file, which already carries the whole storage block untouched. A wizard with nobody at the keyboard would draw an alternate screen over a pipeline's logs and wait for an answer that cannot come, so a non-interactive run refuses, before touching the API, and names the flags that replace it. Twelve tests, each sabotage-checked: asking for the schemes without the template, asking a one-answer question, merging the personal templates, turning the RAID 403 into a failure and removing the terminal guard each turn exactly one test red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Denis-hamon
force-pushed
the
feat/baremetal-reinstall-wizard
branch
from
August 26, 2026 10:02
1cfd7a0 to
eaaf6cb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #253 — only the last commit belongs to this PR.
Opened against #274, which asks for "an easy to use ergonomic wizard that knows most of the dynamic business rules" for OS reinstallation.
The rules are dynamic because the answers depend on each other
#250 added the two commands that answer the questions: which partition schemes a template allows, and whether the machine has a RAID controller. What it could not fix is that they have to be chained by hand:
Four commands, three of them clerical, and the second cannot be typed before the first is read — the schemes are a property of the template. All of it has to happen before the reinstall: finding out at install time means the disks are already wiped.
Each list is fetched for the answer just given. The result is handed to the reinstall that was already there: same confirmation (type the server name), same
--dry-run, same--waitwith the per-step progress from #253.What it knows, and where each rule comes from
Measured on the test account, 8 servers, 25/08:
ovh, no personal onesdefault, every timeThree behaviours come straight out of those numbers:
mine_64installs the wrong system.hardwareRaidblock. With no controller it is illegal; with one, the CLI still cannot size a configuration, becauseinstall/hardwareRaidSizeis in the schema, badged "Stable production version", and answers 404 on every server tried (see the note ininstall.gofrom feat(baremetal): make the storage block of a reinstall discoverable #250). The wizard says which of the two situations you are in and points at--from-file, which already carries the wholestorageblock untouched.Only
partitioning.schemeNameis modelled in Go. The rest of that block — disk groups, layouts, LVM and ZFS extras — stays where it belongs, in a file: expressing an array of partition objects as flags is how a CLI becomes unusable.A wizard with nobody at the keyboard
--wizardin a pipeline would draw an alternate screen over the logs and wait for an answer that cannot come. It refuses instead, before touching the API, and names what to use in its place:It is mutually exclusive with
--from-fileand--editor, which settle the same fields — otherwise the last writer would win in silence.Run end to end, on a real server
Exit 0, and the 32 entries in that list are what the API answered for that machine at that moment. The non-interactive refusal was run the same way: right message, exit 1.
Tests
Twelve, and each one was sabotage-checked: asking for the schemes without the template, asking a one-answer question, merging the personal templates into the OVH list, turning the RAID 403 into a blocking error, and removing the terminal guard each turn exactly one test red, and the right one.
The picker and the terminal check are two package-level variables so the wizard can be driven from a test at all — every other selector in this CLI calls
display.RunGenericChoicePickerdirectly and is therefore untestable.Type of change