From d2de4c3a506c4c92ba452e933e382149dc8d8748 Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Tue, 18 Aug 2026 17:02:04 +0200 Subject: [PATCH 1/7] fix(service-info): send only the renewal settings the operator asked to change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- doc/ovhcloud_vps_service-info_edit.md | 10 +-- ...loud_webhosting_cdn_service-info_update.md | 10 +-- ...ebhosting_extra-sql_service-info_update.md | 10 +-- ..._local-seo_location_service-info_update.md | 10 +-- ...ovhcloud_webhosting_service-info_update.md | 10 +-- internal/cmd/vps.go | 6 +- internal/cmd/vps_test.go | 58 +++++++++++++ internal/cmd/webhosting.go | 24 +----- internal/services/common/common.go | 16 +--- internal/services/common/service_info.go | 82 +++++++++++++++++++ internal/services/vps/vps.go | 4 +- internal/services/webhosting/webhosting.go | 33 +------- 12 files changed, 179 insertions(+), 94 deletions(-) create mode 100644 internal/services/common/service_info.go diff --git a/doc/ovhcloud_vps_service-info_edit.md b/doc/ovhcloud_vps_service-info_edit.md index ca6431a61..0d4ed8f78 100644 --- a/doc/ovhcloud_vps_service-info_edit.md +++ b/doc/ovhcloud_vps_service-info_edit.md @@ -11,11 +11,11 @@ ovhcloud vps service-info edit [flags] ``` --editor Use a text editor to define parameters -h, --help help for edit - --renew-automatic Enable automatic renewal - --renew-delete-at-expiration Delete service at expiration - --renew-forced Force renewal - --renew-manual-payment Enable manual payment for renewal - --renew-period int Renewal period (in months) + --renew-automatic Renew the service automatically + --renew-delete-at-expiration Delete the service when it expires + --renew-forced Force the renewal + --renew-manual-payment Pay the renewal manually + --renew-period int Renewal period, in months ``` ### Options inherited from parent commands diff --git a/doc/ovhcloud_webhosting_cdn_service-info_update.md b/doc/ovhcloud_webhosting_cdn_service-info_update.md index 3090d068b..550fb8c24 100644 --- a/doc/ovhcloud_webhosting_cdn_service-info_update.md +++ b/doc/ovhcloud_webhosting_cdn_service-info_update.md @@ -12,11 +12,11 @@ ovhcloud webhosting cdn service-info update [flags] --editor Use a text editor to define parameters --from-file string File containing parameters -h, --help help for update - --renew-automatic Enable automatic renewal - --renew-delete-at-expiration Delete service at expiration - --renew-forced Force renewal - --renew-manual-payment Enable manual payment for renewal - --renew-period int Renewal period in months + --renew-automatic Renew the service automatically + --renew-delete-at-expiration Delete the service when it expires + --renew-forced Force the renewal + --renew-manual-payment Pay the renewal manually + --renew-period int Renewal period, in months ``` ### Options inherited from parent commands diff --git a/doc/ovhcloud_webhosting_extra-sql_service-info_update.md b/doc/ovhcloud_webhosting_extra-sql_service-info_update.md index a0885eabf..8b6c4192c 100644 --- a/doc/ovhcloud_webhosting_extra-sql_service-info_update.md +++ b/doc/ovhcloud_webhosting_extra-sql_service-info_update.md @@ -12,11 +12,11 @@ ovhcloud webhosting extra-sql service-info update [flags] --editor Use a text editor to define parameters --from-file string File containing parameters -h, --help help for update - --renew-automatic Enable automatic renewal - --renew-delete-at-expiration Delete service at expiration - --renew-forced Force renewal - --renew-manual-payment Enable manual payment for renewal - --renew-period int Renewal period in months + --renew-automatic Renew the service automatically + --renew-delete-at-expiration Delete the service when it expires + --renew-forced Force the renewal + --renew-manual-payment Pay the renewal manually + --renew-period int Renewal period, in months ``` ### Options inherited from parent commands diff --git a/doc/ovhcloud_webhosting_local-seo_location_service-info_update.md b/doc/ovhcloud_webhosting_local-seo_location_service-info_update.md index fc847ba79..01a04c78d 100644 --- a/doc/ovhcloud_webhosting_local-seo_location_service-info_update.md +++ b/doc/ovhcloud_webhosting_local-seo_location_service-info_update.md @@ -12,11 +12,11 @@ ovhcloud webhosting local-seo location service-info update [ --editor Use a text editor to define parameters --from-file string File containing parameters -h, --help help for update - --renew-automatic Enable automatic renewal - --renew-delete-at-expiration Delete service at expiration - --renew-forced Force renewal - --renew-manual-payment Enable manual payment for renewal - --renew-period int Renewal period in months + --renew-automatic Renew the service automatically + --renew-delete-at-expiration Delete the service when it expires + --renew-forced Force the renewal + --renew-manual-payment Pay the renewal manually + --renew-period int Renewal period, in months ``` ### Options inherited from parent commands diff --git a/doc/ovhcloud_webhosting_service-info_update.md b/doc/ovhcloud_webhosting_service-info_update.md index 6ee3bad93..821effd28 100644 --- a/doc/ovhcloud_webhosting_service-info_update.md +++ b/doc/ovhcloud_webhosting_service-info_update.md @@ -12,11 +12,11 @@ ovhcloud webhosting service-info update [flags] --editor Use a text editor to define parameters --from-file string File containing parameters -h, --help help for update - --renew-automatic Enable automatic renewal - --renew-delete-at-expiration Delete service at expiration - --renew-forced Force renewal - --renew-manual-payment Enable manual payment for renewal - --renew-period int Renewal period in months + --renew-automatic Renew the service automatically + --renew-delete-at-expiration Delete the service when it expires + --renew-forced Force the renewal + --renew-manual-payment Pay the renewal manually + --renew-period int Renewal period, in months ``` ### Options inherited from parent commands diff --git a/internal/cmd/vps.go b/internal/cmd/vps.go index acb2ea78f..f452ef0c6 100644 --- a/internal/cmd/vps.go +++ b/internal/cmd/vps.go @@ -221,11 +221,7 @@ func init() { ValidArgsFunction: completion.ServiceList("/v1/vps"), Run: vps.EditVpsServiceInfo, } - serviceInfoEditCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.Automatic, "renew-automatic", false, "Enable automatic renewal") - serviceInfoEditCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.DeleteAtExpiration, "renew-delete-at-expiration", false, "Delete service at expiration") - serviceInfoEditCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.Forced, "renew-forced", false, "Force renewal") - serviceInfoEditCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.ManualPayment, "renew-manual-payment", false, "Enable manual payment for renewal") - serviceInfoEditCmd.Flags().IntVar(&common.ServiceInfoSpec.Renew.Period, "renew-period", 0, "Renewal period (in months)") + common.AddServiceInfoRenewFlags(serviceInfoEditCmd) addInteractiveEditorFlag(serviceInfoEditCmd) serviceInfoCmd.AddCommand(serviceInfoEditCmd) diff --git a/internal/cmd/vps_test.go b/internal/cmd/vps_test.go index fef023dc5..467362437 100644 --- a/internal/cmd/vps_test.go +++ b/internal/cmd/vps_test.go @@ -6,6 +6,8 @@ package cmd_test import ( "encoding/json" + "io" + "net/http" "github.com/jarcoal/httpmock" "github.com/maxatome/go-testdeep/td" @@ -63,3 +65,59 @@ func (ms *MockSuite) TestVpsGetCmd(assert, require *td.T) { } }`)) } + +// registerVpsServiceInfos wires a service whose renewal is currently automatic, +// and captures whatever the CLI decides to write back. +func registerVpsServiceInfos(captured *map[string]any) { + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/v1/vps/fakeVps/serviceInfos", + httpmock.NewStringResponder(200, `{ + "serviceId": 1, + "domain": "fakeVps", + "renew": {"automatic": true, "deleteAtExpiration": false, "forced": false, "manualPayment": false, "period": 1} + }`), + ) + httpmock.RegisterResponder("PUT", "https://eu.api.ovh.com/v1/vps/fakeVps/serviceInfos", + func(req *http.Request) (*http.Response, error) { + body, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + var sent map[string]any + if err := json.Unmarshal(body, &sent); err != nil { + return nil, err + } + *captured = sent + return httpmock.NewStringResponse(200, `null`), nil + }, + ) +} + +// Editing the renewal period used to send every other renewal setting along +// with it, at its zero value: a service that renewed itself automatically for +// years stopped doing so, and nothing in the output said it had changed. +func (ms *MockSuite) TestVpsServiceInfoEditSendsOnlyWhatWasAsked(assert, require *td.T) { + var sent map[string]any + registerVpsServiceInfos(&sent) + + _, err := cmd.Execute("vps", "service-info", "edit", "fakeVps", "--renew-period", "12") + + require.CmpNoError(err) + renew, _ := sent["renew"].(map[string]any) + require.NotNil(renew, "the renewal block must be written") + assert.Cmp(renew["period"], float64(12), "the period the operator asked for") + assert.Cmp(renew["automatic"], true, "automatic renewal must survive untouched") +} + +// The flag being absent and the flag being set to false are different +// intentions, and pflag can tell them apart: an explicit false must be sent. +func (ms *MockSuite) TestVpsServiceInfoEditSendsAnExplicitFalse(assert, require *td.T) { + var sent map[string]any + registerVpsServiceInfos(&sent) + + _, err := cmd.Execute("vps", "service-info", "edit", "fakeVps", "--renew-automatic=false") + + require.CmpNoError(err) + renew, _ := sent["renew"].(map[string]any) + require.NotNil(renew) + assert.Cmp(renew["automatic"], false, "the operator asked for it, so it is sent") +} diff --git a/internal/cmd/webhosting.go b/internal/cmd/webhosting.go index b2baf62c0..7e5096d0f 100644 --- a/internal/cmd/webhosting.go +++ b/internal/cmd/webhosting.go @@ -605,11 +605,7 @@ func init() { ValidArgsFunction: completion.ServiceList("/v1/hosting/web"), Run: webhosting.UpdateExtraSqlServiceInfo, } - extraSQLServiceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.Automatic, "renew-automatic", false, "Enable automatic renewal") - extraSQLServiceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.DeleteAtExpiration, "renew-delete-at-expiration", false, "Delete service at expiration") - extraSQLServiceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.Forced, "renew-forced", false, "Force renewal") - extraSQLServiceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.ManualPayment, "renew-manual-payment", false, "Enable manual payment for renewal") - extraSQLServiceInfoUpdateCmd.Flags().IntVar(&common.ServiceInfoSpec.Renew.Period, "renew-period", 0, "Renewal period in months") + common.AddServiceInfoRenewFlags(extraSQLServiceInfoUpdateCmd) addParameterFileFlags(extraSQLServiceInfoUpdateCmd, true, nil, "", "", "", nil) addInteractiveEditorFlag(extraSQLServiceInfoUpdateCmd) extraSQLServiceInfoCmd.AddCommand(extraSQLServiceInfoUpdateCmd) @@ -1330,11 +1326,7 @@ func init() { ValidArgsFunction: completion.ServiceList("/v1/hosting/web"), Run: webhosting.UpdateCdnServiceInfo, } - cdnServiceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.Automatic, "renew-automatic", false, "Enable automatic renewal") - cdnServiceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.DeleteAtExpiration, "renew-delete-at-expiration", false, "Delete service at expiration") - cdnServiceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.Forced, "renew-forced", false, "Force renewal") - cdnServiceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.ManualPayment, "renew-manual-payment", false, "Enable manual payment for renewal") - cdnServiceInfoUpdateCmd.Flags().IntVar(&common.ServiceInfoSpec.Renew.Period, "renew-period", 0, "Renewal period in months") + common.AddServiceInfoRenewFlags(cdnServiceInfoUpdateCmd) addParameterFileFlags(cdnServiceInfoUpdateCmd, true, nil, "", "", "", nil) addInteractiveEditorFlag(cdnServiceInfoUpdateCmd) cdnServiceInfoCmd.AddCommand(cdnServiceInfoUpdateCmd) @@ -1468,11 +1460,7 @@ func init() { ValidArgsFunction: completion.ServiceList("/v1/hosting/web"), Run: webhosting.UpdateServiceInfo, } - serviceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.Automatic, "renew-automatic", false, "Enable automatic renewal") - serviceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.DeleteAtExpiration, "renew-delete-at-expiration", false, "Delete service at expiration") - serviceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.Forced, "renew-forced", false, "Force renewal") - serviceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.ManualPayment, "renew-manual-payment", false, "Enable manual payment for renewal") - serviceInfoUpdateCmd.Flags().IntVar(&common.ServiceInfoSpec.Renew.Period, "renew-period", 0, "Renewal period in months") + common.AddServiceInfoRenewFlags(serviceInfoUpdateCmd) addParameterFileFlags(serviceInfoUpdateCmd, true, nil, "", "", "", nil) addInteractiveEditorFlag(serviceInfoUpdateCmd) serviceInfoCmd.AddCommand(serviceInfoUpdateCmd) @@ -1583,11 +1571,7 @@ func init() { ValidArgsFunction: completion.ServiceList("/v1/hosting/web"), Run: webhosting.UpdateLocalSeoLocationServiceInfo, } - localSeoLocationServiceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.Automatic, "renew-automatic", false, "Enable automatic renewal") - localSeoLocationServiceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.DeleteAtExpiration, "renew-delete-at-expiration", false, "Delete service at expiration") - localSeoLocationServiceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.Forced, "renew-forced", false, "Force renewal") - localSeoLocationServiceInfoUpdateCmd.Flags().BoolVar(&common.ServiceInfoSpec.Renew.ManualPayment, "renew-manual-payment", false, "Enable manual payment for renewal") - localSeoLocationServiceInfoUpdateCmd.Flags().IntVar(&common.ServiceInfoSpec.Renew.Period, "renew-period", 0, "Renewal period in months") + common.AddServiceInfoRenewFlags(localSeoLocationServiceInfoUpdateCmd) addParameterFileFlags(localSeoLocationServiceInfoUpdateCmd, true, nil, "", "", "", nil) addInteractiveEditorFlag(localSeoLocationServiceInfoUpdateCmd) localSeoLocationServiceInfoCmd.AddCommand(localSeoLocationServiceInfoUpdateCmd) diff --git a/internal/services/common/common.go b/internal/services/common/common.go index 7a1729c99..c92516fcb 100644 --- a/internal/services/common/common.go +++ b/internal/services/common/common.go @@ -24,20 +24,8 @@ import ( "github.com/spf13/cobra" ) -var ( - //go:embed templates/service_info.tmpl - ServiceInfoTemplate string - - ServiceInfoSpec struct { - Renew struct { - Automatic bool `json:"automatic"` - DeleteAtExpiration bool `json:"deleteAtExpiration"` - Forced bool `json:"forced"` - ManualPayment bool `json:"manualPayment"` - Period int `json:"period"` - } `json:"renew"` - } -) +//go:embed templates/service_info.tmpl +var ServiceInfoTemplate string func ManageListRequest(path, idField string, columnsToDisplay, filters []string) { body, err := httpLib.FetchExpandedArray(path, idField) diff --git a/internal/services/common/service_info.go b/internal/services/common/service_info.go new file mode 100644 index 000000000..9ce0b4c5b --- /dev/null +++ b/internal/services/common/service_info.go @@ -0,0 +1,82 @@ +// SPDX-FileCopyrightText: 2025 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +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"}, +} + +// 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) + } +} + +// ServiceInfoRenewPayload returns the renewal settings the operator actually +// asked to change, and nothing else. +// +// The distinction matters more than it looks. These settings are booleans +// bound to a struct with no `omitempty`, so building the payload from that +// struct sends every one of them on every call — and a merge that lets the +// command line win then turns `--renew-period 12` into "set the period to 12 +// AND switch automatic renewal off". The service kept renewing itself for +// years; one unrelated edit stopped it, and nothing said so. +// +// Reading `Changed` rather than the values also keeps `--renew-automatic=false` +// working: pflag records a flag as changed whatever value it was given, so an +// explicit false is sent while an absent flag stays absent. +func ServiceInfoRenewPayload(cmd *cobra.Command) map[string]any { + renew := map[string]any{} + + for _, flag := range serviceInfoRenewFlags { + if !cmd.Flags().Changed(flag.name) { + continue + } + + if flag.field == "period" { + period, err := cmd.Flags().GetInt(flag.name) + if err != nil { + continue + } + renew[flag.field] = period + continue + } + + value, err := cmd.Flags().GetBool(flag.name) + if err != nil { + continue + } + renew[flag.field] = value + } + + if len(renew) == 0 { + return map[string]any{} + } + + return map[string]any{"renew": renew} +} diff --git a/internal/services/vps/vps.go b/internal/services/vps/vps.go index c06a59100..5178be5db 100644 --- a/internal/services/vps/vps.go +++ b/internal/services/vps/vps.go @@ -310,11 +310,13 @@ func GetVpsServiceInfo(_ *cobra.Command, args []string) { } func EditVpsServiceInfo(cmd *cobra.Command, args []string) { + renewPayload := common.ServiceInfoRenewPayload(cmd) + if err := common.EditResource( cmd, "/vps/{serviceName}/serviceInfos", fmt.Sprintf("/v1/vps/%s/serviceInfos", url.PathEscape(args[0])), - common.ServiceInfoSpec, + renewPayload, assets.VpsOpenapiSchema, ); err != nil { display.OutputError(&flags.OutputFormatConfig, "%s", err) diff --git a/internal/services/webhosting/webhosting.go b/internal/services/webhosting/webhosting.go index b24ec8b4b..f8aabc6fb 100644 --- a/internal/services/webhosting/webhosting.go +++ b/internal/services/webhosting/webhosting.go @@ -940,7 +940,7 @@ func GetExtraSqlServiceInfo(_ *cobra.Command, args []string) { } func UpdateExtraSqlServiceInfo(cmd *cobra.Command, args []string) { - payload := buildServiceInfoRenewPayload(cmd) + payload := common.ServiceInfoRenewPayload(cmd) if len(payload) == 0 && !flags.ParametersViaEditor && flags.ParametersFile == "" { display.OutputInfo(&flags.OutputFormatConfig, nil, "🟠 No parameters given, nothing to edit") return @@ -2532,7 +2532,7 @@ func GetCdnServiceInfo(_ *cobra.Command, args []string) { } func UpdateCdnServiceInfo(cmd *cobra.Command, args []string) { - payload := buildServiceInfoRenewPayload(cmd) + payload := common.ServiceInfoRenewPayload(cmd) if len(payload) == 0 && !flags.ParametersViaEditor && flags.ParametersFile == "" { display.OutputInfo(&flags.OutputFormatConfig, nil, "🟠 No parameters given, nothing to edit") return @@ -2735,31 +2735,6 @@ func buildCdnOptionConfig(cmd *cobra.Command) map[string]any { return config } -func buildServiceInfoRenewPayload(cmd *cobra.Command) map[string]any { - renew := map[string]any{} - if cmd.Flags().Changed("renew-automatic") { - renew["automatic"] = common.ServiceInfoSpec.Renew.Automatic - } - if cmd.Flags().Changed("renew-delete-at-expiration") { - renew["deleteAtExpiration"] = common.ServiceInfoSpec.Renew.DeleteAtExpiration - } - if cmd.Flags().Changed("renew-forced") { - renew["forced"] = common.ServiceInfoSpec.Renew.Forced - } - if cmd.Flags().Changed("renew-manual-payment") { - renew["manualPayment"] = common.ServiceInfoSpec.Renew.ManualPayment - } - if cmd.Flags().Changed("renew-period") { - renew["period"] = common.ServiceInfoSpec.Renew.Period - } - - if len(renew) == 0 { - return map[string]any{} - } - - return map[string]any{"renew": renew} -} - func formatQuota(value any) (string, bool) { quotaMap, ok := value.(map[string]any) if !ok { @@ -2980,7 +2955,7 @@ func GetServiceInfo(_ *cobra.Command, args []string) { } func UpdateServiceInfo(cmd *cobra.Command, args []string) { - payload := buildServiceInfoRenewPayload(cmd) + payload := common.ServiceInfoRenewPayload(cmd) if len(payload) == 0 && !flags.ParametersViaEditor && flags.ParametersFile == "" && !utils.IsInputFromPipe() { display.OutputInfo(&flags.OutputFormatConfig, nil, "🟠 No parameters given, nothing to edit") return @@ -3240,7 +3215,7 @@ func GetLocalSeoLocationServiceInfo(_ *cobra.Command, args []string) { } func UpdateLocalSeoLocationServiceInfo(cmd *cobra.Command, args []string) { - payload := buildServiceInfoRenewPayload(cmd) + payload := common.ServiceInfoRenewPayload(cmd) if len(payload) == 0 && !flags.ParametersViaEditor && flags.ParametersFile == "" { display.OutputInfo(&flags.OutputFormatConfig, nil, "🟠 No parameters given, nothing to edit") return From d44f8a2fa8c1a14f99a454ff0a1d0df69b296235 Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Wed, 19 Aug 2026 10:46:22 +0200 Subject: [PATCH 2/7] refactor(service-info): register the renewal flags from the command layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-Authored-By: Claude Opus 5 (1M context) --- internal/cmd/service_info.go | 28 +++++++++++ internal/cmd/vps.go | 3 +- internal/cmd/webhosting.go | 9 ++-- internal/services/common/service_info.go | 64 +++++++++++------------- 4 files changed, 63 insertions(+), 41 deletions(-) create mode 100644 internal/cmd/service_info.go 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/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 { From 81197f5895aedd7e117f027cf1b0cc2cb094dba3 Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Wed, 19 Aug 2026 13:21:45 +0200 Subject: [PATCH 3/7] feat(baremetal): say what a reinstall is doing while it does it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `reinstall --wait` followed the task, and the task API answers whether the work is finished and nothing else. So a reinstall that runs for twenty minutes printed the same sentence sixty times: Still waiting for task 559351769 to complete (status=doing)… install/status has the missing half — one entry per step, with the running one marked — and the CLI used it nowhere. It does now, in two places: - `baremetal install-status ` reads it, for somebody who started the installation in another terminal, or who answered the confirmation, walked away, and wants to know whether to keep waiting. - `reinstall --wait` prints the step it is on instead of repeating itself. Measured against a real reinstall of an ADVANCE-1: step 5/21: Preparing disks for new Partitioning step 10/21: Applying Partitioning step 13/21: Downloading OS image step 16/21: Checking Partitioning Two things the API does that the code has to know about. Outside an installation the route answers 404 with "Server is not being installed or reinstalled at the moment". That is an answer, not a breakdown, and it is also why this endpoint cannot carry the verdict of a --wait: it says the same thing before an installation starts and after it ends. The task stays the authority on whether the work is done; install/status only says where it has got to, and a progress read that fails is an empty line rather than a failed command. And elapsedTime is not the age of the installation. Sampled through one real run: -1935 during the hardware reboot, +42 four minutes later, -87 again after the final reboot, counting up at exactly one per second in each stretch and never resetting between steps. It is a continuous counter whose origin is rebased every time the machine restarts. So --wait times itself from the request it sent, and the read command attributes the figure to the API instead of asserting it — it was reporting 1m47s while the install had been running for six minutes. Signed-off-by: Denis Hamon Co-Authored-By: Claude Opus 5 (1M context) --- doc/ovhcloud_baremetal.md | 1 + doc/ovhcloud_baremetal_install-status.md | 37 ++++ internal/cmd/baremetal.go | 12 ++ internal/cmd/baremetal_install_test.go | 71 +++++++ internal/services/baremetal/baremetal.go | 32 +++- internal/services/baremetal/baremetal_test.go | 42 +++- internal/services/baremetal/install.go | 180 ++++++++++++++++++ internal/services/baremetal/install_test.go | 101 ++++++++++ .../baremetal/templates/install_status.tmpl | 9 + 9 files changed, 479 insertions(+), 6 deletions(-) create mode 100644 doc/ovhcloud_baremetal_install-status.md create mode 100644 internal/services/baremetal/install_test.go create mode 100644 internal/services/baremetal/templates/install_status.tmpl diff --git a/doc/ovhcloud_baremetal.md b/doc/ovhcloud_baremetal.md index dc3bd3ccc..77ccb23d4 100644 --- a/doc/ovhcloud_baremetal.md +++ b/doc/ovhcloud_baremetal.md @@ -35,6 +35,7 @@ Retrieve information and manage your Bare Metal services * [ovhcloud baremetal confirm-termination](ovhcloud_baremetal_confirm-termination.md) - Confirm the termination of the given baremetal * [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 * [ovhcloud baremetal ipmi](ovhcloud_baremetal_ipmi.md) - Manage IPMI on your baremetal * [ovhcloud baremetal list](ovhcloud_baremetal_list.md) - List your Baremetal services * [ovhcloud baremetal list-compatible-os](ovhcloud_baremetal_list-compatible-os.md) - Retrieve OSes that can be installed on this baremetal diff --git a/doc/ovhcloud_baremetal_install-status.md b/doc/ovhcloud_baremetal_install-status.md new file mode 100644 index 000000000..99690adf5 --- /dev/null +++ b/doc/ovhcloud_baremetal_install-status.md @@ -0,0 +1,37 @@ +## ovhcloud baremetal install-status + +Show how far the running installation of this baremetal has got + +``` +ovhcloud baremetal install-status [flags] +``` + +### Options + +``` + -h, --help help for install-status +``` + +### 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/internal/cmd/baremetal.go b/internal/cmd/baremetal.go index 512b8f258..f83ae7560 100644 --- a/internal/cmd/baremetal.go +++ b/internal/cmd/baremetal.go @@ -415,6 +415,18 @@ a server is sitting on the power-off entry.`, Run: baremetal.GetBaremetalRaidProfile, })) + // A reinstall runs for tens of minutes and, until now, said nothing while + // it did. This reads the same progress `reinstall --wait` follows, for + // somebody who started the install in another terminal — or who answered + // the confirmation, walked away, and wants to know whether to keep waiting. + baremetalCmd.AddCommand(&cobra.Command{ + Use: "install-status ", + Short: "Show how far the running installation of this baremetal has got", + Args: cobra.ExactArgs(1), + ValidArgsFunction: completion.ServiceList("/v1/dedicated/server"), + Run: baremetal.ShowBaremetalInstallStatus, + }) + // Commands to manage virtual network interfaces // Private network, seen from the machine. The same work lives under // `ovhcloud vrack`; this is where somebody holding a server looks for it. diff --git a/internal/cmd/baremetal_install_test.go b/internal/cmd/baremetal_install_test.go index 0f76cc3ff..8b8f4bb3e 100644 --- a/internal/cmd/baremetal_install_test.go +++ b/internal/cmd/baremetal_install_test.go @@ -75,3 +75,74 @@ func (ms *MockSuite) TestBaremetalRaidProfileListsControllersAndDisks(assert, re assert.Cmp(out, td.Contains("PERC H730P")) assert.Cmp(out, td.Contains("3726GiB")) } + +// Outside an installation the route answers 404 with a sentence, not a +// payload. That is the state most servers are in most of the time, and it is +// an answer: reporting it as a failure would send somebody looking for a +// broken command. +func (ms *MockSuite) TestBaremetalInstallStatusTreatsIdleAsAnAnswer(assert, require *td.T) { + httpmock.RegisterResponder("GET", + "https://eu.api.ovh.com/v1/dedicated/server/fakeBaremetal/install/status", + httpmock.NewStringResponder(404, + `{"message": "Server is not being installed or reinstalled at the moment"}`)) + + out, err := cmd.Execute("baremetal", "install-status", "fakeBaremetal") + + require.CmpNoError(err, "an idle server is not a failed command") + assert.Cmp(out, td.Contains("not being installed")) +} + +// A genuine failure stays one: the message match only ever softens the idle +// case. +func (ms *MockSuite) TestBaremetalInstallStatusStillReportsRealFailures(assert, require *td.T) { + httpmock.RegisterResponder("GET", + "https://eu.api.ovh.com/v1/dedicated/server/fakeBaremetal/install/status", + httpmock.NewStringResponder(403, `{"message": "This call has not been granted"}`)) + + _, err := cmd.Execute("baremetal", "install-status", "fakeBaremetal") + + require.CmpError(err) + assert.Cmp(err.Error(), td.Contains("not been granted")) +} + +// The point of the command is to say which step is running and for how long. +// A list of steps without either is the same non-answer --wait used to give. +func (ms *MockSuite) TestBaremetalInstallStatusNamesTheRunningStep(assert, require *td.T) { + httpmock.RegisterResponder("GET", + "https://eu.api.ovh.com/v1/dedicated/server/fakeBaremetal/install/status", + httpmock.NewStringResponder(200, `{ + "elapsedTime": 754, + "progress": [ + {"comment": "Initialising Installation process", "status": "done"}, + {"comment": "Installing operating system", "status": "doing"}, + {"comment": "Rebooting", "status": "todo"} + ] + }`)) + + out, err := cmd.Execute("baremetal", "install-status", "fakeBaremetal") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("step 2 of 3"), "the position has to be visible") + assert.Cmp(out, td.Contains("Installing operating system")) + assert.Cmp(out, td.Contains("12m34s"), "754 seconds, not 754") + assert.Cmp(out, td.Contains("API reports"), + "the figure is the API's: measured shifting origin mid-install") + assert.Cmp(out, td.Contains("Rebooting"), "and the steps still to come") +} + +// The error of a failed step is what says why, so it travels with it. +func (ms *MockSuite) TestBaremetalInstallStatusShowsWhyAStepFailed(assert, require *td.T) { + httpmock.RegisterResponder("GET", + "https://eu.api.ovh.com/v1/dedicated/server/fakeBaremetal/install/status", + httpmock.NewStringResponder(200, `{ + "elapsedTime": 42, + "progress": [ + {"comment": "Partitioning", "status": "error", "error": "disk 2 is not present"} + ] + }`)) + + out, err := cmd.Execute("baremetal", "install-status", "fakeBaremetal") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("disk 2 is not present")) +} diff --git a/internal/services/baremetal/baremetal.go b/internal/services/baremetal/baremetal.go index 4f61007e1..7a1a964ac 100644 --- a/internal/services/baremetal/baremetal.go +++ b/internal/services/baremetal/baremetal.go @@ -214,7 +214,7 @@ func RebootRescueBaremetal(cmd *cobra.Command, args []string) { return } - if err := waitForDedicatedServerTask(args[0], task["taskId"]); err != nil { + if err := waitForDedicatedServerTask(args[0], task["taskId"], nil); err != nil { display.OutputError(&flags.OutputFormatConfig, "failed to wait for server to be rebooted: %s", err) return } @@ -261,7 +261,15 @@ func taskFailureReason(task map[string]any) string { return "" } -func waitForDedicatedServerTask(serviceName string, taskID any) error { +// waitForDedicatedServerTask follows a task until it ends. +// +// note, when given, is asked for one extra line on every poll. The task API +// says whether the work is finished and nothing else, so a reinstall that +// takes half an hour printed the same sentence sixty times; the caller that +// has a second source of progress passes it here. It is decoration: a note +// that cannot be produced is an empty string, never an error, because failing +// to describe an installation must not fail the wait for it. +func waitForDedicatedServerTask(serviceName string, taskID any, note func() string) error { endpoint := fmt.Sprintf("/v1/dedicated/server/%s/task/%s", url.PathEscape(serviceName), taskID) followUp := fmt.Sprintf("follow it with: ovhcloud baremetal list-tasks %s", serviceName) @@ -285,6 +293,14 @@ func waitForDedicatedServerTask(serviceName string, taskID any) error { return nil case "todo", "init", "doing": + if note != nil { + if progress := note(); progress != "" { + log.Printf("%s — %s", progress, describeTask(taskID, task)) + time.Sleep(taskPollInterval) + continue + } + } + log.Printf("Still waiting for task %s to complete (status=%v)…", describeTask(taskID, task), task["status"]) time.Sleep(taskPollInterval) @@ -336,7 +352,7 @@ func BaremetalGetIPMIAccess(_ *cobra.Command, args []string) { return } - if err := waitForDedicatedServerTask(args[0], task["taskId"]); err != nil { + if err := waitForDedicatedServerTask(args[0], task["taskId"], nil); err != nil { display.OutputError(&flags.OutputFormatConfig, "failed waiting for task: %s", err) return } @@ -588,12 +604,20 @@ func ReinstallBaremetal(cmd *cobra.Command, args []string) { log.Println("⚡️ Reinstallation started…") + // The wait times itself from here. install/status reports an elapsedTime, + // but it answered -1935 seconds on the reinstall this was measured + // against, counting up correctly from an origin about half an hour ahead: + // its differences are true and its absolute value is not. + startedAt := time.Now() + if !flags.WaitForTask { display.OutputInfo(&flags.OutputFormatConfig, nil, "⚡️ Reinstallation is started…") return } - if err := waitForDedicatedServerTask(args[0], task["taskId"]); err != nil { + if err := waitForDedicatedServerTask(args[0], task["taskId"], func() string { + return installProgressNote(args[0], startedAt) + }); err != nil { display.OutputError(&flags.OutputFormatConfig, "failed to wait for server to be reinstalled: %s", err) return } diff --git a/internal/services/baremetal/baremetal_test.go b/internal/services/baremetal/baremetal_test.go index 981927ca1..28f7a6bc1 100644 --- a/internal/services/baremetal/baremetal_test.go +++ b/internal/services/baremetal/baremetal_test.go @@ -5,6 +5,9 @@ package baremetal import ( + "bytes" + "log" + "os" "testing" "time" @@ -45,7 +48,7 @@ func withTaskAPI(t *testing.T, attempts int, task string) { func TestWaitForTask_TimeoutDoesNotClaimTheTaskFailed(t *testing.T) { withTaskAPI(t, 2, `{"taskId": 156839472, "function": "reinstallServer", "status": "doing"}`) - err := waitForDedicatedServerTask("srv", "156839472") + err := waitForDedicatedServerTask("srv", "156839472", nil) td.Require(t).CmpError(err) td.Cmp(t, err.Error(), td.Contains("stopped waiting")) @@ -63,9 +66,44 @@ func TestWaitForTask_TimeoutDoesNotClaimTheTaskFailed(t *testing.T) { func TestWaitForTask_TimeoutPrintsAReadableIdentifier(t *testing.T) { withTaskAPI(t, 1, `{"taskId": 156839472, "status": "todo"}`) - err := waitForDedicatedServerTask("srv", "156839472") + err := waitForDedicatedServerTask("srv", "156839472", nil) td.Require(t).CmpError(err) td.Cmp(t, err.Error(), td.Contains("156839472")) td.Cmp(t, err.Error(), td.Not(td.Contains("%!"))) } + +// The note is the whole point of the change: without it a reinstall printed +// the same sentence every thirty seconds for half an hour. A note that is +// computed and then dropped would leave the wait exactly as mute as before, +// and no other test would notice. +func TestWaitForTask_PrintsTheNoteInsteadOfRepeatingItself(t *testing.T) { + withTaskAPI(t, 2, `{"taskId": 156839472, "function": "reinstallServer", "status": "doing"}`) + + var logged bytes.Buffer + log.SetOutput(&logged) + defer log.SetOutput(os.Stderr) + + _ = waitForDedicatedServerTask("srv", "156839472", func() string { + return "step 3/21: Running Hardware Reboot (2m14s elapsed)" + }) + + td.Cmp(t, logged.String(), td.Contains("step 3/21: Running Hardware Reboot")) + td.Cmp(t, logged.String(), td.Not(td.Contains("Still waiting for task")), + "the note replaces the bare line rather than adding to it") +} + +// Reading the progress is decoration on top of the task poll. A server whose +// status cannot be read is still being installed, and the wait must carry on +// saying what it always said rather than fail. +func TestWaitForTask_FallsBackWhenThereIsNoNote(t *testing.T) { + withTaskAPI(t, 2, `{"taskId": 156839472, "function": "reinstallServer", "status": "doing"}`) + + var logged bytes.Buffer + log.SetOutput(&logged) + defer log.SetOutput(os.Stderr) + + _ = waitForDedicatedServerTask("srv", "156839472", func() string { return "" }) + + td.Cmp(t, logged.String(), td.Contains("Still waiting for task")) +} diff --git a/internal/services/baremetal/install.go b/internal/services/baremetal/install.go index 6913c9b0e..a5743dd64 100644 --- a/internal/services/baremetal/install.go +++ b/internal/services/baremetal/install.go @@ -5,9 +5,11 @@ package baremetal import ( + _ "embed" "fmt" "net/url" "strings" + "time" "github.com/ovh/ovhcloud-cli/internal/display" "github.com/ovh/ovhcloud-cli/internal/flags" @@ -33,6 +35,9 @@ import ( // layout and learning at install time whether the server would take it. On a // reinstall, learning at install time means the disks are already wiped. +//go:embed templates/install_status.tmpl +var installStatusTemplate string + var ( // InstallTemplate is the OS template these answers are relative to. InstallTemplate string @@ -128,3 +133,178 @@ func GetBaremetalRaidProfile(_ *cobra.Command, args []string) { func isUnsupportedHardwareRaid(err error) bool { return err != nil && strings.Contains(strings.ToLower(err.Error()), "hardware raid is not supported") } + +// InstallationProgress is what install/status answers while a machine is being +// installed: how long it has been going, and one entry per step. +// +// The steps carry no name of their own — `comment` is the whole description — +// so "which step is this" is answered by position, and "which one is running" +// by the single entry whose status is `doing`. +type InstallationProgress struct { + ElapsedTime int `json:"elapsedTime"` + Progress []struct { + Comment string `json:"comment"` + Error string `json:"error"` + Status string `json:"status"` + } `json:"progress"` +} + +// current returns the step being worked on, its position, and whether there is +// one at all. A finished-but-not-yet-cleared installation has every step done. +func (p InstallationProgress) current() (comment string, position int, ok bool) { + for i, step := range p.Progress { + if step.Status == "doing" { + return step.Comment, i + 1, true + } + } + return "", 0, false +} + +// failed returns the first step that reported an error. +func (p InstallationProgress) failed() (comment, reason string, ok bool) { + for _, step := range p.Progress { + if step.Status == "error" { + return step.Comment, step.Error, true + } + } + return "", "", false +} + +// isNotBeingInstalled recognises the API saying nothing is running. +// +// The route answers 404 outside an installation — verified on three servers — +// with "Server is not being installed or reinstalled at the moment". That is +// an answer, not a breakdown, and it is also the reason this endpoint cannot +// carry the verdict of a --wait: it says the same thing before an install has +// started and after it has finished. The task remains the authority on +// whether the work is done; this one only says where it has got to. +func isNotBeingInstalled(err error) bool { + return err != nil && + strings.Contains(strings.ToLower(err.Error()), "not being installed or reinstalled") +} + +// fetchInstallProgress reads install/status, separating "nothing is running" +// from "the call failed". +func fetchInstallProgress(server string) (InstallationProgress, bool, error) { + var progress InstallationProgress + + path := fmt.Sprintf("/v1/dedicated/server/%s/install/status", url.PathEscape(server)) + if err := httpLib.Client.Get(path, &progress); err != nil { + if isNotBeingInstalled(err) { + return progress, false, nil + } + return progress, false, err + } + + return progress, true, nil +} + +// ShowBaremetalInstallStatus reports where a running installation has got to. +func ShowBaremetalInstallStatus(_ *cobra.Command, args []string) { + server := args[0] + + progress, installing, err := fetchInstallProgress(server) + if err != nil { + display.OutputError(&flags.OutputFormatConfig, + "failed to fetch the installation status of %s: %s", server, err) + return + } + + if !installing { + display.OutputInfo(&flags.OutputFormatConfig, + map[string]any{"server": server, "installing": false}, + "%s is not being installed or reinstalled at the moment.", server) + return + } + + steps := make([]map[string]any, 0, len(progress.Progress)) + for i, step := range progress.Progress { + steps = append(steps, map[string]any{ + "position": i + 1, + "status": step.Status, + "comment": step.Comment, + "error": step.Error, + }) + } + + // elapsedTime is attributed to the API rather than presented as the age of + // the installation, because it is not one. Measured across a real + // reinstall: -1935 during the hardware reboot, +42 four minutes later, + // then -87 again after the final reboot. It counts up at one per second in + // each stretch and does not reset between steps — step 5 at 42s became + // step 10 at 72s — so it is a continuous counter whose origin is rebased + // every time the machine restarts, which is what its clock does before NTP + // catches up. Reporting it as "running for 1m47s" when the install had + // been going for six minutes would state something measured to be false. + elapsed := "" + if progress.ElapsedTime >= 0 { + elapsed = " · API reports " + formatElapsed(progress.ElapsedTime) + " elapsed" + } + + summary := fmt.Sprintf("%d step(s)%s", len(steps), elapsed) + if comment, position, ok := progress.current(); ok { + summary = fmt.Sprintf("step %d of %d — %s%s", position, len(steps), comment, elapsed) + } + + display.OutputObject(map[string]any{ + "installing": true, + "elapsedTime": progress.ElapsedTime, + "summary": summary, + "steps": steps, + }, server, installStatusTemplate, &flags.OutputFormatConfig) +} + +// unknownElapsed is what is printed instead of a duration the API did not +// give a usable value for. +const unknownElapsed = "an unknown time" + +// formatElapsed prints seconds the way somebody watching a reinstall reads +// them. The API counts in seconds, and a reinstall runs for tens of minutes, +// so "1834" is the one form nobody wants. +// +// Negative values are not turned into durations. elapsedTime is not always one: +// measured on a real reinstall of an ADVANCE-1, it answered -1935 while the +// machine had been installing for under two minutes, and it then counted up at +// one per second — so the field is a correct counter with an origin set some +// thirty minutes ahead. Its differences are true and its absolute value is not, +// which is why --wait times itself instead of asking. +func formatElapsed(seconds int) string { + if seconds < 0 { + return unknownElapsed + } + d := time.Duration(seconds) * time.Second + if d < time.Minute { + return fmt.Sprintf("%ds", int(d.Seconds())) + } + return fmt.Sprintf("%dm%02ds", int(d.Minutes()), int(d.Seconds())%60) +} + +// installProgressNote is the extra line --wait prints while it follows the +// reinstall task, so that a twenty-minute wait says something other than the +// same sentence over and over. +// +// It answers an empty string whenever it cannot say anything useful: this is +// decoration on top of the task poll, and a progress read that fails must +// never turn a running installation into a failed command. +func installProgressNote(server string, since time.Time) string { + progress, installing, err := fetchInstallProgress(server) + if err != nil || !installing { + return "" + } + + // The wait knows when it sent the request, so it says how long it has been + // waiting rather than repeating a field that answered -1935 seconds on the + // installation this was measured against. + elapsed := formatElapsed(int(time.Since(since).Seconds())) + + if comment, position, ok := progress.current(); ok { + return fmt.Sprintf("step %d/%d: %s (%s elapsed)", + position, len(progress.Progress), comment, elapsed) + } + + if comment, reason, ok := progress.failed(); ok { + return fmt.Sprintf("step %q reported an error: %s", comment, reason) + } + + return fmt.Sprintf("%d step(s), %s elapsed", len(progress.Progress), elapsed) +} diff --git a/internal/services/baremetal/install_test.go b/internal/services/baremetal/install_test.go new file mode 100644 index 000000000..f7e0e9b0b --- /dev/null +++ b/internal/services/baremetal/install_test.go @@ -0,0 +1,101 @@ +// SPDX-FileCopyrightText: 2025 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +package baremetal + +import ( + "errors" + "testing" + + "github.com/maxatome/go-testdeep/td" +) + +// The route answers 404 outside an installation, and that sentence is the only +// thing separating "nothing is running" from "the call failed". +func TestIsNotBeingInstalledRecognisesTheIdleAnswer(t *testing.T) { + td.Cmp(t, isNotBeingInstalled( + errors.New(`Error 404: "Server is not being installed or reinstalled at the moment"`)), true) + + td.Cmp(t, isNotBeingInstalled( + errors.New(`Error 403: "This call has not been granted"`)), false, + "a real failure must stay one") + + td.Cmp(t, isNotBeingInstalled(nil), false) +} + +// A reinstall runs for tens of minutes, so the number somebody reads has to be +// minutes. The API counts in seconds. +func TestFormatElapsedReadsAsTime(t *testing.T) { + td.Cmp(t, formatElapsed(0), "0s") + td.Cmp(t, formatElapsed(59), "59s") + td.Cmp(t, formatElapsed(60), "1m00s") + td.Cmp(t, formatElapsed(754), "12m34s") + td.Cmp(t, formatElapsed(3600), "60m00s") +} + +// Measured on a real reinstall, twice: elapsedTime answered -1935 during the +// hardware reboot and -87 again after the final one. A negative age is not a +// duration, so it is not printed as one — and never as a large positive +// number, which is what an unsigned reading of it would have produced. +func TestFormatElapsedRefusesToInventANegativeDuration(t *testing.T) { + td.Cmp(t, formatElapsed(-1935), unknownElapsed) + td.Cmp(t, formatElapsed(-87), unknownElapsed) + td.Cmp(t, formatElapsed(-1), unknownElapsed) +} + +// The running step is the one the operator is waiting on. +func TestCurrentStepIsTheOneBeingWorkedOn(t *testing.T) { + progress := InstallationProgress{} + progress.Progress = append(progress.Progress, + step("Checking BIOS version", "done", ""), + step("Running Hardware Reboot", "doing", ""), + step("Setting up hardware raid", "todo", "")) + + comment, position, ok := progress.current() + + td.Cmp(t, ok, true) + td.Cmp(t, comment, "Running Hardware Reboot") + td.Cmp(t, position, 2) +} + +// Every step done and none running is what an installation looks like on the +// poll after the last one finished. Reporting a running step there would name +// one that is not. +func TestCurrentStepIsAbsentWhenNothingIsRunning(t *testing.T) { + progress := InstallationProgress{} + progress.Progress = append(progress.Progress, + step("Checking BIOS version", "done", ""), + step("Rebooting", "done", "")) + + _, _, ok := progress.current() + + td.Cmp(t, ok, false) +} + +// A failed step carries why in a field of its own, and that is the whole value +// of reading the status at all when something goes wrong. +func TestFailedStepCarriesItsReason(t *testing.T) { + progress := InstallationProgress{} + progress.Progress = append(progress.Progress, + step("Checking BIOS version", "done", ""), + step("Preparing disks for new Partitioning", "error", "disk 2 is not present")) + + comment, reason, ok := progress.failed() + + td.Cmp(t, ok, true) + td.Cmp(t, comment, "Preparing disks for new Partitioning") + td.Cmp(t, reason, "disk 2 is not present") +} + +func step(comment, status, failure string) struct { + Comment string `json:"comment"` + Error string `json:"error"` + Status string `json:"status"` +} { + return struct { + Comment string `json:"comment"` + Error string `json:"error"` + Status string `json:"status"` + }{Comment: comment, Error: failure, Status: status} +} diff --git a/internal/services/baremetal/templates/install_status.tmpl b/internal/services/baremetal/templates/install_status.tmpl new file mode 100644 index 000000000..7745bb549 --- /dev/null +++ b/internal/services/baremetal/templates/install_status.tmpl @@ -0,0 +1,9 @@ +🔧 Installation of {{.ServiceName}} +======= + +**{{index .Result "summary"}}** + +| # | Status | Step | +| --- | --- | --- | +{{range index .Result "steps"}}| {{index . "position"}} | {{index . "status"}} | {{index . "comment"}}{{if index . "error"}} — {{index . "error"}}{{end}} | +{{end}} From cbf77a7242ad9be45c5837436d46241ad27e3e4a Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Thu, 20 Aug 2026 16:29:58 +0200 Subject: [PATCH 4/7] chore: no account data as test fixtures (case variants) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first sweep replaced only the exact spelling. A test that checks a name can be copied back without reproducing its case carried the uppercase form of a real hostname, which therefore survived — and, once its neighbour was replaced, made that test fail. A real value does not stop being one because one of its spellings was removed. Signed-off-by: Denis Hamon Co-Authored-By: Claude Opus 5 (1M context) --- internal/services/vrack/attach_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/services/vrack/attach_test.go b/internal/services/vrack/attach_test.go index f3d52f30a..4302b517f 100644 --- a/internal/services/vrack/attach_test.go +++ b/internal/services/vrack/attach_test.go @@ -139,7 +139,7 @@ func TestInterfacesOfAcceptsTheNameItPrints(t *testing.T) { assert.Cmp(len(interfacesOf(fleet, "ns0000002.ip-203-0-113.eu")), 1, "by hostname") assert.Cmp(len(interfacesOf(fleet, "Mail relay - Paris")), 1, "and by the name its owner gave it") - assert.Cmp(len(interfacesOf(fleet, "yaniv - rise-1 - lim")), 1, "typed by a human, so case-insensitively") + assert.Cmp(len(interfacesOf(fleet, "mail relay - paris")), 1, "typed by a human, so case-insensitively") assert.Cmp(len(interfacesOf(fleet, "nothing")), 0) } From 1a1bda3155664c0a6546aaeb3b39b38dc63d2b5d Mon Sep 17 00:00:00 2001 From: Denis Date: Mon, 24 Aug 2026 12:03:02 +0200 Subject: [PATCH 5/7] fix(dry-run): stop printing the payload twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/cmd/baremetal_test.go | 50 ++++++++++++++++++++++++++++++ internal/services/common/common.go | 15 +++++++-- 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/internal/cmd/baremetal_test.go b/internal/cmd/baremetal_test.go index de7da2116..4c243ae30 100644 --- a/internal/cmd/baremetal_test.go +++ b/internal/cmd/baremetal_test.go @@ -5,6 +5,9 @@ package cmd_test import ( + "bytes" + "log" + "github.com/jarcoal/httpmock" "github.com/maxatome/go-testdeep/td" "github.com/ovh/ovhcloud-cli/internal/cmd" @@ -166,3 +169,50 @@ func (ms *MockSuite) TestBaremetalReinstallDryRun(assert, require *td.T) { assert.Cmp(httpmock.GetCallCountInfo()["POST https://eu.api.ovh.com/v1/dedicated/server/fakeBaremetal/reinstall"], 0, "no reinstall call must reach the API") } + +// 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/services/common/common.go b/internal/services/common/common.go index 49988d8aa..f1f2c7d31 100644 --- a/internal/services/common/common.go +++ b/internal/services/common/common.go @@ -178,8 +178,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 { @@ -198,6 +196,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) From c421ef4e382b1135ad08ebd796c3e3146d2afbc6 Mon Sep 17 00:00:00 2001 From: Denis Date: Mon, 24 Aug 2026 12:06:07 +0200 Subject: [PATCH 6/7] test: move the dry-run log tests to their own file 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 --- internal/cmd/baremetal_test.go | 50 ----------------------- internal/cmd/dry_run_log_test.go | 68 ++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 50 deletions(-) create mode 100644 internal/cmd/dry_run_log_test.go diff --git a/internal/cmd/baremetal_test.go b/internal/cmd/baremetal_test.go index 4c243ae30..de7da2116 100644 --- a/internal/cmd/baremetal_test.go +++ b/internal/cmd/baremetal_test.go @@ -5,9 +5,6 @@ package cmd_test import ( - "bytes" - "log" - "github.com/jarcoal/httpmock" "github.com/maxatome/go-testdeep/td" "github.com/ovh/ovhcloud-cli/internal/cmd" @@ -169,50 +166,3 @@ func (ms *MockSuite) TestBaremetalReinstallDryRun(assert, require *td.T) { assert.Cmp(httpmock.GetCallCountInfo()["POST https://eu.api.ovh.com/v1/dedicated/server/fakeBaremetal/reinstall"], 0, "no reinstall call must reach the API") } - -// 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/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"`)) +} From 4ff961da25b6c4cbf4ad923152b2d4b4a48bf4f3 Mon Sep 17 00:00:00 2001 From: Denis Date: Mon, 24 Aug 2026 12:09:51 +0200 Subject: [PATCH 7/7] chore(doc): regenerate after bringing in the parent branch Signed-off-by: Denis --- doc/ovhcloud.md | 125 ++++++++--------------------------------- doc/ovhcloud_logout.md | 46 +++++++++++++++ 2 files changed, 69 insertions(+), 102 deletions(-) create mode 100644 doc/ovhcloud_logout.md 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_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 +