diff --git a/doc/ovhcloud.md b/doc/ovhcloud.md index 7d79d6150..acad5f7fc 100644 --- a/doc/ovhcloud.md +++ b/doc/ovhcloud.md @@ -1,90 +1,36 @@ -# OVHcloud CLI (`ovhcloud`) Documentation +## ovhcloud ---- +CLI to manage your OVHcloud services -## Overview +### Options -`ovhcloud` is a single, unified command‑line interface for managing the full range of OVHcloud products and account resources directly from your terminal. Whether you need to automate provisioning, perform quick look‑ups, or integrate OVHcloud operations into CI/CD pipelines, `ovhcloud` offers fine‑grained commands and consistent output formats (table, JSON, YAML, or custom gval expressions). - ---- - -## Quick Start - -```bash -# Display the top‑level help -ovhcloud --help - -# Log in and create API credentials (interactive) -ovhcloud login - -# List your VPS instances as JSON -ohvcloud vps list -o json -``` - -Check out the [authentication page](authentication.md) for further information about the authentication means. - -You can manage multiple OVHcloud accounts using [profiles](profiles.md). Create a profile with `ovhcloud login --profile `, switch between them with `ovhcloud config profile switch `, or use `--profile ` on any command. - -### Generate Shell Completion - -```bash -# Bash -eval "$(./ovhcloud completion bash)" -# Zsh -eval "$(./ovhcloud completion zsh)" -# Fish -./ovhcloud completion fish | source -# PowerShell -./ovhcloud completion powershell | Out-String | Invoke-Expression ``` - -Add the appropriate line to your shell’s startup file (`~/.bashrc`, `~/.zshrc`, etc.) to enable persistent autocompletion. - ---- - -## Global Usage - -```text -ovhcloud [command] [flags] + -d, --debug Activate debug mode (will log all HTTP requests details) + -h, --help help for ovhcloud + -e, --ignore-errors Ignore errors in API calls when it is not fatal to the execution + -o, --output string Output format: json, yaml, interactive, or a custom format expression (using https://github.com/PaesslerAG/gval syntax) + Examples: + --output json + --output yaml + --output interactive + --output 'id' (to extract a single field) + --output 'nested.field.subfield' (to extract a nested field) + --output '[id, "name"]' (to extract multiple fields as an array) + --output '{"newKey": oldKey, "otherKey": nested.field}' (to extract and rename fields in an object) + --output 'name+","+type' (to extract and concatenate fields in a string) + --output '(nbFieldA + nbFieldB) * 10' (to compute values from numeric fields) + --profile string Use a specific profile from the configuration file ``` -### Global Flags - -| Flag | Description | -| ------------------ | ---------------------------------------------------- | -| `--debug` | Activate debug mode (logs all HTTP‑request details). | -| `--ignore-errors` | Ignore errors of API calls made when listing items. | -| `--filter ` | Filter lists output with a [gval] expression. | -| `-h`, `--help` | Display help for `ovhcloud` or a specific command. | -| `-o interactive` | Produce interactive (prompt‑based) output. | -| `-o json` | Output data in JSON format. | -| `-o yaml` | Output data in YAML format. | -| `-o ` | Format output with a [gval] expression. | - -[gval]: https://github.com/PaesslerAG/gval - -#### Filtering examples - -- Strict string equality: `--filter 'name=="something"'` -- String regexp comparison: `--filter 'name=~"something"'` -- Number comparison: `--filter 'bootId > 1'` - -#### Formatting example - -- Extract only one field: `-o 'ip'` -- Extract an object: `-o '{name: ip}'` - ---- - -## Command Reference - -Below is the full list of primary sub‑commands available at the time of writing. Each can be explored in depth with `ovhcloud --help`. +### SEE ALSO * [ovhcloud account](ovhcloud_account.md) - Manage your account * [ovhcloud alldom](ovhcloud_alldom.md) - Retrieve information and manage your AllDom services * [ovhcloud baremetal](ovhcloud_baremetal.md) - Retrieve information and manage your Bare Metal services +* [ovhcloud browser](ovhcloud_browser.md) - Launch a TUI for the OVHcloud Manager - Public Cloud universe only [EXPERIMENTAL] * [ovhcloud cdn-dedicated](ovhcloud_cdn-dedicated.md) - Retrieve information and manage your dedicated CDN services * [ovhcloud cloud](ovhcloud_cloud.md) - Manage your projects and services in the Public Cloud universe (MKS, MPR, MRS, Object Storage...) +* [ovhcloud completion](ovhcloud_completion.md) - Generate shell completion scripts * [ovhcloud config](ovhcloud_config.md) - Manage your CLI configuration * [ovhcloud dedicated-ceph](ovhcloud_dedicated-ceph.md) - Retrieve information and manage your Dedicated Ceph services * [ovhcloud dedicated-cloud](ovhcloud_dedicated-cloud.md) - Retrieve information and manage your DedicatedCloud services @@ -102,6 +48,7 @@ Below is the full list of primary sub‑commands available at the time of writin * [ovhcloud ldp](ovhcloud_ldp.md) - Retrieve information and manage your LDP (Logs Data Platform) services * [ovhcloud location](ovhcloud_location.md) - Retrieve information and manage your Location services * [ovhcloud login](ovhcloud_login.md) - Login to your OVHcloud account to create API credentials +* [ovhcloud logout](ovhcloud_logout.md) - Revoke your API credentials and remove them from the configuration * [ovhcloud nutanix](ovhcloud_nutanix.md) - Retrieve information and manage your Nutanix services * [ovhcloud okms](ovhcloud_okms.md) - Retrieve information and manage your OKMS (Key Management Services) * [ovhcloud overthebox](ovhcloud_overthebox.md) - Retrieve information and manage your OverTheBox services @@ -113,6 +60,7 @@ Below is the full list of primary sub‑commands available at the time of writin * [ovhcloud storage-netapp](ovhcloud_storage-netapp.md) - Retrieve information and manage your Storage NetApp services * [ovhcloud support-tickets](ovhcloud_support-tickets.md) - Retrieve information and manage your support tickets * [ovhcloud telephony](ovhcloud_telephony.md) - Retrieve information and manage your Telephony services +* [ovhcloud upgrade](ovhcloud_upgrade.md) - Upgrade OVHcloud CLI to the latest version * [ovhcloud veeamcloudconnect](ovhcloud_veeamcloudconnect.md) - Retrieve information and manage your VeeamCloudConnect services * [ovhcloud veeamenterprise](ovhcloud_veeamenterprise.md) - Retrieve information and manage your VeeamEnterprise services * [ovhcloud version](ovhcloud_version.md) - Get OVHcloud CLI version @@ -124,30 +72,3 @@ Below is the full list of primary sub‑commands available at the time of writin * [ovhcloud webhosting](ovhcloud_webhosting.md) - Retrieve information and manage your WebHosting services * [ovhcloud xdsl](ovhcloud_xdsl.md) - Retrieve information and manage your XDSL services -> **Tip**  Use `-o json`, `-o yaml`, or `-o ` with a gval expression to integrate `ovhcloud` into scripts and automation pipelines. - ---- - -## Examples - -| Task | Command | -| ------------------------------------- | ---------------------------------------------- | -| Log in and save credentials | `ovhcloud login` | -| List VPS instances (tabular) | `ovhcloud vps list` | -| Fetch details of a single VPS in JSON | `ovhcloud vps get -o json` | -| Reinstall a baremetal interactively | `ovhcloud baremetal reinstall --editor` | - ---- - -## Troubleshooting - -* **Verbose output** — Use `--debug` to inspect raw API calls and responses. -* **Authentication issues** — Run `ovhcloud login` again to regenerate valid API keys. -* **Rate limits** — OVHcloud APIs impose rate limits; plan retries or exponential backoff in scripts. - ---- - -## Further Reading - -* OVHcloud API reference: [https://eu.api.ovh.com/console](https://eu.api.ovh.com/console) -* OVHcloud community guides and tutorials. diff --git a/doc/ovhcloud_baremetal.md b/doc/ovhcloud_baremetal.md index 10b285103..968f97429 100644 --- a/doc/ovhcloud_baremetal.md +++ b/doc/ovhcloud_baremetal.md @@ -34,6 +34,7 @@ Retrieve information and manage your Bare Metal services * [ovhcloud baremetal boot](ovhcloud_baremetal_boot.md) - Manage boot options for the given baremetal * [ovhcloud baremetal catalog](ovhcloud_baremetal_catalog.md) - List orderable servers, their availability and their price * [ovhcloud baremetal confirm-termination](ovhcloud_baremetal_confirm-termination.md) - Confirm the termination of the given baremetal +* [ovhcloud baremetal doctor](ovhcloud_baremetal_doctor.md) - Report what is wrong with a server, or with every server * [ovhcloud baremetal edit](ovhcloud_baremetal_edit.md) - Update the given baremetal * [ovhcloud baremetal get](ovhcloud_baremetal_get.md) - Retrieve information of a specific baremetal * [ovhcloud baremetal install-status](ovhcloud_baremetal_install-status.md) - Show how far the running installation of this baremetal has got diff --git a/doc/ovhcloud_baremetal_doctor.md b/doc/ovhcloud_baremetal_doctor.md new file mode 100644 index 000000000..447fd4de1 --- /dev/null +++ b/doc/ovhcloud_baremetal_doctor.md @@ -0,0 +1,64 @@ +## ovhcloud baremetal doctor + +Report what is wrong with a server, or with every server + +### Synopsis + +Check the things that silently break a dedicated server: a machine left on the +rescue system, monitoring switched off, hardware intervention refused, a renewal +that will not happen, work still running, maintenance already planned. + +With no argument it checks every server of the account. + +The exit code stays 0 when findings are reported, because the command ran and +answered. Use --strict to make findings fail the command instead, which is what +a pipeline gating on it wants. + +--strict fails on a warning or a critical. A note never fails it: every server +renewing inside the next 30 days reports one, so a --strict that counted notes +would be red permanently, and a gate that is always red is read like no gate. +Narrowing with --filter narrows the gate too — 'severity=="critical"' fails only +on criticals. + +``` +ovhcloud baremetal doctor [service_name...] [flags] +``` + +### Options + +``` + --expiry-days int Report a server expiring within this many days (default 30) + --filter stringArray Filter results by any property using https://github.com/PaesslerAG/gval syntax + Examples: + --filter 'state=="running"' + --filter 'name=~"^my.*"' + --filter 'nested.property.subproperty>10' + --filter 'startDate>="2023-12-01"' + --filter 'name=~"something" && nbField>10' + -h, --help help for doctor + --strict Exit non-zero when a warning or a critical is reported (notes never fail it) +``` + +### Options inherited from parent commands + +``` + -d, --debug Activate debug mode (will log all HTTP requests details) + -e, --ignore-errors Ignore errors in API calls when it is not fatal to the execution + -o, --output string Output format: json, yaml, interactive, or a custom format expression (using https://github.com/PaesslerAG/gval syntax) + Examples: + --output json + --output yaml + --output interactive + --output 'id' (to extract a single field) + --output 'nested.field.subfield' (to extract a nested field) + --output '[id, "name"]' (to extract multiple fields as an array) + --output '{"newKey": oldKey, "otherKey": nested.field}' (to extract and rename fields in an object) + --output 'name+","+type' (to extract and concatenate fields in a string) + --output '(nbFieldA + nbFieldB) * 10' (to compute values from numeric fields) + --profile string Use a specific profile from the configuration file +``` + +### SEE ALSO + +* [ovhcloud baremetal](ovhcloud_baremetal.md) - Retrieve information and manage your Bare Metal services + diff --git a/doc/ovhcloud_logout.md b/doc/ovhcloud_logout.md new file mode 100644 index 000000000..c78e5c513 --- /dev/null +++ b/doc/ovhcloud_logout.md @@ -0,0 +1,46 @@ +## ovhcloud logout + +Revoke your API credentials and remove them from the configuration + +``` +ovhcloud logout [flags] +``` + +### Examples + +``` +ovhcloud logout +ovhcloud logout --yes +ovhcloud logout --profile work +``` + +### Options + +``` + -h, --help help for logout + -y, --yes Do not ask for confirmation +``` + +### Options inherited from parent commands + +``` + -d, --debug Activate debug mode (will log all HTTP requests details) + -e, --ignore-errors Ignore errors in API calls when it is not fatal to the execution + -o, --output string Output format: json, yaml, interactive, or a custom format expression (using https://github.com/PaesslerAG/gval syntax) + Examples: + --output json + --output yaml + --output interactive + --output 'id' (to extract a single field) + --output 'nested.field.subfield' (to extract a nested field) + --output '[id, "name"]' (to extract multiple fields as an array) + --output '{"newKey": oldKey, "otherKey": nested.field}' (to extract and rename fields in an object) + --output 'name+","+type' (to extract and concatenate fields in a string) + --output '(nbFieldA + nbFieldB) * 10' (to compute values from numeric fields) + --profile string Use a specific profile from the configuration file +``` + +### SEE ALSO + +* [ovhcloud](ovhcloud.md) - CLI to manage your OVHcloud services + diff --git a/internal/cmd/baremetal.go b/internal/cmd/baremetal.go index 361d82825..604f2d5fa 100644 --- a/internal/cmd/baremetal.go +++ b/internal/cmd/baremetal.go @@ -11,7 +11,6 @@ import ( "github.com/ovh/ovhcloud-cli/internal/completion" "github.com/ovh/ovhcloud-cli/internal/flags" "github.com/ovh/ovhcloud-cli/internal/services/baremetal" - "github.com/ovh/ovhcloud-cli/internal/services/common" "github.com/ovh/ovhcloud-cli/internal/services/vrack" "github.com/spf13/cobra" ) @@ -51,8 +50,8 @@ func init() { editBaremetalCmd.Flags().IntVar(&baremetal.EditBaremetalParams.BootId, "boot-id", 0, "Boot ID") editBaremetalCmd.Flags().StringVar(&baremetal.EditBaremetalParams.BootScript, "boot-script", "", "Boot script") editBaremetalCmd.Flags().StringVar(&baremetal.EditBaremetalParams.EfiBootloaderPath, "efi-bootloader-path", "", "EFI bootloader path") - editBaremetalCmd.Flags().BoolVar(&baremetal.EditBaremetalParams.Monitoring, "monitoring", false, "Enable monitoring") - editBaremetalCmd.Flags().BoolVar(&baremetal.EditBaremetalParams.NoIntervention, "no-intervention", false, "Disable interventions") + editBaremetalCmd.Flags().BoolVar(&baremetal.EditBaremetalMonitoring, "monitoring", false, "Enable monitoring") + editBaremetalCmd.Flags().BoolVar(&baremetal.EditBaremetalNoIntervention, "no-intervention", false, "Disable interventions") editBaremetalCmd.Flags().StringVar(&baremetal.EditBaremetalParams.RescueMail, "rescue-mail", "", "Rescue mail") editBaremetalCmd.Flags().StringVar(&baremetal.EditBaremetalParams.RescueSshKey, "rescue-ssh-key", "", "Rescue SSH key") editBaremetalCmd.Flags().StringVar(&baremetal.EditBaremetalParams.RootDevice, "root-device", "", "Root device") @@ -124,7 +123,7 @@ they are not sold from the public price list, and show as "on quotation".`, ValidArgsFunction: completion.ServiceList("/v1/dedicated/server"), Run: baremetal.EditBaremetalServiceInfo, } - common.AddServiceInfoRenewFlags(baremetalServiceInfoEditCmd) + addServiceInfoRenewFlags(baremetalServiceInfoEditCmd) addInteractiveEditorFlag(baremetalServiceInfoEditCmd) baremetalServiceInfoCmd.AddCommand(baremetalServiceInfoEditCmd) @@ -565,6 +564,36 @@ a server is sitting on the power-off entry.`, Run: baremetal.BaremetalResetIPMISessions, }) + // What quietly breaks a dedicated server is spread across five routes and + // none of them is where somebody would look. This reads them together. + baremetalDoctorCmd := &cobra.Command{ + Use: "doctor [service_name...]", + Short: "Report what is wrong with a server, or with every server", + Long: `Check the things that silently break a dedicated server: a machine left on the +rescue system, monitoring switched off, hardware intervention refused, a renewal +that will not happen, work still running, maintenance already planned. + +With no argument it checks every server of the account. + +The exit code stays 0 when findings are reported, because the command ran and +answered. Use --strict to make findings fail the command instead, which is what +a pipeline gating on it wants. + +--strict fails on a warning or a critical. A note never fails it: every server +renewing inside the next 30 days reports one, so a --strict that counted notes +would be red permanently, and a gate that is always red is read like no gate. +Narrowing with --filter narrows the gate too — 'severity=="critical"' fails only +on criticals.`, + Args: cobra.ArbitraryArgs, + ValidArgsFunction: completion.ServiceList("/v1/dedicated/server"), + Run: baremetal.Doctor, + } + baremetalDoctorCmd.Flags().IntVar(&baremetal.DoctorExpiryDays, "expiry-days", 30, + "Report a server expiring within this many days") + baremetalDoctorCmd.Flags().BoolVar(&baremetal.DoctorStrict, "strict", false, + "Exit non-zero when a warning or a critical is reported (notes never fail it)") + baremetalCmd.AddCommand(withFilterFlag(baremetalDoctorCmd)) + // Nine backup routes, none of them reachable: the space included with the // server, the access list that guards it, the two passwords and the cloud // backup beside it. diff --git a/internal/cmd/baremetal_doctor_test.go b/internal/cmd/baremetal_doctor_test.go new file mode 100644 index 000000000..9908e49e2 --- /dev/null +++ b/internal/cmd/baremetal_doctor_test.go @@ -0,0 +1,298 @@ +// SPDX-FileCopyrightText: 2025 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +package cmd_test + +import ( + "time" + + "fmt" + "net/http" + + "github.com/jarcoal/httpmock" + "github.com/maxatome/go-testdeep/td" + "github.com/ovh/ovhcloud-cli/internal/cmd" + "github.com/ovh/ovhcloud-cli/internal/display" +) + +const doctorServer = "https://eu.api.ovh.com/v1/dedicated/server/ns1.example" + +// registerHealthyServer answers every route doctor reads with a server that has +// nothing wrong with it. +func registerHealthyServer() { + httpmock.RegisterResponder("GET", doctorServer, + httpmock.NewStringResponder(200, `{"state":"ok","powerState":"poweron","monitoring":true,"noIntervention":false,"bootId":1}`)) + httpmock.RegisterResponder("GET", doctorServer+"/boot/1", + httpmock.NewStringResponder(200, `{"bootId":1,"bootType":"harddisk","kernel":"hd","description":"Boot to disk"}`)) + httpmock.RegisterResponder("GET", doctorServer+"/serviceInfos", + httpmock.NewStringResponder(200, `{"expiration":"2099-01-01","renew":{"automatic":true,"deleteAtExpiration":false}}`)) + httpmock.RegisterResponder("GET", doctorServer+"/task?status=doing", + httpmock.NewStringResponder(200, `[]`)) + httpmock.RegisterResponder("GET", doctorServer+"/plannedIntervention", + httpmock.NewStringResponder(200, `[]`)) +} + +func (ms *MockSuite) TestBaremetalDoctorSaysWhenNothingIsWrong(assert, require *td.T) { + registerHealthyServer() + + out, err := cmd.Execute("baremetal", "doctor", "ns1.example") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("Nothing to report")) +} + +// Six servers of the fleet measured were booted on a rescue system, and three +// of them carried bootType "internal" — the obvious check would have called +// them healthy. +func (ms *MockSuite) TestBaremetalDoctorCatchesTheRescueTypedInternal(assert, require *td.T) { + registerHealthyServer() + httpmock.RegisterResponder("GET", doctorServer, + httpmock.NewStringResponder(200, `{"state":"ok","powerState":"poweron","monitoring":true,"bootId":46371}`)) + httpmock.RegisterResponder("GET", doctorServer+"/boot/46371", + httpmock.NewStringResponder(200, `{"bootId":46371,"bootType":"internal","kernel":"rescue-customer","description":"Customer rescue system (Debian-10-based)[REMOVAL ON 2025-06-23]"}`)) + + out, err := cmd.Execute("baremetal", "doctor", "ns1.example") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("rescue")) + assert.Cmp(out, td.Contains("boot-image"), "the retired image is its own finding") +} + +// A server that could not be read is not a healthy server, and counting it as +// checked is the one mistake this command cannot afford. +func (ms *MockSuite) TestBaremetalDoctorRefusesToGiveACleanBillForAServerItCouldNotRead(assert, require *td.T) { + httpmock.RegisterResponder("GET", doctorServer, + httpmock.NewStringResponder(500, `{"class":"Server::InternalServerError","message":"Internal server error"}`)) + + _, err := cmd.Execute("baremetal", "doctor", "ns1.example") + + require.CmpError(err) + assert.Cmp(err.Error(), td.Contains("not a clean bill of health")) +} + +// `renew.automatic` comes back differently between consecutive reads of the +// same object. The finding says so rather than picking one of the answers. +func (ms *MockSuite) TestBaremetalDoctorReportsTheApiDisagreeingWithItself(assert, require *td.T) { + registerHealthyServer() + + answers := []string{ + `{"expiration":"2099-01-01","renew":{"automatic":true}}`, + `{"expiration":"2099-01-01","renew":{"automatic":false}}`, + } + var call int + httpmock.RegisterResponder("GET", doctorServer+"/serviceInfos", + func(*http.Request) (*http.Response, error) { + body := answers[call%len(answers)] + call++ + return httpmock.NewStringResponse(200, body), nil + }) + + out, err := cmd.Execute("baremetal", "doctor", "ns1.example") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("different answers")) + assert.Cmp(call >= 2, true, fmt.Sprintf("the field has to be read more than once, was read %d time(s)", call)) +} + +// The exit code stays 0 when findings are reported, because the command ran and +// answered. --strict is for the pipeline gating on it, and has to be asked for. +func (ms *MockSuite) TestBaremetalDoctorOnlyFailsWhenAskedTo(assert, require *td.T) { + registerHealthyServer() + httpmock.RegisterResponder("GET", doctorServer, + httpmock.NewStringResponder(200, `{"state":"ok","powerState":"poweron","monitoring":false,"bootId":1}`)) + + out, err := cmd.Execute("baremetal", "doctor", "ns1.example") + require.CmpNoError(err, "reporting a finding is not a failed command") + assert.Cmp(out, td.Contains("monitoring")) + + // --strict changes the exit code and nothing else. A second document after + // the table would be the only thing left under -o json, so the pipeline + // that asked for --strict would get the error instead of the findings. + var code *int + previous := display.ExitFunc + display.ExitFunc = func(c int) { code = &c } + defer func() { display.ExitFunc = previous }() + + strictOut, strictErr := cmd.Execute("baremetal", "doctor", "ns1.example", "--strict") + + require.CmpNoError(strictErr, "--strict must not emit a second document") + require.NotNil(code, "--strict has to set a non-zero exit code") + assert.Cmp(*code, 1) + assert.Cmp(strictOut, td.Contains("monitoring"), "the findings are still the output") + assert.Cmp(strictOut, td.Not(td.Contains("🛑")), "no error document is printed") +} + +// A note on its own must NOT fail --strict, and this is the case that decides +// whether the flag is usable at all: `expiry` reports a note for every server +// renewing inside the next 30 days, which on a real account is most of them, +// most of the time. Counting notes made --strict red permanently, and a gate +// that is always red is read exactly like no gate. +// +// The suite only ever exercised --strict against a warning, so nothing said +// what a note should do — which is why the behaviour could be wrong and green. +func (ms *MockSuite) TestBaremetalDoctorStrictIgnoresANoteOnItsOwn(assert, require *td.T) { + registerHealthyServer() + // Expiring inside the default 30-day window: a note, and nothing else. + httpmock.RegisterResponder("GET", doctorServer+"/serviceInfos", + httpmock.NewStringResponder(200, + `{"expiration":"`+time.Now().AddDate(0, 0, 7).Format("2006-01-02")+ + `","renew":{"automatic":true,"deleteAtExpiration":false}}`)) + + var code *int + previous := display.ExitFunc + display.ExitFunc = func(c int) { code = &c } + defer func() { display.ExitFunc = previous }() + + out, err := cmd.Execute("baremetal", "doctor", "ns1.example", "--strict") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("expires in"), "the note is still reported") + assert.Cmp(code, td.Nil(), "a note alone must not fail the gate") +} + +// The positive control of the test above: a warning alongside the same note +// still fails. Without it, never exiting at all would pass. +func (ms *MockSuite) TestBaremetalDoctorStrictStillFailsOnAWarningBesideANote(assert, require *td.T) { + registerHealthyServer() + httpmock.RegisterResponder("GET", doctorServer+"/serviceInfos", + httpmock.NewStringResponder(200, + `{"expiration":"`+time.Now().AddDate(0, 0, 7).Format("2006-01-02")+ + `","renew":{"automatic":true,"deleteAtExpiration":false}}`)) + httpmock.RegisterResponder("GET", doctorServer, + httpmock.NewStringResponder(200, + `{"state":"ok","powerState":"poweron","monitoring":false,"bootId":1}`)) + + var code *int + previous := display.ExitFunc + display.ExitFunc = func(c int) { code = &c } + defer func() { display.ExitFunc = previous }() + + out, err := cmd.Execute("baremetal", "doctor", "ns1.example", "--strict") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("monitoring")) + require.NotNil(code, "a warning must still fail the gate") + assert.Cmp(*code, 1) +} + +// A clean fleet under --strict exits 0. The no-finding case returns before the +// --strict block, so this pins the ORDER: moving the exit above that return +// would fail a green fleet for having been asked to check. +func (ms *MockSuite) TestBaremetalDoctorStrictStaysGreenWithNoFinding(assert, require *td.T) { + registerHealthyServer() + httpmock.RegisterResponder("GET", doctorServer, + httpmock.NewStringResponder(200, `{"state":"ok","powerState":"poweron","monitoring":true,"bootId":1}`)) + + exited := false + previous := display.ExitFunc + display.ExitFunc = func(int) { exited = true } + defer func() { display.ExitFunc = previous }() + + _, err := cmd.Execute("baremetal", "doctor", "ns1.example", "--strict") + + require.CmpNoError(err) + assert.Cmp(exited, false, "nothing to report means nothing to fail on") +} + +// The server itself reads fine and every check under it fails. This is the +// answer the command must never give: four dead checks used to print as a clean +// bill, and exit 0 under --strict, because four of the five reads discarded +// their error. +func (ms *MockSuite) TestBaremetalDoctorWillNotCallAServerHealthyOnFourDeadChecks(assert, require *td.T) { + registerHealthyServer() + broken := httpmock.NewStringResponder(500, `{"class":"Server::InternalServerError","message":"Internal server error"}`) + for _, route := range []string{"/boot/1", "/serviceInfos", "/task?status=doing", "/plannedIntervention"} { + httpmock.RegisterResponder("GET", doctorServer+route, broken) + } + + exited := false + previous := display.ExitFunc + display.ExitFunc = func(int) { exited = true } + defer func() { display.ExitFunc = previous }() + + out, err := cmd.Execute("baremetal", "doctor", "ns1.example", "--strict") + + require.CmpNoError(err) + assert.Cmp(out, td.Not(td.Contains("Nothing to report")), + "a check that did not run is not a check that found nothing") + assert.Cmp(exited, true, "and --strict must not gate green on it") + for _, check := range []string{"boot", "renewal", "tasks", "planned-intervention"} { + assert.Cmp(out, td.Contains(check), "%s is named as unchecked", check) + } +} + +// One failed read among five, on an otherwise perfect server. The three checks +// that did run still report, and the one that did not is still said. +func (ms *MockSuite) TestBaremetalDoctorNamesTheOneCheckThatCouldNotRun(assert, require *td.T) { + registerHealthyServer() + httpmock.RegisterResponder("GET", doctorServer+"/plannedIntervention", + httpmock.NewStringResponder(500, `{"message":"Internal server error"}`)) + + out, err := cmd.Execute("baremetal", "doctor", "ns1.example") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("planned-intervention")) + assert.Cmp(out, td.Contains("could not be run")) + assert.Cmp(out, td.Not(td.Contains("Nothing to report"))) +} + +// The renewal check reads the same object five times because the field +// disagrees with itself; three readings agree by chance about once in four. +// A short read is therefore not a weaker answer, it is no answer — and the old +// code decided on whatever it had managed to collect, which for a failure on +// the second read is a single coin toss. +func (ms *MockSuite) TestBaremetalDoctorRefusesToVoteOnAShortReadOfTheRenewal(assert, require *td.T) { + registerHealthyServer() + + reads := 0 + httpmock.RegisterResponder("GET", doctorServer+"/serviceInfos", + func(*http.Request) (*http.Response, error) { + reads++ + if reads > 1 { + return httpmock.NewStringResponse(500, `{"message":"Internal server error"}`), nil + } + // The one reading that lands says renewal is off. Deciding on it + // alone is exactly what must not happen. + return httpmock.NewStringResponse(200, + `{"expiration":"2099-01-01","renew":{"automatic":false}}`), nil + }) + + out, err := cmd.Execute("baremetal", "doctor", "ns1.example") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("could not be run"), "the check is reported as unrun") + assert.Cmp(out, td.Not(td.Contains("read as off")), + "and no verdict is reached on one reading out of five") +} + +// With no argument it checks every server of the account. +func (ms *MockSuite) TestBaremetalDoctorChecksTheWholeFleetByDefault(assert, require *td.T) { + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/v1/dedicated/server", + httpmock.NewStringResponder(200, `["ns1.example"]`)) + registerHealthyServer() + + out, err := cmd.Execute("baremetal", "doctor") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("1 server")) +} + +// Disagreement and absence are not the same reading, and they used to print the +// same sentence: "the API gave different answers across 5 reads" about a route +// that answered the same thing every time — nothing. That sends the operator +// looking for an instability that is not there. +func (ms *MockSuite) TestBaremetalDoctorTellsAbsenceFromDisagreement(assert, require *td.T) { + registerHealthyServer() + httpmock.RegisterResponder("GET", doctorServer+"/serviceInfos", + httpmock.NewStringResponder(200, `{"expiration":"2099-01-01","renew":{"deleteAtExpiration":false}}`)) + + out, err := cmd.Execute("baremetal", "doctor", "ns1.example") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("does not return renew.automatic")) + assert.Cmp(out, td.Not(td.Contains("gave different answers")), + "a field that is absent has not disagreed with itself") + assert.Cmp(out, td.Not(td.Contains("read as off")), + "and no verdict is reached on a field that was never returned") +} diff --git a/internal/cmd/baremetal_edit_bool_test.go b/internal/cmd/baremetal_edit_bool_test.go new file mode 100644 index 000000000..09c399368 --- /dev/null +++ b/internal/cmd/baremetal_edit_bool_test.go @@ -0,0 +1,95 @@ +// SPDX-FileCopyrightText: 2026 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +package cmd_test + +import ( + "encoding/json" + "io" + "net/http" + + "github.com/jarcoal/httpmock" + "github.com/maxatome/go-testdeep/td" + "github.com/ovh/ovhcloud-cli/internal/cmd" +) + +const editServer = "https://eu.api.ovh.com/v1/dedicated/server/ns1.example" + +// registerEditableServer answers the read EditResource does before its write, +// with both booleans on, and records the body of the PUT. +func registerEditableServer(body *map[string]any) { + httpmock.RegisterResponder("GET", editServer, + httpmock.NewStringResponder(200, + `{"name":"ns1.example","monitoring":true,"noIntervention":true,"state":"ok","rootDevice":null}`)) + httpmock.RegisterResponder("PUT", editServer, func(req *http.Request) (*http.Response, error) { + raw, _ := io.ReadAll(req.Body) + _ = json.Unmarshal(raw, body) + return httpmock.NewStringResponse(200, `null`), nil + }) +} + +// `omitempty` on a bool drops false, so turning a flag off sent nothing — +// and EditResource reads the object, merges the empty command-line map into it +// and PUTs the result, so the current true came straight back while the command +// printed "✅ Resource updated successfully". The doctor's remedy for a server +// refusing hardware intervention was exactly this call. +func (ms *MockSuite) TestBaremetalEditSendsAnExplicitFalse(assert, require *td.T) { + var body map[string]any + registerEditableServer(&body) + + _, err := cmd.Execute("baremetal", "edit", "ns1.example", "--no-intervention=false") + + require.CmpNoError(err) + require.NotNil(body["noIntervention"], "the field must be in the body at all") + assert.Cmp(body["noIntervention"], false, "and it must carry the false that was asked for") +} + +// Monitoring had the same defect and looked healthy, because the only caller +// that names it happens to pass true. +func (ms *MockSuite) TestBaremetalEditSendsAnExplicitFalseForMonitoring(assert, require *td.T) { + var body map[string]any + registerEditableServer(&body) + + _, err := cmd.Execute("baremetal", "edit", "ns1.example", "--monitoring=false") + + require.CmpNoError(err) + assert.Cmp(body["monitoring"], false) +} + +// The other half: a flag not typed must not appear, or every edit would rewrite +// fields it was never asked about. +func (ms *MockSuite) TestBaremetalEditLeavesUntypedBooleansAlone(assert, require *td.T) { + var body map[string]any + registerEditableServer(&body) + + _, err := cmd.Execute("baremetal", "edit", "ns1.example", "--boot-script", "hello") + + require.CmpNoError(err) + assert.Cmp(body["monitoring"], true, "the value read back, not one this command invented") + assert.Cmp(body["noIntervention"], true) +} + +// The wasm build keeps one process across invocations, so a pointer set by an +// earlier command is still set for the next one — and the flag's Changed bit is +// reset in between, so the "was it typed" question answers no while the pointer +// says yes. Two commands in one process, which is the shape that exposes it. +func (ms *MockSuite) TestBaremetalEditDoesNotCarryABooleanIntoTheNextRun(assert, require *td.T) { + var first map[string]any + registerEditableServer(&first) + + _, err := cmd.Execute("baremetal", "edit", "ns1.example", "--no-intervention=false") + require.CmpNoError(err) + require.Cmp(first["noIntervention"], false, "the first command asked for it") + + cmd.PostExecute() + + var second map[string]any + registerEditableServer(&second) + + _, err = cmd.Execute("baremetal", "edit", "ns1.example", "--boot-script", "hello") + + require.CmpNoError(err) + assert.Cmp(second["noIntervention"], true, + "the second command did not ask, so it must send back what it read") +} diff --git a/internal/cmd/dry_run_log_test.go b/internal/cmd/dry_run_log_test.go new file mode 100644 index 000000000..fcf9f15f3 --- /dev/null +++ b/internal/cmd/dry_run_log_test.go @@ -0,0 +1,68 @@ +// SPDX-FileCopyrightText: 2025 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +// Ces deux tests vivent dans leur propre fichier, et non dans +// baremetal_test.go, pour une raison mecanique : 23 branches en aval ajoutent +// elles aussi des tests juste apres TestBaremetalReinstallDryRun et dans le +// meme bloc d'imports. Les y placer a fait echouer les 23 merges d'un coup, sur +// une adjacence et non sur un desaccord. Un fichier neuf ne peut entrer en +// collision qu'avec un fichier de meme nom. + +package cmd_test + +import ( + "bytes" + "log" + + "github.com/jarcoal/httpmock" + "github.com/maxatome/go-testdeep/td" + "github.com/ovh/ovhcloud-cli/internal/cmd" +) + +// A dry run prints the payload once. It used to print it twice: the message +// carries it, and a log.Println just above the --dry-run 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 goes to stderr, so no assertion on stdout could ever have seen +// it — which is why it survived every green run. This one redirects the logger. +func (ms *MockSuite) TestBaremetalReinstallDryRunDoesNotLogTheParametersTwice(assert, require *td.T) { + httpmock.RegisterResponder("POST", "https://eu.api.ovh.com/v1/dedicated/server/fakeBaremetal/reinstall", + httpmock.NewStringResponder(200, `{"taskId": 123}`), + ) + var logged bytes.Buffer + previous := log.Writer() + log.SetOutput(&logged) + defer log.SetOutput(previous) + + out, err := cmd.Execute("baremetal", "reinstall", "fakeBaremetal", "--os", "debian12_64", "--dry-run") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains(`"operatingSystem": "debian12_64"`), + "the payload is still printed, once, as the message") + assert.Cmp(logged.String(), td.Not(td.Contains("Final parameters")), + "a dry run logs nothing: it sends nothing") +} + +// The positive control of the test above: a REAL run still logs what it is +// about to send. Without it, deleting the log line altogether would pass. +func (ms *MockSuite) TestBaremetalReinstallStillLogsTheParametersItSends(assert, require *td.T) { + httpmock.RegisterResponder("POST", "https://eu.api.ovh.com/v1/dedicated/server/fakeBaremetal/reinstall", + httpmock.NewStringResponder(200, `{"taskId": 123}`), + ) + var logged bytes.Buffer + previous := log.Writer() + log.SetOutput(&logged) + defer log.SetOutput(previous) + + _, err := cmd.Execute("baremetal", "reinstall", "fakeBaremetal", "--os", "debian12_64", "--yes") + + require.CmpNoError(err) + assert.Cmp(logged.String(), td.Contains("Final parameters")) + assert.Cmp(logged.String(), td.Contains(`"operatingSystem": "debian12_64"`)) +} diff --git a/internal/cmd/ip.go b/internal/cmd/ip.go index 934850875..737f34e1f 100644 --- a/internal/cmd/ip.go +++ b/internal/cmd/ip.go @@ -7,7 +7,6 @@ package cmd import ( "github.com/ovh/ovhcloud-cli/internal/assets" "github.com/ovh/ovhcloud-cli/internal/completion" - "github.com/ovh/ovhcloud-cli/internal/services/common" "github.com/ovh/ovhcloud-cli/internal/services/ip" "github.com/spf13/cobra" ) @@ -192,7 +191,7 @@ func init() { ValidArgsFunction: completion.ServiceList("/v1/ip/service"), Run: ip.EditIpServiceInfo, } - common.AddServiceInfoRenewFlags(ipServiceInfoEditCmd) + addServiceInfoRenewFlags(ipServiceInfoEditCmd) addInteractiveEditorFlag(ipServiceInfoEditCmd) ipServiceInfoCmd.AddCommand(ipServiceInfoEditCmd) diff --git a/internal/cmd/service_info.go b/internal/cmd/service_info.go new file mode 100644 index 000000000..e08a19d1b --- /dev/null +++ b/internal/cmd/service_info.go @@ -0,0 +1,28 @@ +// SPDX-FileCopyrightText: 2025 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +package cmd + +import ( + "github.com/ovh/ovhcloud-cli/internal/services/common" + "github.com/spf13/cobra" +) + +// addServiceInfoRenewFlags registers the renewal flags on a `service-info edit` +// command. +// +// It sits here, beside the other shared flag helpers, rather than in the +// service package: declaring cobra flags is the command layer's job, and +// internal/services/common was the only service package doing it. The flag +// names still come from common.ServiceInfoRenewFlags, which is also what the +// payload builder reads — one table, so the two halves cannot drift. +func addServiceInfoRenewFlags(cmd *cobra.Command) { + for _, flag := range common.ServiceInfoRenewFlags { + if flag.Period { + cmd.Flags().Int(flag.Name, 0, flag.Usage) + continue + } + cmd.Flags().Bool(flag.Name, false, flag.Usage) + } +} diff --git a/internal/cmd/vps.go b/internal/cmd/vps.go index f452ef0c6..f112381a0 100644 --- a/internal/cmd/vps.go +++ b/internal/cmd/vps.go @@ -11,7 +11,6 @@ import ( "github.com/ovh/ovhcloud-cli/internal/assets" "github.com/ovh/ovhcloud-cli/internal/completion" "github.com/ovh/ovhcloud-cli/internal/flags" - "github.com/ovh/ovhcloud-cli/internal/services/common" "github.com/ovh/ovhcloud-cli/internal/services/vps" "github.com/spf13/cobra" ) @@ -221,7 +220,7 @@ func init() { ValidArgsFunction: completion.ServiceList("/v1/vps"), Run: vps.EditVpsServiceInfo, } - common.AddServiceInfoRenewFlags(serviceInfoEditCmd) + addServiceInfoRenewFlags(serviceInfoEditCmd) addInteractiveEditorFlag(serviceInfoEditCmd) serviceInfoCmd.AddCommand(serviceInfoEditCmd) diff --git a/internal/cmd/webhosting.go b/internal/cmd/webhosting.go index 7e5096d0f..e98a7e7f7 100644 --- a/internal/cmd/webhosting.go +++ b/internal/cmd/webhosting.go @@ -10,7 +10,6 @@ import ( "strings" "github.com/ovh/ovhcloud-cli/internal/completion" - "github.com/ovh/ovhcloud-cli/internal/services/common" "github.com/ovh/ovhcloud-cli/internal/services/webhosting" "github.com/spf13/cobra" ) @@ -605,7 +604,7 @@ func init() { ValidArgsFunction: completion.ServiceList("/v1/hosting/web"), Run: webhosting.UpdateExtraSqlServiceInfo, } - common.AddServiceInfoRenewFlags(extraSQLServiceInfoUpdateCmd) + addServiceInfoRenewFlags(extraSQLServiceInfoUpdateCmd) addParameterFileFlags(extraSQLServiceInfoUpdateCmd, true, nil, "", "", "", nil) addInteractiveEditorFlag(extraSQLServiceInfoUpdateCmd) extraSQLServiceInfoCmd.AddCommand(extraSQLServiceInfoUpdateCmd) @@ -1326,7 +1325,7 @@ func init() { ValidArgsFunction: completion.ServiceList("/v1/hosting/web"), Run: webhosting.UpdateCdnServiceInfo, } - common.AddServiceInfoRenewFlags(cdnServiceInfoUpdateCmd) + addServiceInfoRenewFlags(cdnServiceInfoUpdateCmd) addParameterFileFlags(cdnServiceInfoUpdateCmd, true, nil, "", "", "", nil) addInteractiveEditorFlag(cdnServiceInfoUpdateCmd) cdnServiceInfoCmd.AddCommand(cdnServiceInfoUpdateCmd) @@ -1460,7 +1459,7 @@ func init() { ValidArgsFunction: completion.ServiceList("/v1/hosting/web"), Run: webhosting.UpdateServiceInfo, } - common.AddServiceInfoRenewFlags(serviceInfoUpdateCmd) + addServiceInfoRenewFlags(serviceInfoUpdateCmd) addParameterFileFlags(serviceInfoUpdateCmd, true, nil, "", "", "", nil) addInteractiveEditorFlag(serviceInfoUpdateCmd) serviceInfoCmd.AddCommand(serviceInfoUpdateCmd) @@ -1571,7 +1570,7 @@ func init() { ValidArgsFunction: completion.ServiceList("/v1/hosting/web"), Run: webhosting.UpdateLocalSeoLocationServiceInfo, } - common.AddServiceInfoRenewFlags(localSeoLocationServiceInfoUpdateCmd) + addServiceInfoRenewFlags(localSeoLocationServiceInfoUpdateCmd) addParameterFileFlags(localSeoLocationServiceInfoUpdateCmd, true, nil, "", "", "", nil) addInteractiveEditorFlag(localSeoLocationServiceInfoUpdateCmd) localSeoLocationServiceInfoCmd.AddCommand(localSeoLocationServiceInfoUpdateCmd) diff --git a/internal/display/display_wasm.go b/internal/display/display_wasm.go index 71137ff6c..3a6b7e398 100644 --- a/internal/display/display_wasm.go +++ b/internal/display/display_wasm.go @@ -170,6 +170,19 @@ func exitError(message string, params ...any) { ResultError = fmt.Errorf("🛑 "+message+"\n", params...) } +// ExitFunc is the wasm counterpart of the one in display.go. There is no +// process to leave in a browser, and calling os.Exit would take the whole Go +// runtime down with the page, so a non-zero code is reported the way every +// other failure is here: on ResultError, which is a separate global from +// ResultString. The output the command already produced therefore survives — +// which is the whole point of the callers that reach for this instead of +// OutputError. +var ExitFunc = func(code int) { + if code != 0 && ResultError == nil { + ResultError = fmt.Errorf("🛑 command exited with code %d\n", code) + } +} + func outputf(message string, params ...any) { valueOut := &OutputMessage{ Message: fmt.Sprintf(message, params...), diff --git a/internal/services/baremetal/baremetal.go b/internal/services/baremetal/baremetal.go index 7a1a964ac..03af7270e 100644 --- a/internal/services/baremetal/baremetal.go +++ b/internal/services/baremetal/baremetal.go @@ -64,17 +64,35 @@ var ( BaremetalIpmiIP string BaremetalIpmiSshKey string + // The two booleans are pointers, and the two plain bools below them are what + // cobra writes into. + // + // `omitempty` on a bool drops false, so `--monitoring=false` and + // `--no-intervention=false` sent nothing at all — and EditResource reads the + // object, merges the (empty) command-line map into it and PUTs the result, so + // the current value came straight back and the command answered "✅ Resource + // updated successfully". Turning a flag off was not a no-op, it was a no-op + // that claimed to have worked. On a pointer, omitempty drops only nil, so an + // explicit false travels. + // + // Monitoring had the same defect and looked fine: the only caller that names + // it, the doctor's remedy for monitoring being off, happens to pass true. EditBaremetalParams struct { BootId int `json:"bootId,omitempty"` BootScript string `json:"bootScript,omitempty"` EfiBootloaderPath string `json:"efiBootloaderPath,omitempty"` - Monitoring bool `json:"monitoring,omitempty"` - NoIntervention bool `json:"noIntervention,omitempty"` + Monitoring *bool `json:"monitoring,omitempty"` + NoIntervention *bool `json:"noIntervention,omitempty"` RescueMail string `json:"rescueMail,omitempty"` RescueSshKey string `json:"rescueSshKey,omitempty"` RootDevice string `json:"rootDevice,omitempty"` State string `json:"state,omitempty"` } + + // EditBaremetalMonitoring and EditBaremetalNoIntervention hold what cobra + // parsed; EditBaremetal decides whether it was asked for. + EditBaremetalMonitoring bool + EditBaremetalNoIntervention bool ) func ListBaremetal(_ *cobra.Command, _ []string) { @@ -126,6 +144,21 @@ func GetBaremetal(_ *cobra.Command, args []string) { } func EditBaremetal(cmd *cobra.Command, args []string) { + // A boolean flag has no absent value of its own, so the question "was it + // typed" is asked of cobra rather than of the value. + // + // Assigned both ways round, never only on the true branch: the wasm build + // keeps one process across invocations, and a pointer left over from an + // earlier command would send a value nobody asked for this time. + EditBaremetalParams.Monitoring = nil + EditBaremetalParams.NoIntervention = nil + if cmd.Flags().Changed("monitoring") { + EditBaremetalParams.Monitoring = &EditBaremetalMonitoring + } + if cmd.Flags().Changed("no-intervention") { + EditBaremetalParams.NoIntervention = &EditBaremetalNoIntervention + } + if err := common.EditResource( cmd, "/dedicated/server/{serviceName}", diff --git a/internal/services/baremetal/doctor.go b/internal/services/baremetal/doctor.go new file mode 100644 index 000000000..4e90c7a57 --- /dev/null +++ b/internal/services/baremetal/doctor.go @@ -0,0 +1,651 @@ +// SPDX-FileCopyrightText: 2025 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +package baremetal + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/url" + "sort" + "strings" + "time" + + "github.com/ovh/ovhcloud-cli/internal/display" + filtersLib "github.com/ovh/ovhcloud-cli/internal/filters" + "github.com/ovh/ovhcloud-cli/internal/flags" + httpLib "github.com/ovh/ovhcloud-cli/internal/http" + "github.com/spf13/cobra" + "golang.org/x/sync/errgroup" + "golang.org/x/sync/semaphore" +) + +// The things that quietly break a dedicated server are spread across five +// routes and none of them is where somebody would look. A machine left on the +// rescue system runs no service and says nothing; monitoring switched off means +// OVHcloud will not call when a disk dies; a renewal switched off means the +// server goes back at expiry. Each is one field, in a different object. +// +// `doctor` reads them together and answers the only question worth asking: +// is anything wrong with this server, or with any of them. +// +// Every command named in a Fix was checked against the command tree of this +// binary before being written down. The first draft sent operators to +// `set-boot`, `list-boots` and `list-planned-interventions`, none of which +// exist — the same defect an earlier lot of this audit shipped in its own +// error message, and the reason this note is here. + +var ( + // DoctorExpiryDays is how far ahead an expiry counts as imminent. + DoctorExpiryDays int + + // DoctorStrict makes the command exit non-zero when it finds something. + DoctorStrict bool +) + +// severity orders the findings, worst first. +type severity int + +const ( + critical severity = iota + warning + note +) + +func (s severity) String() string { + switch s { + case critical: + return "critical" + case warning: + return "warning" + default: + return "note" + } +} + +// finding is one thing that is wrong, on one server. +type finding struct { + Server string + Severity severity + Check string + Detail string + Fix string +} + +// diagnosis is everything read about one server, plus what it says. +type diagnosis struct { + Server string + Findings []finding + Err error +} + +// Doctor reports what is wrong with a server, or with every server. +func Doctor(_ *cobra.Command, args []string) { + servers := args + if len(servers) == 0 { + var all []string + if err := httpLib.Client.Get("/v1/dedicated/server", &all); err != nil { + display.OutputError(&flags.OutputFormatConfig, "failed to list your servers: %s", err) + return + } + servers = all + } + + if len(servers) == 0 { + display.OutputInfo(&flags.OutputFormatConfig, map[string]any{"servers": 0}, + "You have no dedicated server.") + return + } + + diagnoses, err := diagnoseAll(servers) + if err != nil { + display.OutputError(&flags.OutputFormatConfig, "%s", err) + return + } + + var ( + findings []finding + failed []string + ) + for _, d := range diagnoses { + if d.Err != nil { + failed = append(failed, fmt.Sprintf("%s (%s)", d.Server, d.Err)) + continue + } + findings = append(findings, d.Findings...) + } + + // A server that could not be read is not a healthy server. Counting it as + // checked would be the one mistake this command cannot afford: it exists to + // be trusted when it says nothing is wrong. + if len(failed) > 0 { + display.OutputError(&flags.OutputFormatConfig, + "could not check %d of %d servers, so this is not a clean bill of health:\n %s", + len(failed), len(servers), strings.Join(failed, "\n ")) + return + } + + if len(findings) == 0 { + display.OutputInfo(&flags.OutputFormatConfig, + map[string]any{"servers": len(servers), "findings": []any{}}, + "✅ Nothing to report on %d server(s).", len(servers)) + return + } + + sortFindings(findings) + + rows := make([]map[string]any, 0, len(findings)) + for _, f := range findings { + rows = append(rows, map[string]any{ + "server": f.Server, + "severity": f.Severity.String(), + "check": f.Check, + "detail": f.Detail, + "fix": f.Fix, + }) + } + + filtered, err := filtersLib.FilterLines(rows, flags.GenericFilters) + if err != nil { + display.OutputError(&flags.OutputFormatConfig, "failed to filter results: %s", err) + return + } + + display.RenderTable(filtered, + []string{"server", "severity", "check", "detail", "fix"}, + &flags.OutputFormatConfig) + + // The default exit code stays 0 because a command that ran and answered did + // not fail, and every other command of this CLI reads a non-zero code that + // way. --strict is for the pipeline that wants the answer as a gate, and it + // has to be asked for rather than discovered. + // + // It changes the exit code and nothing else. Calling OutputError here would + // print a second document after the table, and under -o json only the last + // one survives -- the pipeline that asked for --strict would receive the + // error message instead of the findings it came for. + // A note never fails the gate, and that is the whole point of having three + // severities. `expiry` reports a note for every server renewing inside the + // next 30 days, which on a real account is most of them, most of the time: + // a --strict that counted notes would be red permanently, and a gate that + // is always red is read exactly like no gate at all. + // + // The filtered rows are what the caller sees, so they are what the exit + // code answers for: --filter 'severity=="critical"' narrows the gate too, + // instead of failing on rows the caller asked not to see. + if DoctorStrict && worseThanNote(filtered) { + display.ExitFunc(1) + } +} + +// worseThanNote reports whether any displayed row is a warning or a critical. +func worseThanNote(rows []map[string]any) bool { + for _, row := range rows { + switch row["severity"] { + case critical.String(), warning.String(): + return true + } + } + return false +} + +// diagnoseAll checks every server, ten at a time. +func diagnoseAll(servers []string) ([]diagnosis, error) { + const parallelChecks = 10 + + var ( + sem = semaphore.NewWeighted(parallelChecks) + diagnoses = make([]diagnosis, len(servers)) + group, ctx = errgroup.WithContext(context.Background()) + ) + + for i, server := range servers { + if err := sem.Acquire(ctx, 1); err != nil { + return nil, fmt.Errorf("failed to check your servers: %w", err) + } + + group.Go(func() error { + defer sem.Release(1) + diagnoses[i] = diagnose(server) + return nil + }) + } + + if err := group.Wait(); err != nil { + return nil, err + } + + return diagnoses, nil +} + +// diagnose reads one server and reports what is wrong with it. +// +// Five reads. The first one is fatal — without the server object there is +// nothing to check — and the other four each answer for one check. A check +// whose read fails does not disappear: it becomes a finding of its own. +// +// It used to disappear. Four of the five reads discarded their error, so a +// server whose boot entry, renewal, running tasks and planned interventions had +// all failed to read printed "✅ Nothing to report" — the one answer this +// command must never give wrongly, and the exact opposite of the rule it states +// about a server it could not read. Under --strict it also exited 0, so a +// pipeline built on it read four dead checks as a green gate. +func diagnose(server string) diagnosis { + escaped := url.PathEscape(server) + + var detail map[string]any + if err := httpLib.Client.Get(fmt.Sprintf("/v1/dedicated/server/%s", escaped), &detail); err != nil { + return diagnosis{Server: server, Err: err} + } + + found := checkState(server, detail) + found = append(found, checkMonitoring(server, detail)...) + found = append(found, checkIntervention(server, detail)...) + + var boot map[string]any + if bootID := bootIdentifier(detail); bootID == 0 { + found = append(found, uncheckable(server, "boot", + errors.New("the server object carries no boot identifier"))) + } else if err := httpLib.Client.Get( + fmt.Sprintf("/v1/dedicated/server/%s/boot/%d", escaped, bootID), &boot); err != nil { + found = append(found, uncheckable(server, "boot", err)) + } else { + found = append(found, checkBoot(server, boot)...) + } + + // A short read is not a weaker answer here, it is no answer: the renewal + // check needs its five readings because this field disagrees with itself, + // and three agree by chance about once in four. Deciding on one reading + // would be the coin toss the check exists to refuse. + if readings, err := readServiceInfos(escaped, renewalReadings); err != nil { + found = append(found, uncheckable(server, "renewal", err)) + } else { + found = append(found, checkRenewal(server, readings)...) + } + + var running []int64 + if err := httpLib.Client.Get( + fmt.Sprintf("/v1/dedicated/server/%s/task?status=doing", escaped), &running); err != nil { + found = append(found, uncheckable(server, "tasks", err)) + } else { + found = append(found, checkTasks(server, running)...) + } + + var planned []int64 + if err := httpLib.Client.Get( + fmt.Sprintf("/v1/dedicated/server/%s/plannedIntervention", escaped), &planned); err != nil { + found = append(found, uncheckable(server, "planned-intervention", err)) + } else { + found = append(found, checkPlannedIntervention(server, planned)...) + } + + return diagnosis{Server: server, Findings: found} +} + +// uncheckable turns a check that could not be run into a finding. +// +// A finding rather than a counter, so that it travels the same way as +// everything else: it sorts with the rest, it is in -o json, --filter reaches +// it, and it makes the finding list non-empty — which is what mechanically +// removes "Nothing to report" and, under --strict, the zero exit code. Nothing +// here has to remember to check a second list. +// +// warning and not critical: a check that did not run is not evidence of a +// problem. It is the withdrawal of the guarantee, and the guarantee is the +// whole product of this command. +func uncheckable(server, check string, err error) finding { + return finding{ + Server: server, Severity: warning, Check: check, + Detail: fmt.Sprintf("this check could not be run, so nothing here answers for it: %s", err), + Fix: fmt.Sprintf("ovhcloud baremetal doctor %s, once the API answers again", server), + } +} + +// checkBoot reports a server that will not come up on its own disk. +// +// The obvious test — bootType == "rescue" — misses half of them. On the account +// measured, six servers of thirty-five were booted on a rescue system: three +// carried bootType "rescue", and three carried bootType "internal" on a boot +// entry whose kernel is "rescue-customer" and whose description ends with +// "[REMOVAL ON 2025-06-23]". The type is the API's, and it is wrong for that +// entry; the kernel is not. So both are read. +func checkBoot(server string, boot map[string]any) []finding { + bootType := stringValue(boot, "bootType") + kernel := stringValue(boot, "kernel") + description := stringValue(boot, "description") + + switch { + case bootType == "harddisk": + return nil + + case bootType == "power": + return []finding{{ + Server: server, Severity: critical, Check: "boot", + Detail: fmt.Sprintf("next boot is %q, so the server will not come back up", description), + Fix: fmt.Sprintf("ovhcloud baremetal boot list %s, then boot set", server), + }} + + case isRescueBoot(bootType, kernel): + found := []finding{{ + Server: server, Severity: warning, Check: "boot", + Detail: fmt.Sprintf("booted on the rescue system (%s), so it is not running its own OS", kernel), + Fix: fmt.Sprintf("ovhcloud baremetal boot list %s, then boot set + reboot", server), + }} + + // The rescue image itself can be past its removal date. Three servers + // of the fleet measured sat on one announced for removal fourteen + // months earlier. + if strings.Contains(description, "REMOVAL ON") { + found = append(found, finding{ + Server: server, Severity: warning, Check: "boot-image", + Detail: fmt.Sprintf("its rescue image is retired: %q", description), + Fix: fmt.Sprintf("ovhcloud baremetal boot list %s", server), + }) + } + + return found + + default: + return []finding{{ + Server: server, Severity: note, Check: "boot", + Detail: fmt.Sprintf("boots on %q rather than its disk (%s)", bootType, description), + Fix: fmt.Sprintf("ovhcloud baremetal boot list %s", server), + }} + } +} + +// isRescueBoot answers whether a boot entry is a rescue system, by its type or +// by its kernel. See checkBoot for why one of the two is not enough. +func isRescueBoot(bootType, kernel string) bool { + return bootType == "rescue" || strings.HasPrefix(kernel, "rescue") +} + +// checkState reports a server the API does not consider healthy. +func checkState(server string, detail map[string]any) []finding { + var found []finding + + if state := stringValue(detail, "state"); state != "" && state != "ok" { + found = append(found, finding{ + Server: server, Severity: critical, Check: "state", + Detail: fmt.Sprintf("the API reports this server as %q", state), + Fix: "ovhcloud support-tickets create", + }) + } + + if power := stringValue(detail, "powerState"); power != "" && power != "poweron" { + found = append(found, finding{ + Server: server, Severity: critical, Check: "power", + Detail: fmt.Sprintf("the server is %s", power), + Fix: fmt.Sprintf("ovhcloud baremetal reboot %s", server), + }) + } + + return found +} + +// checkMonitoring reports a server nobody will be called about. +func checkMonitoring(server string, detail map[string]any) []finding { + if monitoring, set := detail["monitoring"].(bool); set && !monitoring { + return []finding{{ + Server: server, Severity: warning, Check: "monitoring", + Detail: "monitoring is off, so OVHcloud will not raise an alert if this server stops answering", + Fix: fmt.Sprintf("ovhcloud baremetal edit %s --monitoring", server), + }} + } + + return nil +} + +// checkIntervention reports a server whose hardware will not be touched. +func checkIntervention(server string, detail map[string]any) []finding { + if refused, set := detail["noIntervention"].(bool); set && refused { + return []finding{{ + Server: server, Severity: warning, Check: "intervention", + Detail: "hardware intervention is refused, so a failed disk will not be replaced without asking first", + Fix: fmt.Sprintf("ovhcloud baremetal edit %s --no-intervention=false", server), + }} + } + + return nil +} + +// renewalReadings is how many times the renewal state is read. +// +// Once is not enough, and that is measured rather than defensive: +// `renew.automatic` on this route comes back differently between consecutive +// reads of the same object. Twenty sequential reads of five servers gave +// 10/10, 9/11, 8/12, 6/14 and 5/15 splits between true and false, with the +// `domain` field proving each answer belonged to the server asked about; three +// other servers answered the same value twenty times out of twenty, and +// `monitoring` on the server object was stable ten out of ten. So it is that +// one field, on that one route, and a health check built on a single read of +// it reports a coin toss. +// +// Five rather than three: on a field behaving like a fair coin, three readings +// agree by chance once in four, and a health check wrong one time in four is +// not one anybody should act on. Five brings that to about one in sixteen, and +// what is reported when they do agree says how it was established rather than +// claiming certainty this route cannot give. +const renewalReadings = 5 + +// readServiceInfos reads the billing object several times. +func readServiceInfos(escaped string, times int) ([]map[string]any, error) { + readings := make([]map[string]any, 0, times) + + for range times { + var infos map[string]any + if err := httpLib.Client.Get( + fmt.Sprintf("/v1/dedicated/server/%s/serviceInfos", escaped), &infos); err != nil { + // The readings gathered so far are returned with the error, but the + // error is the answer: a vote decided on fewer ballots than it needs + // is not a smaller vote, it is a different one. + return readings, err + } + readings = append(readings, infos) + } + + return readings, nil +} + +// checkRenewal reports a server on its way out — or the fact that the API will +// not say whether it is. +func checkRenewal(server string, readings []map[string]any) []finding { + var found []finding + + infos := readings[len(readings)-1] + renew, _ := infos["renew"].(map[string]any) + expiration := stringValue(infos, "expiration") + + switch automatic, verdict := agreedRenewal(readings); { + case verdict == renewalDisagreed: + // Reporting "renewal is off" from one of these readings would be a coin + // toss, and reporting nothing would hide that nobody can tell. What is + // certain is that the answer is not trustworthy, and that is the + // finding. + found = append(found, finding{ + Server: server, Severity: warning, Check: "renewal", + Detail: fmt.Sprintf("the API gave different answers about automatic renewal across %d reads, so whether this server renews on %s cannot be established from here", + len(readings), expiration), + Fix: fmt.Sprintf("ovhcloud baremetal service-info get %s (and check the Manager)", server), + }) + + case verdict == renewalAbsent: + // Not the same thing at all, and it used to print as the case above. + found = append(found, finding{ + Server: server, Severity: warning, Check: "renewal", + Detail: fmt.Sprintf("this route does not return renew.automatic for this server, so whether it renews on %s cannot be read here", + expiration), + Fix: fmt.Sprintf("ovhcloud baremetal service-info get %s (and check the Manager)", server), + }) + + case !automatic: + found = append(found, finding{ + Server: server, Severity: warning, Check: "renewal", + Detail: fmt.Sprintf("automatic renewal read as off %d times out of %d, and this server expires on %s — this field is unreliable on this route, so confirm before acting", + len(readings), len(readings), expiration), + Fix: fmt.Sprintf("ovhcloud baremetal service-info edit %s --renew-automatic", server), + }) + } + + if deleteAtExpiration, set := renew["deleteAtExpiration"].(bool); set && deleteAtExpiration { + found = append(found, finding{ + Server: server, Severity: critical, Check: "renewal", + Detail: fmt.Sprintf("this server is set to be deleted when it expires on %s", expiration), + Fix: fmt.Sprintf("ovhcloud baremetal service-info edit %s --renew-delete-at-expiration=false", server), + }) + } + + if days, ok := daysUntil(expiration); ok && days <= int64(DoctorExpiryDays) { + severity := note + if automatic, verdict := agreedRenewal(readings); verdict == renewalAgreed && !automatic { + severity = warning + } + found = append(found, finding{ + Server: server, Severity: severity, Check: "expiry", + Detail: fmt.Sprintf("expires in %d day(s), on %s", days, expiration), + Fix: fmt.Sprintf("ovhcloud baremetal service-info get %s", server), + }) + } + + return found +} + +// agreedRenewal answers what the readings say about automatic renewal, and +// whether they agree at all. See renewalReadings for why the question has to be +// asked that way. +// renewalVerdict is what several reads of the same field add up to. +// +// Three outcomes and not two: the readings can agree, they can disagree, and +// the field can simply not be there. Folding the last two together made the +// finding say "the API gave different answers across 5 reads" about a route +// that had answered the same thing every time — namely nothing. That sends the +// operator to look for an instability that does not exist. +type renewalVerdict int + +const ( + renewalAgreed renewalVerdict = iota + renewalDisagreed + renewalAbsent +) + +func agreedRenewal(readings []map[string]any) (bool, renewalVerdict) { + var ( + value bool + known bool + ) + + for _, infos := range readings { + renew, _ := infos["renew"].(map[string]any) + automatic, set := renew["automatic"].(bool) + if !set { + return false, renewalAbsent + } + + if !known { + value, known = automatic, true + continue + } + + if automatic != value { + return false, renewalDisagreed + } + } + + if !known { + // No readings at all: nothing was measured, so nothing is known. + return false, renewalAbsent + } + + return value, renewalAgreed +} + +// checkTasks reports work still running on the server, because most of the +// other checks read a state that is about to change. +func checkTasks(server string, running []int64) []finding { + if len(running) == 0 { + return nil + } + + return []finding{{ + Server: server, Severity: note, Check: "tasks", + Detail: fmt.Sprintf("%d task(s) still running, so this server is being changed right now", len(running)), + Fix: fmt.Sprintf("ovhcloud baremetal list-tasks %s", server), + }} +} + +// checkPlannedIntervention reports maintenance already scheduled. +func checkPlannedIntervention(server string, planned []int64) []finding { + if len(planned) == 0 { + return nil + } + + return []finding{{ + Server: server, Severity: warning, Check: "planned-intervention", + Detail: fmt.Sprintf("%d intervention(s) planned on this server", len(planned)), + Fix: fmt.Sprintf("ovhcloud baremetal list-interventions %s", server), + }} +} + +// sortFindings puts the worst first, then groups by server so one machine's +// problems are read together. +func sortFindings(findings []finding) { + sort.SliceStable(findings, func(i, j int) bool { + if findings[i].Severity != findings[j].Severity { + return findings[i].Severity < findings[j].Severity + } + if findings[i].Server != findings[j].Server { + return findings[i].Server < findings[j].Server + } + return findings[i].Check < findings[j].Check + }) +} + +// daysUntil answers how many days are left before a date, and whether the date +// could be read at all. +// +// An unreadable date reports "no" rather than zero: zero would mean "expires +// today" and raise an alarm about a field nobody managed to parse. +func daysUntil(date string) (int64, bool) { + if date == "" { + return 0, false + } + + for _, layout := range []string{"2006-01-02", time.RFC3339} { + if parsed, err := time.Parse(layout, date); err == nil { + return int64(time.Until(parsed).Hours() / 24), true + } + } + + return 0, false +} + +func stringValue(object map[string]any, key string) string { + value, _ := object[key].(string) + return value +} + +// bootIdentifier reads the active boot entry whatever shape the decoder left +// it in. go-ovh decodes with UseNumber, so this arrives as a json.Number and +// never as a float64 — a type switch that only knew float64 turned a vRack +// branch into dead code once in this CLI. +func bootIdentifier(detail map[string]any) int64 { + switch value := detail["bootId"].(type) { + case json.Number: + parsed, err := value.Int64() + if err != nil { + return 0 + } + return parsed + case float64: + return int64(value) + case int64: + return value + case int: + return int64(value) + } + + return 0 +} diff --git a/internal/services/baremetal/doctor_test.go b/internal/services/baremetal/doctor_test.go new file mode 100644 index 000000000..2a4e572f6 --- /dev/null +++ b/internal/services/baremetal/doctor_test.go @@ -0,0 +1,211 @@ +// SPDX-FileCopyrightText: 2025 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +package baremetal + +import ( + "strings" + "testing" +) + +// The obvious test — bootType == "rescue" — misses half of them. On the fleet +// measured, six servers of thirty-five were booted on a rescue system: three +// carried bootType "rescue", and three carried bootType "internal" on an entry +// whose kernel is "rescue-customer". The type is the API's, and it is wrong for +// that entry; the kernel is not. +func TestARescueBootIsRecognisedByItsKernelToo(t *testing.T) { + if !isRescueBoot("rescue", "rescue12-customer") { + t.Fatal("the current rescue is typed rescue") + } + if !isRescueBoot("internal", "rescue-customer") { + t.Fatal("the retired rescue is typed internal and would otherwise be missed") + } + if isRescueBoot("harddisk", "hd") { + t.Fatal("booting from the disk is not a rescue") + } + if isRescueBoot("internal", "something-else") { + t.Fatal("internal alone does not make a rescue") + } +} + +func TestBootFromDiskReportsNothing(t *testing.T) { + if found := checkBoot("ns1", map[string]any{ + "bootType": "harddisk", "kernel": "hd", "description": "Boot to disk", + }); len(found) != 0 { + t.Fatalf("a server booting from its disk is healthy, got %d finding(s)", len(found)) + } +} + +// A boot entry of type "power" with the poweroff kernel exists in the list every +// server is offered. A server sitting on it does not come back up. +func TestABootThatPowersOffIsCritical(t *testing.T) { + found := checkBoot("ns1", map[string]any{ + "bootType": "power", "kernel": "poweroff", "description": "Power-off server", + }) + if len(found) != 1 || found[0].Severity != critical { + t.Fatalf("expected one critical finding, got %+v", found) + } +} + +// Three servers of the fleet sat on a rescue image whose removal had been +// announced fourteen months earlier. +func TestARetiredRescueImageIsReportedSeparately(t *testing.T) { + found := checkBoot("ns1", map[string]any{ + "bootType": "internal", + "kernel": "rescue-customer", + "description": "Customer rescue system (Debian-10-based)[REMOVAL ON 2025-06-23]", + }) + + if len(found) != 2 { + t.Fatalf("expected the rescue and its retired image, got %+v", found) + } + checks := found[0].Check + "," + found[1].Check + if !strings.Contains(checks, "boot-image") { + t.Fatalf("the retired image should be its own finding, got %s", checks) + } +} + +// `renew.automatic` comes back differently between consecutive reads of the +// same object — measured 10/10, 9/11, 8/12, 6/14 and 5/15 over twenty reads of +// five servers. Reporting "renewal is off" from readings that disagree is a +// coin toss; what is certain is that nobody can tell, and that is the finding. +func TestDisagreeingReadingsAreReportedAsSuch(t *testing.T) { + readings := []map[string]any{ + {"renew": map[string]any{"automatic": true}, "expiration": "2026-09-01"}, + {"renew": map[string]any{"automatic": false}, "expiration": "2026-09-01"}, + {"renew": map[string]any{"automatic": true}, "expiration": "2026-09-01"}, + } + + found := checkRenewal("ns1", readings) + if len(found) == 0 { + t.Fatal("readings that disagree must not be silently dropped") + } + if !strings.Contains(found[0].Detail, "different answers") { + t.Fatalf("the finding should say the API disagreed with itself, got %q", found[0].Detail) + } + if strings.Contains(found[0].Detail, "is off") { + t.Fatalf("it must not assert a value it could not establish, got %q", found[0].Detail) + } +} + +func TestAgreedReadingsDecide(t *testing.T) { + off := []map[string]any{ + {"renew": map[string]any{"automatic": false}}, + {"renew": map[string]any{"automatic": false}}, + } + if value, verdict := agreedRenewal(off); verdict != renewalAgreed || value { + t.Fatalf("readings that agree on false decide false, got value=%v verdict=%v", value, verdict) + } + + on := []map[string]any{ + {"renew": map[string]any{"automatic": true}}, + {"renew": map[string]any{"automatic": true}}, + } + if value, verdict := agreedRenewal(on); verdict != renewalAgreed || !value { + t.Fatalf("readings that agree on true decide true, got value=%v verdict=%v", value, verdict) + } + + // Three outcomes, not two: an absent field is neither an agreement nor a + // disagreement, and calling it the latter made the finding describe an + // instability that had not happened. + if _, verdict := agreedRenewal([]map[string]any{{"renew": map[string]any{}}}); verdict != renewalAbsent { + t.Fatalf("an absent field is absent, not %v", verdict) + } + + mixed := []map[string]any{ + {"renew": map[string]any{"automatic": true}}, + {"renew": map[string]any{"automatic": false}}, + } + if _, verdict := agreedRenewal(mixed); verdict != renewalDisagreed { + t.Fatalf("readings that differ disagree, got %v", verdict) + } + + if _, verdict := agreedRenewal(nil); verdict != renewalAbsent { + t.Fatalf("no reading at all establishes nothing, got %v", verdict) + } +} + +// A server whose renewal reads as off, five times, is still reported with what +// established it: this route cannot give certainty, and the finding must not +// pretend otherwise. +func TestAnAgreedOffReadingSaysHowItWasEstablished(t *testing.T) { + readings := make([]map[string]any, 5) + for i := range readings { + readings[i] = map[string]any{ + "renew": map[string]any{"automatic": false}, "expiration": "2026-09-01", + } + } + + found := checkRenewal("ns1", readings) + if len(found) == 0 { + t.Fatal("a renewal that reads as off must be reported") + } + if !strings.Contains(found[0].Detail, "5 times out of 5") { + t.Fatalf("the finding should say how it was established, got %q", found[0].Detail) + } + if !strings.Contains(found[0].Detail, "confirm before acting") { + t.Fatalf("the finding should carry its own uncertainty, got %q", found[0].Detail) + } +} + +// A server set to be deleted at expiry is the one renewal finding that is not +// in doubt: that field was stable across every reading. +func TestDeletionAtExpiryIsCritical(t *testing.T) { + found := checkRenewal("ns1", []map[string]any{ + {"renew": map[string]any{"automatic": true, "deleteAtExpiration": true}, + "expiration": "2026-09-01"}, + }) + + var seen bool + for _, f := range found { + if f.Severity == critical && strings.Contains(f.Detail, "deleted") { + seen = true + } + } + if !seen { + t.Fatalf("expected a critical finding about the deletion, got %+v", found) + } +} + +func TestMonitoringOffIsReported(t *testing.T) { + if found := checkMonitoring("ns1", map[string]any{"monitoring": false}); len(found) != 1 { + t.Fatal("monitoring off means nobody is called when the server dies") + } + if found := checkMonitoring("ns1", map[string]any{"monitoring": true}); len(found) != 0 { + t.Fatal("monitoring on is not a finding") + } + // An absent field is not a false one: it would report every server of an + // account whose API stopped sending it. + if found := checkMonitoring("ns1", map[string]any{}); len(found) != 0 { + t.Fatal("an absent field must not be read as off") + } +} + +func TestFindingsAreOrderedWorstFirst(t *testing.T) { + findings := []finding{ + {Server: "b", Severity: note, Check: "tasks"}, + {Server: "a", Severity: warning, Check: "boot"}, + {Server: "c", Severity: critical, Check: "power"}, + } + sortFindings(findings) + + if findings[0].Severity != critical || findings[2].Severity != note { + t.Fatalf("expected critical, warning, note — got %v, %v, %v", + findings[0].Severity, findings[1].Severity, findings[2].Severity) + } +} + +// An unreadable date reports "no" rather than zero: zero would mean "expires +// today" and raise an alarm about a field nobody managed to parse. +func TestAnUnreadableDateIsNotToday(t *testing.T) { + if _, ok := daysUntil(""); ok { + t.Fatal("an empty date cannot be counted from") + } + if _, ok := daysUntil("next tuesday"); ok { + t.Fatal("an unparseable date cannot be counted from") + } + if _, ok := daysUntil("2026-09-01"); !ok { + t.Fatal("the format this API uses must be read") + } +} diff --git a/internal/services/common/common.go b/internal/services/common/common.go index 7bf12c982..86b8db26a 100644 --- a/internal/services/common/common.go +++ b/internal/services/common/common.go @@ -201,8 +201,6 @@ func CreateResource(cmd *cobra.Command, path, endpoint, defaultExample string, return nil, fmt.Errorf("parameters cannot be marshalled: %w", err) } - log.Println("Final parameters: \n" + string(out)) - // --dry-run stops here: the caller sees exactly what would have been sent, // and nothing reaches the API. if flags.DryRun { @@ -221,6 +219,19 @@ func CreateResource(cmd *cobra.Command, path, endpoint, defaultExample string, return nil, nil } + // Logged only once the dry run is ruled out. A --dry-run already prints the + // payload as its message, so logging it here printed the same JSON twice, + // the second time 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" } + // + // A real run still logs what it is about to send, which is what this line + // was for. + log.Println("Final parameters: \n" + string(out)) + var createdResource map[string]any if err := httpLib.Client.Post(endpoint, parameters, &createdResource); err != nil { return nil, fmt.Errorf("error creating resource: %w", err) diff --git a/internal/services/common/service_info.go b/internal/services/common/service_info.go index 9ce0b4c5b..b678696f0 100644 --- a/internal/services/common/service_info.go +++ b/internal/services/common/service_info.go @@ -6,35 +6,31 @@ package common import "github.com/spf13/cobra" -// The renewal flags shared by every `service-info edit` command, paired with -// the field each one sets in the API object. -// -// The registration and the payload builder live side by side on purpose: the -// flag name is the only thing that ties them together, so a rename that -// touches one and not the other would silently stop sending a setting rather -// than fail to compile. -var serviceInfoRenewFlags = []struct { - name string - field string - usage string -}{ - {"renew-automatic", "automatic", "Renew the service automatically"}, - {"renew-delete-at-expiration", "deleteAtExpiration", "Delete the service when it expires"}, - {"renew-forced", "forced", "Force the renewal"}, - {"renew-manual-payment", "manualPayment", "Pay the renewal manually"}, - {"renew-period", "period", "Renewal period, in months"}, +// ServiceInfoRenewFlag describes one renewal flag: what it is called on the +// command line, and which field of the API object it sets. +type ServiceInfoRenewFlag struct { + Name string + Field string + Usage string + + // Period is the one flag that carries a number rather than a yes or no. + Period bool } -// AddServiceInfoRenewFlags registers the renewal flags on a `service-info -// edit` command. -func AddServiceInfoRenewFlags(cmd *cobra.Command) { - for _, flag := range serviceInfoRenewFlags { - if flag.field == "period" { - cmd.Flags().Int(flag.name, 0, flag.usage) - continue - } - cmd.Flags().Bool(flag.name, false, flag.usage) - } +// ServiceInfoRenewFlags is the single description of the renewal flags shared +// by every `service-info edit` command. +// +// It is exported rather than kept private because the command layer registers +// the flags and this layer reads them back: the flag name is the only thing +// tying the two halves together, so they must not each hold their own copy of +// it. One table, read twice — a rename in it changes both sides at once, and a +// rename anywhere else does not compile. +var ServiceInfoRenewFlags = []ServiceInfoRenewFlag{ + {Name: "renew-automatic", Field: "automatic", Usage: "Renew the service automatically"}, + {Name: "renew-delete-at-expiration", Field: "deleteAtExpiration", Usage: "Delete the service when it expires"}, + {Name: "renew-forced", Field: "forced", Usage: "Force the renewal"}, + {Name: "renew-manual-payment", Field: "manualPayment", Usage: "Pay the renewal manually"}, + {Name: "renew-period", Field: "period", Usage: "Renewal period, in months", Period: true}, } // ServiceInfoRenewPayload returns the renewal settings the operator actually @@ -53,25 +49,25 @@ func AddServiceInfoRenewFlags(cmd *cobra.Command) { func ServiceInfoRenewPayload(cmd *cobra.Command) map[string]any { renew := map[string]any{} - for _, flag := range serviceInfoRenewFlags { - if !cmd.Flags().Changed(flag.name) { + for _, flag := range ServiceInfoRenewFlags { + if !cmd.Flags().Changed(flag.Name) { continue } - if flag.field == "period" { - period, err := cmd.Flags().GetInt(flag.name) + if flag.Period { + period, err := cmd.Flags().GetInt(flag.Name) if err != nil { continue } - renew[flag.field] = period + renew[flag.Field] = period continue } - value, err := cmd.Flags().GetBool(flag.name) + value, err := cmd.Flags().GetBool(flag.Name) if err != nil { continue } - renew[flag.field] = value + renew[flag.Field] = value } if len(renew) == 0 {