From d2de4c3a506c4c92ba452e933e382149dc8d8748 Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Tue, 18 Aug 2026 17:02:04 +0200 Subject: [PATCH 1/9] 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/9] 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 1b2d0d6fbb1d678b23832b5746b85b0c0e60fba9 Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Wed, 19 Aug 2026 15:08:21 +0200 Subject: [PATCH 3/9] feat(ip): move a failover IP, and say where it can go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An additional IP is bought to be moved. That is what makes it additional: it follows a service rather than a machine, so a failover survives the server under it. The CLI could list those IPs and show what each was routed to, and could move none of them — `/ip/{ip}/move` and `/ip/{ip}/park` were among the 66 routes of this domain it did not reach. ovhcloud ip destinations ovhcloud ip move [--nexthop] [--wait] [--yes|--dry-run] ovhcloud ip park [--wait] [--yes|--dry-run] ovhcloud ip tasks `destinations` is what makes the rest usable. The API answers a map keyed by service family, and on a real account one address accepted a hundred of them — 33 cloud projects, 33 VPS, 22 dedicated servers, 12 load balancers. Families that accept nothing come back as empty lists rather than being absent, so they are dropped: four empty sections answer the question worse than three full ones. `move` validates the destination against that same list before sending anything. A service that does not accept the IP is refused locally, naming the families and their counts, because the alternative is a 400 several seconds later with no hint of which names would have worked. The individual services are deliberately not printed — there can be a hundred — so the refusal names the command that lists them. The guard is `Disruptive`: the traffic that stops is the traffic of the service the IP is leaving, and that service is the one thing the operator cannot see from the command they typed, so the prompt names it. `--wait` reads where the IP is routed rather than following the task. Measured on a real move: a minute after the API accepted the request, the task existed and `routedTo` still pointed at the old server. A wait that trusts the task it was handed can report success while the work is unfinished — the same shape as the vRack attach in #249, where one request produced two tasks. Verified on a real failover address, moved and put back: move to ns3118333.ip-51-68-100.eu 1 min 32 s, exit 0 move back to ns3018397… 1 min 06 s, exit 0 final state, read outside the CLI identical to the start The four refusal paths were run against the live API: unknown destination, --dry-run, an IP that serves nothing, and refusal without --yes. Signed-off-by: Denis Hamon Co-Authored-By: Claude Opus 5 (1M context) --- doc/ovhcloud_ip.md | 4 + doc/ovhcloud_ip_destinations.md | 37 ++ doc/ovhcloud_ip_move.md | 41 ++ doc/ovhcloud_ip_park.md | 40 ++ doc/ovhcloud_ip_tasks.md | 44 +++ internal/cmd/ip.go | 44 +++ internal/cmd/ip_move_test.go | 91 +++++ internal/services/ip/move.go | 367 ++++++++++++++++++ internal/services/ip/move_test.go | 163 ++++++++ .../services/ip/templates/destinations.tmpl | 9 + 10 files changed, 840 insertions(+) create mode 100644 doc/ovhcloud_ip_destinations.md create mode 100644 doc/ovhcloud_ip_move.md create mode 100644 doc/ovhcloud_ip_park.md create mode 100644 doc/ovhcloud_ip_tasks.md create mode 100644 internal/cmd/ip_move_test.go create mode 100644 internal/services/ip/move.go create mode 100644 internal/services/ip/move_test.go create mode 100644 internal/services/ip/templates/destinations.tmpl diff --git a/doc/ovhcloud_ip.md b/doc/ovhcloud_ip.md index 0ef2d6ef2..12c45f541 100644 --- a/doc/ovhcloud_ip.md +++ b/doc/ovhcloud_ip.md @@ -30,9 +30,13 @@ Retrieve information and manage your IP services ### SEE ALSO * [ovhcloud](ovhcloud.md) - CLI to manage your OVHcloud services +* [ovhcloud ip destinations](ovhcloud_ip_destinations.md) - List the services this IP can be moved to * [ovhcloud ip edit](ovhcloud_ip_edit.md) - Edit the given IP * [ovhcloud ip firewall](ovhcloud_ip_firewall.md) - Manage firewall (Edge Firewall) on the given IP * [ovhcloud ip get](ovhcloud_ip_get.md) - Retrieve information of a specific Ip * [ovhcloud ip list](ovhcloud_ip_list.md) - List your Ip services +* [ovhcloud ip move](ovhcloud_ip_move.md) - Route the given IP to another service +* [ovhcloud ip park](ovhcloud_ip_park.md) - Detach the given IP from the service it currently serves * [ovhcloud ip reverse](ovhcloud_ip_reverse.md) - Manage reverses on the given IP +* [ovhcloud ip tasks](ovhcloud_ip_tasks.md) - List the tasks of the given IP diff --git a/doc/ovhcloud_ip_destinations.md b/doc/ovhcloud_ip_destinations.md new file mode 100644 index 000000000..f6fff56de --- /dev/null +++ b/doc/ovhcloud_ip_destinations.md @@ -0,0 +1,37 @@ +## ovhcloud ip destinations + +List the services this IP can be moved to + +``` +ovhcloud ip destinations [flags] +``` + +### Options + +``` + -h, --help help for destinations +``` + +### 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 ip](ovhcloud_ip.md) - Retrieve information and manage your IP services + diff --git a/doc/ovhcloud_ip_move.md b/doc/ovhcloud_ip_move.md new file mode 100644 index 000000000..20fe83ca6 --- /dev/null +++ b/doc/ovhcloud_ip_move.md @@ -0,0 +1,41 @@ +## ovhcloud ip move + +Route the given IP to another service + +``` +ovhcloud ip move [flags] +``` + +### Options + +``` + --dry-run Print the call that would be made without making it + -h, --help help for move + --nexthop string Next hop to use, when the destination offers several + --wait Wait until the IP is actually routed to the destination before exiting + -y, --yes Skip the confirmation prompt (required for unattended runs) +``` + +### 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 ip](ovhcloud_ip.md) - Retrieve information and manage your IP services + diff --git a/doc/ovhcloud_ip_park.md b/doc/ovhcloud_ip_park.md new file mode 100644 index 000000000..73cfabdeb --- /dev/null +++ b/doc/ovhcloud_ip_park.md @@ -0,0 +1,40 @@ +## ovhcloud ip park + +Detach the given IP from the service it currently serves + +``` +ovhcloud ip park [flags] +``` + +### Options + +``` + --dry-run Print the call that would be made without making it + -h, --help help for park + --wait Wait until the IP is actually parked before exiting + -y, --yes Skip the confirmation prompt (required for unattended runs) +``` + +### 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 ip](ovhcloud_ip.md) - Retrieve information and manage your IP services + diff --git a/doc/ovhcloud_ip_tasks.md b/doc/ovhcloud_ip_tasks.md new file mode 100644 index 000000000..7257beb7a --- /dev/null +++ b/doc/ovhcloud_ip_tasks.md @@ -0,0 +1,44 @@ +## ovhcloud ip tasks + +List the tasks of the given IP + +``` +ovhcloud ip tasks [flags] +``` + +### Options + +``` + --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 tasks +``` + +### 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 ip](ovhcloud_ip.md) - Retrieve information and manage your IP services + diff --git a/internal/cmd/ip.go b/internal/cmd/ip.go index 364d62d35..a6a2278b2 100644 --- a/internal/cmd/ip.go +++ b/internal/cmd/ip.go @@ -47,6 +47,50 @@ func init() { addInteractiveEditorFlag(ipEditCmd) ipCmd.AddCommand(ipEditCmd) + // An additional IP is bought to be moved between services; the CLI could + // show where each one pointed and move none of them. + ipCmd.AddCommand(&cobra.Command{ + Use: "destinations ", + Short: "List the services this IP can be moved to", + Args: cobra.ExactArgs(1), + ValidArgsFunction: completion.ServiceList("/v1/ip"), + Run: ip.ListIpDestinations, + }) + + ipMoveCmd := &cobra.Command{ + Use: "move ", + Short: "Route the given IP to another service", + Args: cobra.ExactArgs(2), + ValidArgsFunction: completion.ServiceList("/v1/ip"), + Run: ip.MoveIp, + } + ipMoveCmd.Flags().StringVar(&ip.MoveNexthop, "nexthop", "", + "Next hop to use, when the destination offers several") + ipMoveCmd.Flags().BoolVar(&ip.IPWait, "wait", false, + "Wait until the IP is actually routed to the destination before exiting") + addConfirmationFlags(ipMoveCmd, "Print the call that would be made without making it") + ipCmd.AddCommand(ipMoveCmd) + + ipParkCmd := &cobra.Command{ + Use: "park ", + Short: "Detach the given IP from the service it currently serves", + Args: cobra.ExactArgs(1), + ValidArgsFunction: completion.ServiceList("/v1/ip"), + Run: ip.ParkIp, + } + ipParkCmd.Flags().BoolVar(&ip.IPWait, "wait", false, + "Wait until the IP is actually parked before exiting") + addConfirmationFlags(ipParkCmd, "Print the call that would be made without making it") + ipCmd.AddCommand(ipParkCmd) + + ipCmd.AddCommand(withFilterFlag(&cobra.Command{ + Use: "tasks ", + Short: "List the tasks of the given IP", + Args: cobra.ExactArgs(1), + ValidArgsFunction: completion.ServiceList("/v1/ip"), + Run: ip.ListIpTasks, + })) + ipReverseCmd := &cobra.Command{ Use: "reverse", Short: "Manage reverses on the given IP", diff --git a/internal/cmd/ip_move_test.go b/internal/cmd/ip_move_test.go new file mode 100644 index 000000000..6cc3e0074 --- /dev/null +++ b/internal/cmd/ip_move_test.go @@ -0,0 +1,91 @@ +// SPDX-FileCopyrightText: 2025 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +package cmd_test + +import ( + "github.com/jarcoal/httpmock" + "github.com/maxatome/go-testdeep/td" + "github.com/ovh/ovhcloud-cli/internal/cmd" +) + +const ipMoveDestinations = `{ + "dedicatedServer": [ + {"service": "ns3018397.ip-57-128-116.eu", "nexthop": []}, + {"service": "ns3118333.ip-51-68-100.eu", "nexthop": []} + ], + "vps": [{"service": "vps-c924a68c.vps.ovh.net", "nexthop": ["1.2.3.4"]}], + "dedicatedCloud": [], + "hostingReseller": [] +}` + +func registerIpMoveDestinations() { + httpmock.RegisterResponder("GET", + "https://eu.api.ovh.com/v1/ip/135.125.71.80%2F32/move", + httpmock.NewStringResponder(200, ipMoveDestinations)) +} + +// A family that accepts nothing is returned as an empty list rather than being +// absent. Showing it would be four empty sections in a table whose whole point +// is to say where the IP can go. +func (ms *MockSuite) TestIpDestinationsDropsEmptyFamilies(assert, require *td.T) { + registerIpMoveDestinations() + + out, err := cmd.Execute("ip", "destinations", "135.125.71.80/32") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("ns3118333.ip-51-68-100.eu")) + assert.Cmp(out, td.Contains("vps-c924a68c.vps.ovh.net")) + assert.Cmp(out, td.Not(td.Contains("hostingReseller")), "an empty family is not a destination") + assert.Cmp(out, td.Not(td.Contains("dedicatedCloud"))) +} + +// The destination is checked before the request. The API would answer 400 +// several seconds later without saying which names would have worked, and the +// CLI is holding that list already. +func (ms *MockSuite) TestIpMoveRefusesAnImpossibleDestinationLocally(assert, require *td.T) { + registerIpMoveDestinations() + + _, err := cmd.Execute("ip", "move", "135.125.71.80/32", "ns9999999.example", "--yes") + + require.CmpError(err) + assert.Cmp(err.Error(), td.Contains("ns9999999.example")) + assert.Cmp(err.Error(), td.Contains("2 dedicatedServer"), "the families are counted") + assert.Cmp(err.Error(), td.Contains("ovhcloud ip destinations"), "and the way out is named") + + assert.Cmp(httpmock.GetTotalCallCount() > 0, true) + for call := range httpmock.GetCallCountInfo() { + assert.Cmp(call, td.Not(td.HasPrefix("POST")), "nothing may be sent for a refused move") + } +} + +// --dry-run prints the call and sends nothing. +func (ms *MockSuite) TestIpMoveDryRunSendsNothing(assert, require *td.T) { + registerIpMoveDestinations() + + out, err := cmd.Execute("ip", "move", "135.125.71.80/32", "ns3118333.ip-51-68-100.eu", "--dry-run") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("Dry run")) + assert.Cmp(out, td.Contains("ns3118333.ip-51-68-100.eu")) + for call := range httpmock.GetCallCountInfo() { + assert.Cmp(call, td.Not(td.HasPrefix("POST"))) + } +} + +// Parking an IP that serves nothing is not a failure and not a request: it is +// already in the state being asked for. +func (ms *MockSuite) TestIpParkSaysWhenThereIsNothingToPark(assert, require *td.T) { + httpmock.RegisterResponder("GET", + "https://eu.api.ovh.com/v1/ip/135.125.71.80%2F32", + httpmock.NewStringResponder(200, `{"ip": "135.125.71.80/32", "routedTo": {"serviceName": ""}}`)) + + out, err := cmd.Execute("ip", "park", "135.125.71.80/32", "--yes") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("nothing to park")) + for call := range httpmock.GetCallCountInfo() { + assert.Cmp(call, td.Not(td.HasPrefix("POST"))) + } +} diff --git a/internal/services/ip/move.go b/internal/services/ip/move.go new file mode 100644 index 000000000..fd781e0f2 --- /dev/null +++ b/internal/services/ip/move.go @@ -0,0 +1,367 @@ +// SPDX-FileCopyrightText: 2025 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +package ip + +import ( + _ "embed" + "fmt" + "log" + "net/url" + "sort" + "strings" + "time" + + "github.com/ovh/ovhcloud-cli/internal/display" + "github.com/ovh/ovhcloud-cli/internal/flags" + httpLib "github.com/ovh/ovhcloud-cli/internal/http" + "github.com/ovh/ovhcloud-cli/internal/services/common" + "github.com/spf13/cobra" +) + +// An additional IP is bought to be moved. That is what makes it additional: it +// follows a service rather than a machine, so a failover survives the server +// under it. The CLI could list those IPs and show what each was routed to, and +// could not move a single one. +// +// The move is guarded because it is the traffic of the service it currently +// serves that stops. It is validated first because the API takes a service +// name and the account has eighty of them: a typo there is a 400 several +// seconds later, when the CLI already holds the list of the ones that work. + +//go:embed templates/destinations.tmpl +var destinationsTemplate string + +var ( + // MoveNexthop selects a next hop when the destination offers several. + MoveNexthop string + + // IPWait follows the move until the IP is actually routed. + IPWait bool +) + +// movePollInterval and movePollAttempts bound how long --wait follows a move. +// +// They are variables rather than constants so a test can exercise the timeout +// in milliseconds instead of in ten minutes. +var ( + movePollInterval = 5 * time.Second + movePollAttempts = 120 +) + +// destination is one place an IP can go. +type destination struct { + Family string `json:"family"` + Service string `json:"service"` + Nexthop []string `json:"nexthop"` +} + +// destinationsOf reads where an IP may be routed. +// +// The API answers an object keyed by service family — vps, dedicatedServer, +// cloudProject and four more — each holding its own list. Families that accept +// nothing come back empty rather than absent, so they are dropped here: a table +// with four empty sections answers the question worse than one with three full +// ones. +func destinationsOf(ip string) ([]destination, error) { + var families map[string][]struct { + Service string `json:"service"` + Nexthop []string `json:"nexthop"` + } + + path := fmt.Sprintf("/v1/ip/%s/move", url.PathEscape(ip)) + if err := httpLib.Client.Get(path, &families); err != nil { + return nil, fmt.Errorf("failed to read where %s can be moved: %w", ip, err) + } + + var destinations []destination + for family, services := range families { + for _, service := range services { + destinations = append(destinations, destination{ + Family: family, + Service: service.Service, + Nexthop: service.Nexthop, + }) + } + } + + sortDestinations(destinations) + + return destinations, nil +} + +// sortDestinations groups the answer by family, then by service. +// +// The API returns a map, so its order is whatever Go's iteration gives that +// run: without this the same command printed a hundred rows in a different +// order every time, which makes two readings impossible to compare. +func sortDestinations(destinations []destination) { + sort.Slice(destinations, func(i, j int) bool { + if destinations[i].Family != destinations[j].Family { + return destinations[i].Family < destinations[j].Family + } + return destinations[i].Service < destinations[j].Service + }) +} + +// ListIpDestinations shows where an IP can be moved. +func ListIpDestinations(_ *cobra.Command, args []string) { + ip := args[0] + + destinations, err := destinationsOf(ip) + if err != nil { + display.OutputError(&flags.OutputFormatConfig, "%s", err) + return + } + + if len(destinations) == 0 { + display.OutputInfo(&flags.OutputFormatConfig, + map[string]any{"ip": ip, "destinations": 0}, + "%s cannot be moved to any service.", ip) + return + } + + rows := make([]map[string]any, 0, len(destinations)) + families := map[string]int{} + for _, d := range destinations { + families[d.Family]++ + rows = append(rows, map[string]any{ + "family": d.Family, + "service": d.Service, + "nexthop": strings.Join(d.Nexthop, ", "), + }) + } + + summary := make([]string, 0, len(families)) + for _, family := range sortedKeys(families) { + summary = append(summary, fmt.Sprintf("%d %s", families[family], family)) + } + + display.OutputObject(map[string]any{ + "summary": strings.Join(summary, " · "), + "destinations": rows, + }, ip, destinationsTemplate, &flags.OutputFormatConfig) +} + +// MoveIp routes an IP to another service. +func MoveIp(_ *cobra.Command, args []string) { + ip, target := args[0], args[1] + + // The destination is checked here rather than by the API. The CLI is about + // to ask for this list anyway to know the next hops, and a service name + // that does not accept this IP comes back as a 400 several seconds later — + // with no hint of which names would have worked. + destinations, err := destinationsOf(ip) + if err != nil { + display.OutputError(&flags.OutputFormatConfig, "%s", err) + return + } + + chosen, ok := pickDestination(destinations, target) + if !ok { + display.OutputError(&flags.OutputFormatConfig, "%s", unknownDestination(ip, target, destinations)) + return + } + + from := routedTo(ip) + if !common.ConfirmAction(common.Disruptive, ip, moveWarning(ip, from, chosen)) { + display.OutputError(&flags.OutputFormatConfig, "move of %s cancelled", ip) + return + } + + endpoint := fmt.Sprintf("/v1/ip/%s/move", url.PathEscape(ip)) + // The destination travels in the endpoint string here. #242 adds a Detail + // field for exactly this and is not in this branch's ancestry; when both + // land, this belongs there so that -o json keeps endpoint a path. + if common.ReportDryRun(common.Call{ + Method: "POST", + Endpoint: endpoint + " (to " + chosen.Service + ")", + }) { + return + } + + payload := map[string]any{"to": chosen.Service} + if MoveNexthop != "" { + payload["nexthop"] = MoveNexthop + } + + var task map[string]any + if err := httpLib.Client.Post(endpoint, payload, &task); err != nil { + display.OutputError(&flags.OutputFormatConfig, "failed to move %s to %s: %s", ip, chosen.Service, err) + return + } + + log.Printf("⚡️ %s is moving to %s…", ip, chosen.Service) + + if !IPWait { + display.OutputInfo(&flags.OutputFormatConfig, + map[string]any{"ip": ip, "to": chosen.Service, "task": task["taskId"]}, + "⚡️ %s is being moved to %s. Follow it with: ovhcloud ip tasks %s", ip, chosen.Service, ip) + return + } + + if err := waitForRouting(ip, chosen.Service); err != nil { + display.OutputError(&flags.OutputFormatConfig, "%s", err) + return + } + + display.OutputInfo(&flags.OutputFormatConfig, + map[string]any{"ip": ip, "routedTo": chosen.Service}, + "✅ %s is now routed to %s.", ip, chosen.Service) +} + +// ParkIp detaches an IP from whatever it currently serves. +func ParkIp(_ *cobra.Command, args []string) { + ip := args[0] + + from := routedTo(ip) + if from == "" { + display.OutputInfo(&flags.OutputFormatConfig, + map[string]any{"ip": ip, "routedTo": nil}, + "%s is not routed to any service, so there is nothing to park.", ip) + return + } + + if !common.ConfirmAction(common.Disruptive, ip, fmt.Sprintf( + "Parking %s stops the traffic it carries for %s, and routes it nowhere.", ip, from)) { + display.OutputError(&flags.OutputFormatConfig, "parking of %s cancelled", ip) + return + } + + endpoint := fmt.Sprintf("/v1/ip/%s/park", url.PathEscape(ip)) + if common.ReportDryRun(common.Call{Method: "POST", Endpoint: endpoint}) { + return + } + + if err := httpLib.Client.Post(endpoint, nil, nil); err != nil { + display.OutputError(&flags.OutputFormatConfig, "failed to park %s: %s", ip, err) + return + } + + log.Printf("⚡️ %s is being parked…", ip) + + if !IPWait { + display.OutputInfo(&flags.OutputFormatConfig, + map[string]any{"ip": ip}, "⚡️ %s is being parked.", ip) + return + } + + if err := waitForRouting(ip, ""); err != nil { + display.OutputError(&flags.OutputFormatConfig, "%s", err) + return + } + + display.OutputInfo(&flags.OutputFormatConfig, + map[string]any{"ip": ip, "routedTo": nil}, "✅ %s is parked.", ip) +} + +// ListIpTasks lists the tasks of an IP. +func ListIpTasks(_ *cobra.Command, args []string) { + common.ManageListRequest(fmt.Sprintf("/v1/ip/%s/task", url.PathEscape(args[0])), "", + []string{"taskId", "function", "status", "startDate", "doneDate", "comment"}, + flags.GenericFilters) +} + +// waitForRouting follows a move by reading where the IP is routed, not by +// reading the task. +// +// The task says the work finished; only the IP says where it landed. A vRack +// attach measured earlier in this CLI created two tasks for one request, so a +// wait that trusts the task it was handed can report success while the other +// half is still running. Reading the state costs one call and cannot be wrong +// about the thing it is asked. +// +// An empty want means parked. +func waitForRouting(ip, want string) error { + for attempt := 0; attempt < movePollAttempts; attempt++ { + if current := routedTo(ip); current == want { + return nil + } + + time.Sleep(movePollInterval) + } + + where := "parked" + if want != "" { + where = "routed to " + want + } + + return fmt.Errorf("stopped waiting after %s; %s is not %s yet, follow it with: ovhcloud ip tasks %s", + time.Duration(movePollAttempts)*movePollInterval, ip, where, ip) +} + +// routedTo answers the service an IP currently serves, and an empty string when +// it serves none. +// +// A failure to read is reported as "no service" on purpose: this feeds a +// confirmation prompt and a wait, and neither is improved by turning a +// transient read error into a failed command. The prompt says "not routed" +// rather than naming a service it could not confirm. +func routedTo(ip string) string { + var block struct { + RoutedTo struct { + ServiceName string `json:"serviceName"` + } `json:"routedTo"` + } + + if err := httpLib.Client.Get(fmt.Sprintf("/v1/ip/%s", url.PathEscape(ip)), &block); err != nil { + return "" + } + + return block.RoutedTo.ServiceName +} + +// pickDestination finds the requested service among the ones that accept this IP. +func pickDestination(destinations []destination, target string) (destination, bool) { + for _, candidate := range destinations { + if strings.EqualFold(candidate.Service, target) { + return candidate, true + } + } + + return destination{}, false +} + +// unknownDestination refuses a service this IP cannot reach, and names the ones +// it can. +// +// The full list is not printed: an account measured while writing this offered +// a hundred destinations for one address. The families are named with their +// counts, and the command that prints them is given. +func unknownDestination(ip, target string, destinations []destination) error { + families := map[string]int{} + for _, d := range destinations { + families[d.Family]++ + } + + summary := make([]string, 0, len(families)) + for _, family := range sortedKeys(families) { + summary = append(summary, fmt.Sprintf("%d %s", families[family], family)) + } + + return fmt.Errorf("%s does not accept %s.\n It can be moved to %s — list them with: ovhcloud ip destinations %s", + target, ip, strings.Join(summary, ", "), ip) +} + +// moveWarning is the sentence somebody reads before the traffic stops. It is +// built here rather than inline so a test can read the wording that stands +// between an operator and a service going dark. +func moveWarning(ip, from string, to destination) string { + if from == "" { + return fmt.Sprintf("%s is not routed to any service; moving it will route it to %s (%s).", + ip, to.Service, to.Family) + } + + return fmt.Sprintf("Moving %s to %s (%s) stops the traffic it carries for %s.", + ip, to.Service, to.Family, from) +} + +func sortedKeys(counts map[string]int) []string { + keys := make([]string, 0, len(counts)) + for key := range counts { + keys = append(keys, key) + } + sort.Strings(keys) + return keys +} diff --git a/internal/services/ip/move_test.go b/internal/services/ip/move_test.go new file mode 100644 index 000000000..2b1478539 --- /dev/null +++ b/internal/services/ip/move_test.go @@ -0,0 +1,163 @@ +// SPDX-FileCopyrightText: 2025 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +package ip + +import ( + "strings" + "testing" + "time" + + "github.com/jarcoal/httpmock" + "github.com/maxatome/go-testdeep/td" + "github.com/ovh/go-ovh/ovh" + httpLib "github.com/ovh/ovhcloud-cli/internal/http" +) + +func sample() []destination { + return []destination{ + {Family: "dedicatedServer", Service: "ns3018397.ip-57-128-116.eu"}, + {Family: "dedicatedServer", Service: "ns3118333.ip-51-68-100.eu"}, + {Family: "vps", Service: "vps-c924a68c.vps.ovh.net", Nexthop: []string{"1.2.3.4"}}, + } +} + +// The service name is what the operator copies out of another command's +// output, and case is not something they should have to reproduce. +func TestPickDestinationIgnoresCase(t *testing.T) { + chosen, ok := pickDestination(sample(), "NS3118333.IP-51-68-100.EU") + + td.Require(t).Cmp(ok, true) + td.Cmp(t, chosen.Service, "ns3118333.ip-51-68-100.eu") + td.Cmp(t, chosen.Family, "dedicatedServer") +} + +func TestPickDestinationRefusesWhatIsNotThere(t *testing.T) { + _, ok := pickDestination(sample(), "ns9999999.example") + + td.Cmp(t, ok, false) +} + +// The refusal has to be useful without being a wall of text: a real account +// offered a hundred destinations for one address, so the families are counted +// and the command that lists them is given. +func TestUnknownDestinationCountsRatherThanLists(t *testing.T) { + err := unknownDestination("135.125.71.80/32", "ns9999999.example", sample()) + + td.Require(t).CmpError(err) + message := err.Error() + + td.Cmp(t, message, td.Contains("ns9999999.example")) + td.Cmp(t, message, td.Contains("2 dedicatedServer")) + td.Cmp(t, message, td.Contains("1 vps")) + td.Cmp(t, message, td.Contains("ovhcloud ip destinations 135.125.71.80/32"), + "the way out has to be named") + td.Cmp(t, message, td.Not(td.Contains("vps-c924a68c")), + "individual services are not listed: there can be a hundred of them") +} + +// The sentence before a move must name the service whose traffic stops, not +// only the one it is going to. That service is the thing that goes dark, and +// it is the only part the operator cannot see from the command they typed. +func TestMoveWarningNamesTheServiceThatLosesTheIp(t *testing.T) { + warning := moveWarning("135.125.71.80/32", "ns3018397.ip-57-128-116.eu", sample()[1]) + + td.Cmp(t, warning, td.Contains("ns3018397.ip-57-128-116.eu"), "what it leaves") + td.Cmp(t, warning, td.Contains("ns3118333.ip-51-68-100.eu"), "where it goes") + td.Cmp(t, warning, td.Contains("dedicatedServer")) +} + +// An IP that serves nothing is a different sentence: there is no traffic to +// stop, and saying there is would be a warning about something that is not +// happening. +func TestMoveWarningSaysSoWhenNothingIsServed(t *testing.T) { + warning := moveWarning("135.125.71.80/32", "", sample()[1]) + + td.Cmp(t, warning, td.Contains("not routed to any service")) + td.Cmp(t, strings.Contains(warning, "stops the traffic"), false, + "nothing is being interrupted here") +} + +// The API answers a map, so its iteration order changes from run to run. Two +// readings of the same command have to be comparable, which they are not if a +// hundred rows arrive shuffled. +func TestDestinationsAreSortedByFamilyThenService(t *testing.T) { + destinations := []destination{ + {Family: "vps", Service: "b"}, + {Family: "dedicatedServer", Service: "z"}, + {Family: "vps", Service: "a"}, + {Family: "dedicatedServer", Service: "a"}, + } + + sortDestinations(destinations) + + td.Cmp(t, destinations, []destination{ + {Family: "dedicatedServer", Service: "a"}, + {Family: "dedicatedServer", Service: "z"}, + {Family: "vps", Service: "a"}, + {Family: "vps", Service: "b"}, + }) +} + +func TestSortedKeysAreOrdered(t *testing.T) { + td.Cmp(t, sortedKeys(map[string]int{"vps": 1, "dedicatedServer": 2, "cloudProject": 3}), + []string{"cloudProject", "dedicatedServer", "vps"}) +} + +// withIpAPI points the shared client at httpmock and shrinks the poll so the +// timeout path is reachable in a test instead of in ten minutes. +func withIpAPI(t *testing.T, attempts int, block string) { + t.Helper() + httpmock.Activate(t) + + origClient := httpLib.Client + origInterval, origAttempts := movePollInterval, movePollAttempts + client, err := ovh.NewClient("ovh-eu", "app_key", "app_secret", "consumer_key") + td.Require(t).CmpNoError(err) + httpLib.Client = client + movePollInterval, movePollAttempts = time.Millisecond, attempts + + t.Cleanup(func() { + httpLib.Client = origClient + movePollInterval, movePollAttempts = origInterval, origAttempts + }) + + // go-ovh computes its clock delta before the first signed call. + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/1.0/auth/time", + httpmock.NewStringResponder(200, "0")) + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/v1/ip/1.2.3.4%2F32", + httpmock.NewStringResponder(200, block)) +} + +// The wait reads where the IP is, not whether a task finished. A vRack attach +// measured earlier in this CLI produced two tasks for one request, so a wait +// that trusts the task it was handed can report success while the other half +// is still running. +func TestWaitForRoutingReturnsWhenTheIpHasActuallyMoved(t *testing.T) { + withIpAPI(t, 5, `{"ip": "1.2.3.4/32", "routedTo": {"serviceName": "ns3118333.ip-51-68-100.eu"}}`) + + td.CmpNoError(t, waitForRouting("1.2.3.4/32", "ns3118333.ip-51-68-100.eu")) +} + +// An empty destination is the parked state, and it has to be distinguishable +// from "still on the old service". +func TestWaitForRoutingRecognisesParked(t *testing.T) { + withIpAPI(t, 5, `{"ip": "1.2.3.4/32", "routedTo": {"serviceName": ""}}`) + + td.CmpNoError(t, waitForRouting("1.2.3.4/32", "")) +} + +// Giving up is not the move failing, and the message must not claim it is: +// the task may well still be running. Measured on a real move, the IP was +// still on its old service a minute after the request was accepted. +func TestWaitForRoutingTimesOutWithoutClaimingFailure(t *testing.T) { + withIpAPI(t, 2, `{"ip": "1.2.3.4/32", "routedTo": {"serviceName": "ns3018397.ip-57-128-116.eu"}}`) + + err := waitForRouting("1.2.3.4/32", "ns3118333.ip-51-68-100.eu") + + td.Require(t).CmpError(err) + td.Cmp(t, err.Error(), td.Contains("stopped waiting")) + td.Cmp(t, err.Error(), td.Contains("ovhcloud ip tasks"), "the way to follow it is named") + td.Cmp(t, err.Error(), td.Not(td.Contains("failed")), "the move was not observed to fail") +} diff --git a/internal/services/ip/templates/destinations.tmpl b/internal/services/ip/templates/destinations.tmpl new file mode 100644 index 000000000..d4ade389a --- /dev/null +++ b/internal/services/ip/templates/destinations.tmpl @@ -0,0 +1,9 @@ +🎯 {{.ServiceName}} can be moved to +======= + +**{{index .Result "summary"}}** + +| Family | Service | Next hop | +| --- | --- | --- | +{{range index .Result "destinations"}}| {{index . "family"}} | {{index . "service"}} | {{index . "nexthop"}} | +{{end}} From 0415ac508426d1fb23e70be62d27b22a46e792d0 Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Thu, 20 Aug 2026 16:14:32 +0200 Subject: [PATCH 4/9] chore: no account data as test fixtures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A hostname, an address, an order id and a colleague's name read off a live account had been used as fixtures and as examples in comments. This repository is public: what goes in stays in. The values are replaced with synthetic ones of the same shape — RFC 5737 documentation addresses, hostnames built on them, identifiers of the same length — so the tests keep exercising the same parsing. This is the fourth time in this series that live account data reached a commit, after a live IP migration token, a test server's address, and a real invoice id. The first three were fixed one at a time as they were noticed; this is the sweep that should have followed the first one. Signed-off-by: Denis Hamon Co-Authored-By: Claude Opus 5 (1M context) --- internal/cmd/ip_move_test.go | 22 +++++++++++----------- internal/services/ip/move_test.go | 26 +++++++++++++------------- internal/services/vrack/attach_test.go | 2 +- 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/internal/cmd/ip_move_test.go b/internal/cmd/ip_move_test.go index 6cc3e0074..2f6ba0d23 100644 --- a/internal/cmd/ip_move_test.go +++ b/internal/cmd/ip_move_test.go @@ -12,8 +12,8 @@ import ( const ipMoveDestinations = `{ "dedicatedServer": [ - {"service": "ns3018397.ip-57-128-116.eu", "nexthop": []}, - {"service": "ns3118333.ip-51-68-100.eu", "nexthop": []} + {"service": "ns0000005.ip-203-0-113.eu", "nexthop": []}, + {"service": "ns0000006.ip-203-0-113.eu", "nexthop": []} ], "vps": [{"service": "vps-c924a68c.vps.ovh.net", "nexthop": ["1.2.3.4"]}], "dedicatedCloud": [], @@ -22,7 +22,7 @@ const ipMoveDestinations = `{ func registerIpMoveDestinations() { httpmock.RegisterResponder("GET", - "https://eu.api.ovh.com/v1/ip/135.125.71.80%2F32/move", + "https://eu.api.ovh.com/v1/ip/203.0.113.80%2F32/move", httpmock.NewStringResponder(200, ipMoveDestinations)) } @@ -32,10 +32,10 @@ func registerIpMoveDestinations() { func (ms *MockSuite) TestIpDestinationsDropsEmptyFamilies(assert, require *td.T) { registerIpMoveDestinations() - out, err := cmd.Execute("ip", "destinations", "135.125.71.80/32") + out, err := cmd.Execute("ip", "destinations", "203.0.113.80/32") require.CmpNoError(err) - assert.Cmp(out, td.Contains("ns3118333.ip-51-68-100.eu")) + assert.Cmp(out, td.Contains("ns0000006.ip-203-0-113.eu")) assert.Cmp(out, td.Contains("vps-c924a68c.vps.ovh.net")) assert.Cmp(out, td.Not(td.Contains("hostingReseller")), "an empty family is not a destination") assert.Cmp(out, td.Not(td.Contains("dedicatedCloud"))) @@ -47,7 +47,7 @@ func (ms *MockSuite) TestIpDestinationsDropsEmptyFamilies(assert, require *td.T) func (ms *MockSuite) TestIpMoveRefusesAnImpossibleDestinationLocally(assert, require *td.T) { registerIpMoveDestinations() - _, err := cmd.Execute("ip", "move", "135.125.71.80/32", "ns9999999.example", "--yes") + _, err := cmd.Execute("ip", "move", "203.0.113.80/32", "ns9999999.example", "--yes") require.CmpError(err) assert.Cmp(err.Error(), td.Contains("ns9999999.example")) @@ -64,11 +64,11 @@ func (ms *MockSuite) TestIpMoveRefusesAnImpossibleDestinationLocally(assert, req func (ms *MockSuite) TestIpMoveDryRunSendsNothing(assert, require *td.T) { registerIpMoveDestinations() - out, err := cmd.Execute("ip", "move", "135.125.71.80/32", "ns3118333.ip-51-68-100.eu", "--dry-run") + out, err := cmd.Execute("ip", "move", "203.0.113.80/32", "ns0000006.ip-203-0-113.eu", "--dry-run") require.CmpNoError(err) assert.Cmp(out, td.Contains("Dry run")) - assert.Cmp(out, td.Contains("ns3118333.ip-51-68-100.eu")) + assert.Cmp(out, td.Contains("ns0000006.ip-203-0-113.eu")) for call := range httpmock.GetCallCountInfo() { assert.Cmp(call, td.Not(td.HasPrefix("POST"))) } @@ -78,10 +78,10 @@ func (ms *MockSuite) TestIpMoveDryRunSendsNothing(assert, require *td.T) { // already in the state being asked for. func (ms *MockSuite) TestIpParkSaysWhenThereIsNothingToPark(assert, require *td.T) { httpmock.RegisterResponder("GET", - "https://eu.api.ovh.com/v1/ip/135.125.71.80%2F32", - httpmock.NewStringResponder(200, `{"ip": "135.125.71.80/32", "routedTo": {"serviceName": ""}}`)) + "https://eu.api.ovh.com/v1/ip/203.0.113.80%2F32", + httpmock.NewStringResponder(200, `{"ip": "203.0.113.80/32", "routedTo": {"serviceName": ""}}`)) - out, err := cmd.Execute("ip", "park", "135.125.71.80/32", "--yes") + out, err := cmd.Execute("ip", "park", "203.0.113.80/32", "--yes") require.CmpNoError(err) assert.Cmp(out, td.Contains("nothing to park")) diff --git a/internal/services/ip/move_test.go b/internal/services/ip/move_test.go index 2b1478539..54fc84d24 100644 --- a/internal/services/ip/move_test.go +++ b/internal/services/ip/move_test.go @@ -17,8 +17,8 @@ import ( func sample() []destination { return []destination{ - {Family: "dedicatedServer", Service: "ns3018397.ip-57-128-116.eu"}, - {Family: "dedicatedServer", Service: "ns3118333.ip-51-68-100.eu"}, + {Family: "dedicatedServer", Service: "ns0000005.ip-203-0-113.eu"}, + {Family: "dedicatedServer", Service: "ns0000006.ip-203-0-113.eu"}, {Family: "vps", Service: "vps-c924a68c.vps.ovh.net", Nexthop: []string{"1.2.3.4"}}, } } @@ -29,7 +29,7 @@ func TestPickDestinationIgnoresCase(t *testing.T) { chosen, ok := pickDestination(sample(), "NS3118333.IP-51-68-100.EU") td.Require(t).Cmp(ok, true) - td.Cmp(t, chosen.Service, "ns3118333.ip-51-68-100.eu") + td.Cmp(t, chosen.Service, "ns0000006.ip-203-0-113.eu") td.Cmp(t, chosen.Family, "dedicatedServer") } @@ -43,7 +43,7 @@ func TestPickDestinationRefusesWhatIsNotThere(t *testing.T) { // offered a hundred destinations for one address, so the families are counted // and the command that lists them is given. func TestUnknownDestinationCountsRatherThanLists(t *testing.T) { - err := unknownDestination("135.125.71.80/32", "ns9999999.example", sample()) + err := unknownDestination("203.0.113.80/32", "ns9999999.example", sample()) td.Require(t).CmpError(err) message := err.Error() @@ -51,7 +51,7 @@ func TestUnknownDestinationCountsRatherThanLists(t *testing.T) { td.Cmp(t, message, td.Contains("ns9999999.example")) td.Cmp(t, message, td.Contains("2 dedicatedServer")) td.Cmp(t, message, td.Contains("1 vps")) - td.Cmp(t, message, td.Contains("ovhcloud ip destinations 135.125.71.80/32"), + td.Cmp(t, message, td.Contains("ovhcloud ip destinations 203.0.113.80/32"), "the way out has to be named") td.Cmp(t, message, td.Not(td.Contains("vps-c924a68c")), "individual services are not listed: there can be a hundred of them") @@ -61,10 +61,10 @@ func TestUnknownDestinationCountsRatherThanLists(t *testing.T) { // only the one it is going to. That service is the thing that goes dark, and // it is the only part the operator cannot see from the command they typed. func TestMoveWarningNamesTheServiceThatLosesTheIp(t *testing.T) { - warning := moveWarning("135.125.71.80/32", "ns3018397.ip-57-128-116.eu", sample()[1]) + warning := moveWarning("203.0.113.80/32", "ns0000005.ip-203-0-113.eu", sample()[1]) - td.Cmp(t, warning, td.Contains("ns3018397.ip-57-128-116.eu"), "what it leaves") - td.Cmp(t, warning, td.Contains("ns3118333.ip-51-68-100.eu"), "where it goes") + td.Cmp(t, warning, td.Contains("ns0000005.ip-203-0-113.eu"), "what it leaves") + td.Cmp(t, warning, td.Contains("ns0000006.ip-203-0-113.eu"), "where it goes") td.Cmp(t, warning, td.Contains("dedicatedServer")) } @@ -72,7 +72,7 @@ func TestMoveWarningNamesTheServiceThatLosesTheIp(t *testing.T) { // stop, and saying there is would be a warning about something that is not // happening. func TestMoveWarningSaysSoWhenNothingIsServed(t *testing.T) { - warning := moveWarning("135.125.71.80/32", "", sample()[1]) + warning := moveWarning("203.0.113.80/32", "", sample()[1]) td.Cmp(t, warning, td.Contains("not routed to any service")) td.Cmp(t, strings.Contains(warning, "stops the traffic"), false, @@ -135,9 +135,9 @@ func withIpAPI(t *testing.T, attempts int, block string) { // that trusts the task it was handed can report success while the other half // is still running. func TestWaitForRoutingReturnsWhenTheIpHasActuallyMoved(t *testing.T) { - withIpAPI(t, 5, `{"ip": "1.2.3.4/32", "routedTo": {"serviceName": "ns3118333.ip-51-68-100.eu"}}`) + withIpAPI(t, 5, `{"ip": "1.2.3.4/32", "routedTo": {"serviceName": "ns0000006.ip-203-0-113.eu"}}`) - td.CmpNoError(t, waitForRouting("1.2.3.4/32", "ns3118333.ip-51-68-100.eu")) + td.CmpNoError(t, waitForRouting("1.2.3.4/32", "ns0000006.ip-203-0-113.eu")) } // An empty destination is the parked state, and it has to be distinguishable @@ -152,9 +152,9 @@ func TestWaitForRoutingRecognisesParked(t *testing.T) { // the task may well still be running. Measured on a real move, the IP was // still on its old service a minute after the request was accepted. func TestWaitForRoutingTimesOutWithoutClaimingFailure(t *testing.T) { - withIpAPI(t, 2, `{"ip": "1.2.3.4/32", "routedTo": {"serviceName": "ns3018397.ip-57-128-116.eu"}}`) + withIpAPI(t, 2, `{"ip": "1.2.3.4/32", "routedTo": {"serviceName": "ns0000005.ip-203-0-113.eu"}}`) - err := waitForRouting("1.2.3.4/32", "ns3118333.ip-51-68-100.eu") + err := waitForRouting("1.2.3.4/32", "ns0000006.ip-203-0-113.eu") td.Require(t).CmpError(err) td.Cmp(t, err.Error(), td.Contains("stopped waiting")) 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 eb56fabfc3ac5f0318681855f779ac0f5bd35873 Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Thu, 20 Aug 2026 16:30:32 +0200 Subject: [PATCH 5/9] fix(ip): a routing that could not be read was reported as no routing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit routedTo swallowed every read error into an empty service name, and an empty service name is what this file uses to mean "this IP serves nothing". The two are opposites, and all three callers acted on the difference: - `park` answered "X is not routed to any service, so there is nothing to park" and exited 0. The IP may well have been carrying production traffic; what actually happened is that nobody could tell. - the move prompt lost the "this stops the traffic it carries for Y" warning at the one moment it matters — the last point where a wrong move can still be stopped. - worst, the wait loop. Parking waits for the routing to become empty, so a failed read WAS the answer it was waiting for: `park --wait` would report the park done because the check verifying it had failed. This whole stack is built on probing the state rather than trusting a task status, and this turned that principle against itself. routedTo now returns an error. park refuses, the prompt says the reading failed instead of implying the IP is free, and the wait loop does not count a failure as arrival. The test pins the distinction the prompt has to make: the two cases must not produce the same sentence. This commit also carries the case-variant fixture sweep for this file — the uppercase spelling of a real hostname lived in the same test. Found by a third-party review pass over this branch, which named the prompt. The wait loop was worse and is what the fix is really about. Signed-off-by: Denis Hamon Co-Authored-By: Claude Opus 5 (1M context) --- internal/services/ip/move.go | 47 +++++++++++++++++++++++++------ internal/services/ip/move_test.go | 26 +++++++++++++++-- 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/internal/services/ip/move.go b/internal/services/ip/move.go index fd781e0f2..f68efe7e8 100644 --- a/internal/services/ip/move.go +++ b/internal/services/ip/move.go @@ -164,8 +164,14 @@ func MoveIp(_ *cobra.Command, args []string) { return } - from := routedTo(ip) - if !common.ConfirmAction(common.Disruptive, ip, moveWarning(ip, from, chosen)) { + from, err := routedTo(ip) + if err != nil { + // The move itself does not depend on this read, so it is not a reason + // to refuse. But the prompt must not imply the IP is free when what + // actually happened is that nobody knows. + log.Printf("%s", err) + } + if !common.ConfirmAction(common.Disruptive, ip, moveWarning(ip, from, chosen, err != nil)) { display.OutputError(&flags.OutputFormatConfig, "move of %s cancelled", ip) return } @@ -215,7 +221,11 @@ func MoveIp(_ *cobra.Command, args []string) { func ParkIp(_ *cobra.Command, args []string) { ip := args[0] - from := routedTo(ip) + from, err := routedTo(ip) + if err != nil { + display.OutputError(&flags.OutputFormatConfig, "%s", err) + return + } if from == "" { display.OutputInfo(&flags.OutputFormatConfig, map[string]any{"ip": ip, "routedTo": nil}, @@ -275,9 +285,16 @@ func ListIpTasks(_ *cobra.Command, args []string) { // An empty want means parked. func waitForRouting(ip, want string) error { for attempt := 0; attempt < movePollAttempts; attempt++ { - if current := routedTo(ip); current == want { + // A read that failed is not a read that came back empty. Parking waits + // for exactly the empty string, so counting a failure as one would end + // the wait on the answer it was looking for and report the park done. + current, err := routedTo(ip) + if err == nil && current == want { return nil } + if err != nil { + log.Printf("%s", err) + } time.Sleep(movePollInterval) } @@ -298,7 +315,16 @@ func waitForRouting(ip, want string) error { // confirmation prompt and a wait, and neither is improved by turning a // transient read error into a failed command. The prompt says "not routed" // rather than naming a service it could not confirm. -func routedTo(ip string) string { +// routedTo returns the service this IP currently serves, and an empty string +// when it serves none. +// +// The error is returned rather than folded into that empty string, because the +// two mean opposite things and every caller here acts on the difference: "this +// IP is free" is a green light, "I could not find out" is not. Folding them +// made `park` answer "nothing to park" on a failed read, and made the wait +// loop below — whose target for a park IS the empty string — call the park a +// success because the check had failed. +func routedTo(ip string) (string, error) { var block struct { RoutedTo struct { ServiceName string `json:"serviceName"` @@ -306,10 +332,10 @@ func routedTo(ip string) string { } if err := httpLib.Client.Get(fmt.Sprintf("/v1/ip/%s", url.PathEscape(ip)), &block); err != nil { - return "" + return "", fmt.Errorf("failed to read what %s is routed to: %w", ip, err) } - return block.RoutedTo.ServiceName + return block.RoutedTo.ServiceName, nil } // pickDestination finds the requested service among the ones that accept this IP. @@ -347,7 +373,12 @@ func unknownDestination(ip, target string, destinations []destination) error { // moveWarning is the sentence somebody reads before the traffic stops. It is // built here rather than inline so a test can read the wording that stands // between an operator and a service going dark. -func moveWarning(ip, from string, to destination) string { +func moveWarning(ip, from string, to destination, unknown bool) string { + if unknown { + return fmt.Sprintf("Could not read what %s currently serves; moving it to %s (%s) will stop whatever traffic it carries.", + ip, to.Service, to.Family) + } + if from == "" { return fmt.Sprintf("%s is not routed to any service; moving it will route it to %s (%s).", ip, to.Service, to.Family) diff --git a/internal/services/ip/move_test.go b/internal/services/ip/move_test.go index 54fc84d24..219954d5d 100644 --- a/internal/services/ip/move_test.go +++ b/internal/services/ip/move_test.go @@ -26,7 +26,7 @@ func sample() []destination { // The service name is what the operator copies out of another command's // output, and case is not something they should have to reproduce. func TestPickDestinationIgnoresCase(t *testing.T) { - chosen, ok := pickDestination(sample(), "NS3118333.IP-51-68-100.EU") + chosen, ok := pickDestination(sample(), "NS0000006.IP-203-0-113.EU") td.Require(t).Cmp(ok, true) td.Cmp(t, chosen.Service, "ns0000006.ip-203-0-113.eu") @@ -61,7 +61,7 @@ func TestUnknownDestinationCountsRatherThanLists(t *testing.T) { // only the one it is going to. That service is the thing that goes dark, and // it is the only part the operator cannot see from the command they typed. func TestMoveWarningNamesTheServiceThatLosesTheIp(t *testing.T) { - warning := moveWarning("203.0.113.80/32", "ns0000005.ip-203-0-113.eu", sample()[1]) + warning := moveWarning("203.0.113.80/32", "ns0000005.ip-203-0-113.eu", sample()[1], false) td.Cmp(t, warning, td.Contains("ns0000005.ip-203-0-113.eu"), "what it leaves") td.Cmp(t, warning, td.Contains("ns0000006.ip-203-0-113.eu"), "where it goes") @@ -72,7 +72,7 @@ func TestMoveWarningNamesTheServiceThatLosesTheIp(t *testing.T) { // stop, and saying there is would be a warning about something that is not // happening. func TestMoveWarningSaysSoWhenNothingIsServed(t *testing.T) { - warning := moveWarning("203.0.113.80/32", "", sample()[1]) + warning := moveWarning("203.0.113.80/32", "", sample()[1], false) td.Cmp(t, warning, td.Contains("not routed to any service")) td.Cmp(t, strings.Contains(warning, "stops the traffic"), false, @@ -161,3 +161,23 @@ func TestWaitForRoutingTimesOutWithoutClaimingFailure(t *testing.T) { td.Cmp(t, err.Error(), td.Contains("ovhcloud ip tasks"), "the way to follow it is named") td.Cmp(t, err.Error(), td.Not(td.Contains("failed")), "the move was not observed to fail") } + +// An unread routing is not an empty routing. The prompt is the last place a +// wrong move can still be stopped, and "not routed to any service" is a green +// light: it must never be what a failed read produces. +func TestAnUnreadRoutingIsNotAnEmptyOne(t *testing.T) { + unknown := moveWarning("203.0.113.80/32", "", sample()[1], true) + if strings.Contains(unknown, "is not routed to any service") { + t.Fatalf("a failed read must not read as a free IP: %q", unknown) + } + if !strings.Contains(unknown, "Could not read") { + t.Fatalf("the prompt has to say the reading failed, got %q", unknown) + } + free := moveWarning("203.0.113.80/32", "", sample()[1], false) + if !strings.Contains(free, "is not routed to any service") { + t.Fatalf("an IP really routed nowhere still says so, got %q", free) + } + if free == unknown { + t.Fatal("the two cases produce the same sentence, so the prompt cannot tell them apart") + } +} From a3ed115cd806f722c6e6158516aad2784d664228 Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Fri, 21 Aug 2026 13:35:59 +0200 Subject: [PATCH 6/9] fix(ip): a failed move waited ten minutes and then read as still running MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit waitForRouting had no terminal-failure branch. A move or park whose task ends in cancelled, customerError or ovhError leaves the IP where it was, so the state reads "not there yet" for as long as anyone asks: the wait ran its full ten minutes and finished with "is not routed to X yet, follow it with ovhcloud ip tasks", which reads as work still in progress. Ten minutes and a misleading sentence for something the API knew in seconds. The task is now consulted, and consulted as a complement — never a substitute. The file already argued why: a vRack attach measured earlier in this CLI created two tasks for one request, so a wait that concluded from the task alone could report success while the other half was still running. So the state is read first every round, the task is asked only about failure, and the state is read once more before any failure is announced — if the IP has arrived in the meantime, that wins. The three statuses treated as terminal are retyped in Go because the schema lists the seven without saying which are failures; a guard test holds them against the enum so a rename shows up red instead of silently disabling the branch. `park` was said to have no task to watch. It has: POST /ip/{ip}/park answers with an ip.IpTask, which the code discarded. Keeping it makes both halves symmetric. The identifier is read through a json.Number case, because go-ovh decodes with UseNumber — a type switch handling only float64 is dead code, which this repository has already shipped twice. Also here, the two behavioural halves of eb56fab that had no test: a park is never concluded from a failed read, and an IP whose routing cannot be read is never reported as one that serves nothing. "Nothing to park" reads as "already done". Eight tests, five sabotages, five reds — after two of the five first came back green for the wrong reasons. One broke the build, which proves nothing about a test's teeth; the other exposed a genuinely vacuous test, whose first state read already matched, so the re-read it was written for never ran. Both rewritten. Signed-off-by: Denis Hamon Co-Authored-By: Claude Opus 5 (1M context) --- internal/cmd/ip_move_test.go | 21 +++++ internal/services/ip/move.go | 122 ++++++++++++++++++++++++++---- internal/services/ip/move_test.go | 111 ++++++++++++++++++++++++++- 3 files changed, 235 insertions(+), 19 deletions(-) diff --git a/internal/cmd/ip_move_test.go b/internal/cmd/ip_move_test.go index 2f6ba0d23..d583d0325 100644 --- a/internal/cmd/ip_move_test.go +++ b/internal/cmd/ip_move_test.go @@ -89,3 +89,24 @@ func (ms *MockSuite) TestIpParkSaysWhenThereIsNothingToPark(assert, require *td. assert.Cmp(call, td.Not(td.HasPrefix("POST"))) } } + +// The other half of eb56fab that had no test. "Nothing to park" is the sentence +// an operator reads as "already done, move on", and a failed read must never +// produce it: parking waits for the empty string, and the empty string is also +// what a folded-in error looked like. So an unreadable IP stops the command +// instead of being reported as one that serves nothing. +func (ms *MockSuite) TestIpParkRefusesWhenItCannotTellWhatIsServed(assert, require *td.T) { + httpmock.RegisterResponder("GET", + "https://eu.api.ovh.com/v1/ip/203.0.113.80%2F32", + httpmock.NewStringResponder(500, `{"message": "gateway is having a day"}`)) + + _, err := cmd.Execute("ip", "park", "203.0.113.80/32", "--yes") + + require.CmpError(err) + assert.Cmp(err.Error(), td.Contains("failed to read what 203.0.113.80/32 is routed to")) + assert.Cmp(err.Error(), td.Not(td.Contains("nothing to park")), + "an unread IP is never an unrouted one") + for call := range httpmock.GetCallCountInfo() { + assert.Cmp(call, td.Not(td.HasPrefix("POST")), "and nothing is sent") + } +} diff --git a/internal/services/ip/move.go b/internal/services/ip/move.go index f68efe7e8..c74a7f0e4 100644 --- a/internal/services/ip/move.go +++ b/internal/services/ip/move.go @@ -6,9 +6,11 @@ package ip import ( _ "embed" + "encoding/json" "fmt" "log" "net/url" + "slices" "sort" "strings" "time" @@ -207,7 +209,7 @@ func MoveIp(_ *cobra.Command, args []string) { return } - if err := waitForRouting(ip, chosen.Service); err != nil { + if err := waitForRouting(ip, chosen.Service, taskIDOf(task)); err != nil { display.OutputError(&flags.OutputFormatConfig, "%s", err) return } @@ -244,7 +246,11 @@ func ParkIp(_ *cobra.Command, args []string) { return } - if err := httpLib.Client.Post(endpoint, nil, nil); err != nil { + // POST /ip/{ip}/park answers with an ip.IpTask, which this used to discard. + // Keeping it is what lets the wait below notice a park that failed instead + // of polling the state for ten minutes. + var task map[string]any + if err := httpLib.Client.Post(endpoint, nil, &task); err != nil { display.OutputError(&flags.OutputFormatConfig, "failed to park %s: %s", ip, err) return } @@ -253,11 +259,11 @@ func ParkIp(_ *cobra.Command, args []string) { if !IPWait { display.OutputInfo(&flags.OutputFormatConfig, - map[string]any{"ip": ip}, "⚡️ %s is being parked.", ip) + map[string]any{"ip": ip, "task": task["taskId"]}, "⚡️ %s is being parked.", ip) return } - if err := waitForRouting(ip, ""); err != nil { + if err := waitForRouting(ip, "", taskIDOf(task)); err != nil { display.OutputError(&flags.OutputFormatConfig, "%s", err) return } @@ -283,7 +289,7 @@ func ListIpTasks(_ *cobra.Command, args []string) { // about the thing it is asked. // // An empty want means parked. -func waitForRouting(ip, want string) error { +func waitForRouting(ip, want string, taskID int64) error { for attempt := 0; attempt < movePollAttempts; attempt++ { // A read that failed is not a read that came back empty. Parking waits // for exactly the empty string, so counting a failure as one would end @@ -296,25 +302,109 @@ func waitForRouting(ip, want string) error { log.Printf("%s", err) } + // The task is asked second and only about failure, which is the one + // question the state cannot answer. A task that ends in cancelled, + // customerError or ovhError leaves the IP where it was, so the state + // reads "not there yet" forever: the wait ran its full ten minutes and + // then said "not routed yet, follow it with ovhcloud ip tasks", which + // reads as still in progress. Ten minutes and a misleading sentence for + // something the API knew in seconds. + // + // It stays a complement and never a substitute — a vRack attach measured + // earlier in this CLI created two tasks for one request, so a wait that + // concluded from the task alone could report success while the other half + // was still running. The state is read first every round, and read once + // more below before a failure is announced. + if failed, status, comment := taskFailed(ip, taskID); failed { + if current, err := routedTo(ip); err == nil && current == want { + return nil + } + return fmt.Errorf("the %s of %s failed: task %d ended %s%s\n See it with: ovhcloud ip tasks %s --id %d", + operationName(want), ip, taskID, status, taskComment(comment), ip, taskID) + } + time.Sleep(movePollInterval) } - where := "parked" - if want != "" { - where = "routed to " + want + return fmt.Errorf("stopped waiting after %s; %s is not %s yet, follow it with: ovhcloud ip tasks %s", + time.Duration(movePollAttempts)*movePollInterval, ip, destinationLabel(want), ip) +} + +// terminalTaskFailures are the three ip.TaskStatusEnum values that mean the work +// stopped and will not resume. +// +// Retyped here because the schema lists the seven statuses without saying which +// of them are failures — that classification does not exist in the document. A +// guard test holds these three against the enum, so a rename shows up as a red +// test instead of quietly turning this branch off. +var terminalTaskFailures = []string{"cancelled", "customerError", "ovhError"} + +// taskFailed asks whether the task handed to us has stopped for good. +// +// A task that cannot be read is not a task that failed: this is a complement to +// the state, and turning a transient 500 on the task route into "your move +// failed" would be worse than the ten-minute wait it replaces. +func taskFailed(ip string, taskID int64) (bool, string, string) { + if taskID == 0 { + return false, "", "" } - return fmt.Errorf("stopped waiting after %s; %s is not %s yet, follow it with: ovhcloud ip tasks %s", - time.Duration(movePollAttempts)*movePollInterval, ip, where, ip) + var task struct { + Status string `json:"status"` + Comment string `json:"comment"` + } + endpoint := fmt.Sprintf("/v1/ip/%s/task/%d", url.PathEscape(ip), taskID) + if err := httpLib.Client.Get(endpoint, &task); err != nil { + log.Printf("failed to read task %d of %s: %s", taskID, ip, err) + return false, "", "" + } + + return slices.Contains(terminalTaskFailures, task.Status), task.Status, task.Comment } -// routedTo answers the service an IP currently serves, and an empty string when -// it serves none. +func taskComment(comment string) string { + if comment == "" { + return "" + } + return " — " + comment +} + +// operationName and destinationLabel keep the two messages above readable in +// both directions: this loop serves a move and a park, and each has to name what +// it was doing. +func operationName(want string) string { + if want == "" { + return "parking" + } + return "move" +} + +func destinationLabel(want string) string { + if want == "" { + return "parked" + } + return "routed to " + want +} + +// taskIDOf reads the identifier out of the task an operation returns. // -// A failure to read is reported as "no service" on purpose: this feeds a -// confirmation prompt and a wait, and neither is improved by turning a -// transient read error into a failed command. The prompt says "not routed" -// rather than naming a service it could not confirm. +// go-ovh decodes with UseNumber, so a JSON integer arrives as json.Number and +// not as float64 — a type switch that only handles float64 is dead code, which +// is a mistake this repository has already made twice. +func taskIDOf(task map[string]any) int64 { + switch id := task["taskId"].(type) { + case json.Number: + if n, err := id.Int64(); err == nil { + return n + } + case float64: + return int64(id) + case int64: + return id + } + return 0 +} + // routedTo returns the service this IP currently serves, and an empty string // when it serves none. // diff --git a/internal/services/ip/move_test.go b/internal/services/ip/move_test.go index 219954d5d..8f212eccd 100644 --- a/internal/services/ip/move_test.go +++ b/internal/services/ip/move_test.go @@ -5,6 +5,9 @@ package ip import ( + "encoding/json" + "net/http" + "slices" "strings" "testing" "time" @@ -12,7 +15,9 @@ import ( "github.com/jarcoal/httpmock" "github.com/maxatome/go-testdeep/td" "github.com/ovh/go-ovh/ovh" + "github.com/ovh/ovhcloud-cli/internal/assets" httpLib "github.com/ovh/ovhcloud-cli/internal/http" + "github.com/ovh/ovhcloud-cli/internal/openapi" ) func sample() []destination { @@ -137,7 +142,7 @@ func withIpAPI(t *testing.T, attempts int, block string) { func TestWaitForRoutingReturnsWhenTheIpHasActuallyMoved(t *testing.T) { withIpAPI(t, 5, `{"ip": "1.2.3.4/32", "routedTo": {"serviceName": "ns0000006.ip-203-0-113.eu"}}`) - td.CmpNoError(t, waitForRouting("1.2.3.4/32", "ns0000006.ip-203-0-113.eu")) + td.CmpNoError(t, waitForRouting("1.2.3.4/32", "ns0000006.ip-203-0-113.eu", 0)) } // An empty destination is the parked state, and it has to be distinguishable @@ -145,7 +150,7 @@ func TestWaitForRoutingReturnsWhenTheIpHasActuallyMoved(t *testing.T) { func TestWaitForRoutingRecognisesParked(t *testing.T) { withIpAPI(t, 5, `{"ip": "1.2.3.4/32", "routedTo": {"serviceName": ""}}`) - td.CmpNoError(t, waitForRouting("1.2.3.4/32", "")) + td.CmpNoError(t, waitForRouting("1.2.3.4/32", "", 0)) } // Giving up is not the move failing, and the message must not claim it is: @@ -154,7 +159,7 @@ func TestWaitForRoutingRecognisesParked(t *testing.T) { func TestWaitForRoutingTimesOutWithoutClaimingFailure(t *testing.T) { withIpAPI(t, 2, `{"ip": "1.2.3.4/32", "routedTo": {"serviceName": "ns0000005.ip-203-0-113.eu"}}`) - err := waitForRouting("1.2.3.4/32", "ns0000006.ip-203-0-113.eu") + err := waitForRouting("1.2.3.4/32", "ns0000006.ip-203-0-113.eu", 0) td.Require(t).CmpError(err) td.Cmp(t, err.Error(), td.Contains("stopped waiting")) @@ -181,3 +186,103 @@ func TestAnUnreadRoutingIsNotAnEmptyOne(t *testing.T) { t.Fatal("the two cases produce the same sentence, so the prompt cannot tell them apart") } } + +// The half of eb56fab that had no test: parking waits for exactly the empty +// string, so a read that fails must not be counted as one. Folded together, the +// wait ended on the very answer it was looking for and called the park done — +// the worst shape of wrong, because the operator is told the traffic stopped. +func TestWaitForParkedDoesNotReadAFailureAsArrival(t *testing.T) { + withIpAPI(t, 2, `{}`) + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/v1/ip/1.2.3.4%2F32", + httpmock.NewStringResponder(500, `{"message": "gateway is having a day"}`)) + + err := waitForRouting("1.2.3.4/32", "", 0) + + td.Require(t).CmpError(err, "a park is never concluded from a failed read") + td.Cmp(t, err.Error(), td.Contains("stopped waiting")) +} + +// A task that ended in one of the three terminal failures leaves the IP where it +// was, so the state says "not there yet" for as long as anyone asks. The wait ran +// its full ten minutes and then said "not routed yet, follow it with ovhcloud ip +// tasks", which reads as still in progress. +// +// The task is asked second and only about failure: the state stays the authority +// on where the IP is, because a request here can create more than one task. +func TestWaitForRoutingStopsOnATerminalTaskFailure(t *testing.T) { + withIpAPI(t, 120, `{"ip": "1.2.3.4/32", "routedTo": {"serviceName": "ns0000005.ip-203-0-113.eu"}}`) + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/v1/ip/1.2.3.4%2F32/task/4242", + httpmock.NewStringResponder(200, + `{"taskId": 4242, "function": "genericMoveFloatingIp", "status": "customerError", "comment": "destination refused the route"}`)) + + err := waitForRouting("1.2.3.4/32", "ns0000006.ip-203-0-113.eu", 4242) + + td.Require(t).CmpError(err) + td.Cmp(t, err.Error(), td.Contains("customerError"), "the status is named") + td.Cmp(t, err.Error(), td.Contains("destination refused the route"), "and so is the reason") + td.Cmp(t, err.Error(), td.Not(td.Contains("stopped waiting")), + "it stopped because the task failed, not because it ran out of patience") +} + +// The state remains the authority, and this is the case that says so: the IP had +// not arrived when the round began, the task then reported a terminal failure, +// and the IP had arrived by the time it was read again. +// +// That ordering is the two-tasks-for-one-request shape measured on a vRack +// attach — the task we were handed can fail while the work completes elsewhere. +// So a failure is never announced on the strength of the task: the state is read +// once more first, and it wins. +func TestWaitForRoutingTrustsTheStateOverAFailedTask(t *testing.T) { + withIpAPI(t, 2, `{}`) + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/v1/ip/1.2.3.4%2F32", + httpmock.ResponderFromMultipleResponses([]*http.Response{ + httpmock.NewStringResponse(200, `{"ip": "1.2.3.4/32", "routedTo": {"serviceName": "ns0000005.ip-203-0-113.eu"}}`), + httpmock.NewStringResponse(200, `{"ip": "1.2.3.4/32", "routedTo": {"serviceName": "ns0000006.ip-203-0-113.eu"}}`), + })) + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/v1/ip/1.2.3.4%2F32/task/4242", + httpmock.NewStringResponder(200, `{"taskId": 4242, "status": "ovhError"}`)) + + td.CmpNoError(t, waitForRouting("1.2.3.4/32", "ns0000006.ip-203-0-113.eu", 4242), + "the IP arrived; the failed task does not overrule it") +} + +// A task that cannot be read is not a task that failed. Turning a 500 on the +// task route into "your move failed" would be worse than the wait it replaces. +func TestWaitForRoutingDoesNotFailOnAnUnreadableTask(t *testing.T) { + withIpAPI(t, 2, `{"ip": "1.2.3.4/32", "routedTo": {"serviceName": "ns0000005.ip-203-0-113.eu"}}`) + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/v1/ip/1.2.3.4%2F32/task/4242", + httpmock.NewStringResponder(500, `{"message": "nope"}`)) + + err := waitForRouting("1.2.3.4/32", "ns0000006.ip-203-0-113.eu", 4242) + + td.Require(t).CmpError(err) + td.Cmp(t, err.Error(), td.Contains("stopped waiting"), "it waited, it did not conclude") +} + +// The three statuses treated as terminal failures are retyped in Go, because the +// schema lists the seven statuses without saying which are failures — that +// classification is not in the document. This holds them against the enum, so a +// rename upstream shows up here instead of quietly turning the branch off. +func TestTerminalTaskFailuresAreAllDeclaredByTheSchema(t *testing.T) { + declared, err := openapi.GetComponentEnum(assets.IpOpenapiSchema, "ip.TaskStatusEnum") + td.Require(t).CmpNoError(err) + td.Cmp(t, len(declared) > 3, true, "positive control: the enum was read") + + for _, status := range terminalTaskFailures { + td.Cmp(t, slices.Contains(declared, status), true, + "%q is treated as a terminal failure but the schema does not declare it", status) + } +} + +// go-ovh decodes with UseNumber, so a JSON integer arrives as json.Number. A +// type switch that only handles float64 is dead code — a mistake this repository +// has already made twice, once in baremetal.go and once in the vRack wait. +func TestTaskIDOfReadsAJsonNumber(t *testing.T) { + var task map[string]any + decoder := json.NewDecoder(strings.NewReader(`{"taskId": 559188894}`)) + decoder.UseNumber() + td.Require(t).CmpNoError(decoder.Decode(&task)) + + td.Cmp(t, taskIDOf(task), int64(559188894)) + td.Cmp(t, taskIDOf(map[string]any{}), int64(0), "and an absent task is zero, not a panic") +} From 1a1bda3155664c0a6546aaeb3b39b38dc63d2b5d Mon Sep 17 00:00:00 2001 From: Denis Date: Mon, 24 Aug 2026 12:03:02 +0200 Subject: [PATCH 7/9] 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 8/9] 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 c683fcbe0ad29f981d9f42234fc5566efbb774bc Mon Sep 17 00:00:00 2001 From: Denis Date: Mon, 24 Aug 2026 12:14:18 +0200 Subject: [PATCH 9/9] 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 +