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 9a5ffd343e7387124cbf7f9f9275507200eb4355 Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Thu, 20 Aug 2026 01:58:40 +0200 Subject: [PATCH 3/9] feat(baremetal): list the servers carrying a tag, filtered by the API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IAM tags are how a fleet is organised — owner, Compliance, Team, LandingZone — and nothing here could ask for them. The generic --filter cannot: it runs over the columns of the table, after every server has been fetched, and the tags are not among them. ovhcloud baremetal list --tag Compliance=PCI-DSS ovhcloud baremetal list --tag owner:EXISTS ovhcloud baremetal list --tag owner:NEQ=Denis --tag Team:LIKE=infra% The v2 collection takes an iamTags parameter the v1 one does not, so the narrowing happens on the API rather than after the fact. The servers it names are then read on v1, which is the only route that answers with a machine: the v2 object is {id, iam} and a table built from it would lose every column. Measured on 20 August 2026, both catalogues list exactly the same 35 servers on this account, so nothing is lost in the crossing. Without --tag nothing changes and the v2 collection is never called. The syntax is one rule, key[:OPERATOR][=value], with no invented punctuation. The operator is spelled with the name the API uses, which means a refusal can list the ones that exist and completion can offer them; it is read from the embedded schema rather than transcribed, because a list copied into Go stops being true in silence — what the game protocols of #256 cost. EXISTS and NEXISTS ask whether a key is set at all, so they take no value, and a comparison with no value is refused rather than quietly turned into EXISTS: that would answer a different question. Only the first "=" separates, so a value may contain another one and the wildcards LIKE uses. --tag and --filter are not the same thing and both are honoured, in that order. Measured against the real account: Compliance=PCI-DSS returns 4 servers, owner:EXISTS returns 10, no --tag returns 35, and each matches a reading of the tag maps taken outside the CLI. NEQ on Compliance returns 0, which is right — all four servers carrying that key carry the same value. 11 unit tests, 5 cobra tests, 10 sabotages, 10 red. Signed-off-by: Denis Hamon --- doc/ovhcloud_baremetal_list.md | 9 + internal/assets/assets.go | 3 + internal/assets/assets_wasm.go | 1 + internal/cmd/baremetal.go | 14 +- internal/cmd/baremetal_tag_test.go | 109 ++++++++++++ internal/services/baremetal/baremetal.go | 47 ++++- internal/services/baremetal/tags.go | 215 +++++++++++++++++++++++ internal/services/baremetal/tags_test.go | 188 ++++++++++++++++++++ 8 files changed, 583 insertions(+), 3 deletions(-) create mode 100644 internal/cmd/baremetal_tag_test.go create mode 100644 internal/services/baremetal/tags.go create mode 100644 internal/services/baremetal/tags_test.go diff --git a/doc/ovhcloud_baremetal_list.md b/doc/ovhcloud_baremetal_list.md index 6aeb60a5c..e5b80cf79 100644 --- a/doc/ovhcloud_baremetal_list.md +++ b/doc/ovhcloud_baremetal_list.md @@ -2,6 +2,14 @@ List your Baremetal services +### Synopsis + +List your Baremetal services. + +--tag narrows the list on the API itself, using the IAM tags set on the servers. Write it as key=value, or key:OPERATOR=value to compare another way; OPERATOR is one of the names the API uses (EQ, NEQ, LIKE, ILIKE, EXISTS, NEXISTS), and EXISTS and NEXISTS take no value. Several --tag narrow further. + +It is not the same thing as --filter, which runs on the columns of the table once the servers have been read. + ``` ovhcloud baremetal list [flags] ``` @@ -17,6 +25,7 @@ ovhcloud baremetal list [flags] --filter 'startDate>="2023-12-01"' --filter 'name=~"something" && nbField>10' -h, --help help for list + --tag stringArray Only list servers carrying this IAM tag (key=value, or key:OPERATOR=value) ``` ### Options inherited from parent commands diff --git a/internal/assets/assets.go b/internal/assets/assets.go index 0f72bf9d4..e226b0eca 100644 --- a/internal/assets/assets.go +++ b/internal/assets/assets.go @@ -23,6 +23,9 @@ var ( //go:embed api-schemas/baremetal.json BaremetalOpenapiSchema []byte + //go:embed api-schemas/baremetal_v2.json + BaremetalV2OpenapiSchema []byte + //go:embed api-schemas/dedicatedceph.json DedicatedcephOpenapiSchema []byte diff --git a/internal/assets/assets_wasm.go b/internal/assets/assets_wasm.go index 69ded6a20..39343b853 100644 --- a/internal/assets/assets_wasm.go +++ b/internal/assets/assets_wasm.go @@ -11,6 +11,7 @@ var ( CloudV2OpenapiSchema []byte MeOpenapiSchema []byte BaremetalOpenapiSchema []byte + BaremetalV2OpenapiSchema []byte DedicatedcephOpenapiSchema []byte DedicatednashaOpenapiSchema []byte DomainOpenapiSchema []byte diff --git a/internal/cmd/baremetal.go b/internal/cmd/baremetal.go index 1c471f1db..64147bd0c 100644 --- a/internal/cmd/baremetal.go +++ b/internal/cmd/baremetal.go @@ -27,8 +27,18 @@ func init() { Use: "list", Aliases: []string{"ls"}, Short: "List your Baremetal services", - Run: baremetal.ListBaremetal, - } + Long: "List your Baremetal services.\n\n" + + "--tag narrows the list on the API itself, using the IAM tags set on the servers. " + + "Write it as key=value, or key:OPERATOR=value to compare another way; OPERATOR is one " + + "of the names the API uses (EQ, NEQ, LIKE, ILIKE, EXISTS, NEXISTS), and EXISTS and " + + "NEXISTS take no value. Several --tag narrow further.\n\n" + + "It is not the same thing as --filter, which runs on the columns of the table once the " + + "servers have been read.", + Run: baremetal.ListBaremetal, + } + baremetalListCmd.Flags().StringArrayVar(&baremetal.BaremetalTags, "tag", nil, + "Only list servers carrying this IAM tag (key=value, or key:OPERATOR=value)") + _ = baremetalListCmd.RegisterFlagCompletionFunc("tag", baremetal.CompleteBaremetalTag) baremetalCmd.AddCommand(withFilterFlag(baremetalListCmd)) // Command to get a single Baremetal diff --git a/internal/cmd/baremetal_tag_test.go b/internal/cmd/baremetal_tag_test.go new file mode 100644 index 000000000..93f1e8edb --- /dev/null +++ b/internal/cmd/baremetal_tag_test.go @@ -0,0 +1,109 @@ +// SPDX-FileCopyrightText: 2026 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +package cmd_test + +import ( + "encoding/json" + "net/http" + "net/url" + + "github.com/jarcoal/httpmock" + "github.com/maxatome/go-testdeep/td" + "github.com/ovh/ovhcloud-cli/internal/cmd" +) + +const ( + serversV1 = "https://eu.api.ovh.com/v1/dedicated/server" + serversV2 = "https://eu.api.ovh.com/v2/dedicated/server" +) + +// captureTagQuery answers the v2 collection and keeps the filter it was asked +// for, because the filter is the whole point of the flag. +func captureTagQuery(seen *map[string][]map[string]any) { + httpmock.RegisterResponder(http.MethodGet, serversV2, + func(req *http.Request) (*http.Response, error) { + raw := req.URL.Query().Get("iamTags") + if raw != "" { + decoded, err := url.QueryUnescape(raw) + if err != nil { + return nil, err + } + if err := json.Unmarshal([]byte(decoded), seen); err != nil { + return nil, err + } + } + return httpmock.NewStringResponse(200, `[{"id":"ns1.example"}]`), nil + }) + httpmock.RegisterResponder(http.MethodGet, serversV1+"/ns1.example", + httpmock.NewStringResponder(200, `{"name":"ns1.example","datacenter":"rbx8","region":"eu-west-rbx","os":"debian12","state":"ok","iam":{"displayName":"Paperclip"}}`)) +} + +// The narrowing is asked of the API, in the shape the API takes. +func (ms *MockSuite) TestBaremetalListByTagAsksTheApiToNarrow(assert, require *td.T) { + var seen map[string][]map[string]any + captureTagQuery(&seen) + + out, err := cmd.Execute("baremetal", "list", "--tag", "Compliance=PCI-DSS") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("ns1.example")) + assert.Cmp(seen["Compliance"][0]["operator"], "EQ") + assert.Cmp(seen["Compliance"][0]["value"], "PCI-DSS") + assert.Cmp(httpmock.GetCallCountInfo()["GET "+serversV1], 0, + "the v1 collection is not listed when the API is doing the narrowing") +} + +// The servers that come back are read on v1: the v2 object is {id, iam} and +// carries no machine, so a table built from it would lose every column. +func (ms *MockSuite) TestBaremetalListByTagReadsTheServersOnV1(assert, require *td.T) { + var seen map[string][]map[string]any + captureTagQuery(&seen) + + out, err := cmd.Execute("baremetal", "list", "--tag", "owner:EXISTS") + + require.CmpNoError(err) + assert.Cmp(seen["owner"][0]["operator"], "EXISTS") + assert.Cmp(seen["owner"][0], td.Not(td.ContainsKey("value")), "EXISTS carries no value") + assert.Cmp(out, td.Contains("debian12"), "the operating system only exists on the v1 object") + assert.Cmp(httpmock.GetCallCountInfo()["GET "+serversV1+"/ns1.example"], 1) +} + +// Without the flag nothing changes: the same v1 collection as before, and the +// v2 one is never touched. +func (ms *MockSuite) TestBaremetalListWithoutTagDoesNotTouchV2(assert, require *td.T) { + httpmock.RegisterResponder(http.MethodGet, serversV1, + httpmock.NewStringResponder(200, `["ns1.example"]`)) + httpmock.RegisterResponder(http.MethodGet, serversV1+"/ns1.example", + httpmock.NewStringResponder(200, `{"name":"ns1.example","datacenter":"rbx8","region":"eu-west-rbx","os":"debian12","state":"ok","iam":{"displayName":"Paperclip"}}`)) + + out, err := cmd.Execute("baremetal", "list") + + require.CmpNoError(err) + assert.Cmp(out, td.Contains("ns1.example")) + assert.Cmp(httpmock.GetCallCountInfo()["GET "+serversV2], 0) +} + +// --tag runs on the API before the servers are read, --filter runs on the table +// after. Both are honoured, and they are not the same thing. +func (ms *MockSuite) TestBaremetalListByTagStillHonoursFilter(assert, require *td.T) { + var seen map[string][]map[string]any + captureTagQuery(&seen) + + out, err := cmd.Execute("baremetal", "list", "--tag", "owner:EXISTS", "--filter", `datacenter=="gra1"`) + + require.CmpNoError(err) + assert.Cmp(seen["owner"][0]["operator"], "EXISTS", "the tag still went to the API") + assert.Cmp(out, td.Not(td.Contains("ns1.example")), "and the table filter still removed the row") +} + +// A filter the API would reject is refused here, with the operators that exist. +func (ms *MockSuite) TestBaremetalListByTagRefusesAnUnknownOperator(assert, require *td.T) { + _, err := cmd.Execute("baremetal", "list", "--tag", "owner:CONTAINS=Denis") + + require.CmpError(err) + assert.Cmp(err.Error(), td.Contains("NEXISTS")) + assert.Cmp(httpmock.GetCallCountInfo()["GET "+serversV2], 0, "nothing was asked of the API") + assert.Cmp(httpmock.GetCallCountInfo()["GET "+serversV1], 0) +} diff --git a/internal/services/baremetal/baremetal.go b/internal/services/baremetal/baremetal.go index 7a1a964ac..207e55045 100644 --- a/internal/services/baremetal/baremetal.go +++ b/internal/services/baremetal/baremetal.go @@ -77,8 +77,53 @@ var ( } ) +// ListBaremetal lists the servers of the account, optionally narrowed by tag. +// +// Without --tag it is the v1 collection, unchanged. With one, the narrowing is +// asked of the v2 collection, which takes an iamTags parameter the v1 one does +// not have, and the servers it names are then read on v1 — the only route that +// answers with a machine rather than {id, iam}. Both list exactly the same +// servers, measured, so nothing is lost in the crossing. func ListBaremetal(_ *cobra.Command, _ []string) { - common.ManageListRequest("/v1/dedicated/server", "", baremetalColumnsToDisplay, flags.GenericFilters) + tags, err := parseTagFilters(BaremetalTags) + if err != nil { + display.OutputError(&flags.OutputFormatConfig, "%s", err) + return + } + + if len(tags) == 0 { + common.ManageListRequest("/v1/dedicated/server", "", baremetalColumnsToDisplay, flags.GenericFilters) + return + } + + query, err := tagQuery(tags) + if err != nil { + display.OutputError(&flags.OutputFormatConfig, "%s", err) + return + } + + names, err := httpLib.FetchArray("/v2/dedicated/server"+query, "id") + if err != nil { + display.OutputError(&flags.OutputFormatConfig, "failed to list the servers matching these tags: %s", err) + return + } + + servers, err := httpLib.FetchObjectsParallel[map[string]any]("/v1/dedicated/server/%s", names, flags.IgnoreErrors) + if err != nil { + display.OutputError(&flags.OutputFormatConfig, "failed to read the servers: %s", err) + return + } + + // The generic --filter runs on the table, after the servers have been read, + // and --tag runs on the API before they are. Both are honoured, in that + // order, because they answer different questions. + servers, err = filtersLib.FilterLines(servers, flags.GenericFilters) + if err != nil { + display.OutputError(&flags.OutputFormatConfig, "failed to filter results: %s", err) + return + } + + display.RenderTable(servers, baremetalColumnsToDisplay, &flags.OutputFormatConfig) } func ListBaremetalTasks(_ *cobra.Command, args []string) { diff --git a/internal/services/baremetal/tags.go b/internal/services/baremetal/tags.go new file mode 100644 index 000000000..7f25305b4 --- /dev/null +++ b/internal/services/baremetal/tags.go @@ -0,0 +1,215 @@ +// SPDX-FileCopyrightText: 2026 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +package baremetal + +import ( + "encoding/json" + "fmt" + "net/url" + "sort" + "strings" + "sync" + + "github.com/ovh/ovhcloud-cli/internal/assets" + httpLib "github.com/ovh/ovhcloud-cli/internal/http" + "github.com/ovh/ovhcloud-cli/internal/openapi" + "github.com/spf13/cobra" +) + +// IAM tags are how a fleet is organised — owner, Compliance, Team, LandingZone +// — and until now nothing in this CLI could ask for them. The generic --filter +// cannot: it runs over the columns of the table, after every server has been +// fetched, and the tags are not among them. +// +// The v2 collection takes an iamTags query parameter that the v1 one does not. +// So this filters where the filtering belongs, on the server, and the servers +// that come back are then read on v1 — the only route that carries a machine +// rather than {id, iam}. Measured on 20 August 2026: both catalogues list +// exactly the same 35 servers on this account, so nothing is lost by asking +// one and reading the other. + +// BaremetalTags are the tag filters given on the command line. +var BaremetalTags []string + +// tagOperators are the comparisons the API accepts, read from the embedded +// schema rather than transcribed: a list copied into Go stops being true in +// silence, which is what the game protocols of #256 cost. +var tagOperators = sync.OnceValues(func() ([]string, error) { + return openapi.GetComponentEnum(assets.BaremetalV2OpenapiSchema, "iam.resource.TagFilter.OperatorEnum") +}) + +// tagFilter is one comparison on one tag key, in the shape the API takes. +type tagFilter struct { + Operator string `json:"operator"` + Value string `json:"value,omitempty"` +} + +// valuelessOperators ask whether a key is set at all, so a value would have +// nothing to compare against. Passing one anyway is a mistake worth naming: it +// reads as a filter that was applied. +var valuelessOperators = map[string]bool{"EXISTS": true, "NEXISTS": true} + +// parseTagFilters turns what was typed into the query the API takes. +// +// The shape is key[:OPERATOR][=value], one rule with no invented punctuation: +// +// --tag owner=Denis the common case, EQ, which is the API's own default +// --tag owner:EXISTS set to anything +// --tag owner:NEQ=Denis set to something else +// --tag Project:LIKE=Proof% the API's pattern syntax, passed through untouched +// +// The operator is spelled with the name the API uses, so a refusal can list the +// ones that exist and completion can offer them. +func parseTagFilters(given []string) (map[string][]tagFilter, error) { + if len(given) == 0 { + return nil, nil + } + + operators, err := tagOperators() + if err != nil { + return nil, fmt.Errorf("failed to read the tag operators from the embedded schema: %w", err) + } + + filters := make(map[string][]tagFilter, len(given)) + for _, raw := range given { + key, operator, value, err := splitTagFilter(raw, operators) + if err != nil { + return nil, err + } + + filters[key] = append(filters[key], tagFilter{Operator: operator, Value: value}) + } + + return filters, nil +} + +func splitTagFilter(raw string, operators []string) (key, operator, value string, err error) { + // The value is whatever follows the first "=", untouched: a tag value may + // contain anything, including another "=" and the LIKE wildcards. + head := raw + hasValue := false + if at := strings.Index(raw, "="); at >= 0 { + head, value, hasValue = raw[:at], raw[at+1:], true + } + + operator = "EQ" + if at := strings.Index(head, ":"); at >= 0 { + head, operator = head[:at], strings.ToUpper(head[at+1:]) + } + + key = strings.TrimSpace(head) + if key == "" { + return "", "", "", fmt.Errorf("%q names no tag; write it as key=value, or key:OPERATOR=value", raw) + } + + if !slicesContain(operators, operator) { + return "", "", "", fmt.Errorf("unknown tag operator %q in %q; use one of %s", + operator, raw, strings.Join(operators, ", ")) + } + + switch { + case valuelessOperators[operator] && hasValue: + return "", "", "", fmt.Errorf("%s asks whether %q is set at all, so it takes no value (%q)", operator, key, raw) + + case !valuelessOperators[operator] && !hasValue: + // An empty value is a legitimate filter; no value at all is not, and + // silently turning it into EXISTS would answer a question nobody asked. + return "", "", "", fmt.Errorf("%s needs something to compare %q against; write %s=, or use %s to ask whether it is set", + operator, key, key, strings.Join(valuelessNames(operators), " or ")) + } + + return key, operator, value, nil +} + +func valuelessNames(operators []string) []string { + var names []string + for _, operator := range operators { + if valuelessOperators[operator] { + names = append(names, operator) + } + } + sort.Strings(names) + + return names +} + +// tagQuery renders the filters as the query string the collection takes. +func tagQuery(filters map[string][]tagFilter) (string, error) { + if len(filters) == 0 { + return "", nil + } + + encoded, err := json.Marshal(filters) + if err != nil { + return "", fmt.Errorf("failed to render the tag filter: %w", err) + } + + return "?iamTags=" + url.QueryEscape(string(encoded)), nil +} + +// CompleteBaremetalTag offers the tag keys in use on the account, and the +// operators once a key has been typed. +// +// The keys come from the same collection the filter runs against, so what is +// offered is what exists rather than what somebody documented once. +func CompleteBaremetalTag(_ *cobra.Command, _ []string, toComplete string) ([]string, cobra.ShellCompDirective) { + if at := strings.Index(toComplete, ":"); at >= 0 { + operators, err := tagOperators() + if err != nil { + return nil, cobra.ShellCompDirectiveError + } + + prefix := toComplete[:at+1] + suggestions := make([]string, 0, len(operators)) + for _, operator := range operators { + suggestions = append(suggestions, prefix+operator) + } + + return suggestions, cobra.ShellCompDirectiveNoSpace | cobra.ShellCompDirectiveNoFileComp + } + + if strings.Contains(toComplete, "=") { + return nil, cobra.ShellCompDirectiveNoFileComp + } + + keys, err := tagKeysInUse() + if err != nil { + return nil, cobra.ShellCompDirectiveError + } + + return keys, cobra.ShellCompDirectiveNoSpace | cobra.ShellCompDirectiveNoFileComp +} + +// tagKeysInUse lists the tag keys actually set on the servers of the account. +// +// The v2 collection is the one that carries them: an object there is {id, iam} +// and iam.tags is the map. That is also why it cannot replace the v1 list — +// there is no machine in it, only its name and its identity. +func tagKeysInUse() ([]string, error) { + var servers []struct { + IAM struct { + Tags map[string]string `json:"tags"` + } `json:"iam"` + } + + if err := httpLib.Client.Get("/v2/dedicated/server", &servers); err != nil { + return nil, fmt.Errorf("failed to list the servers: %w", err) + } + + seen := make(map[string]bool) + for _, server := range servers { + for key := range server.IAM.Tags { + seen[key] = true + } + } + + keys := make([]string, 0, len(seen)) + for key := range seen { + keys = append(keys, key) + } + sort.Strings(keys) + + return keys, nil +} diff --git a/internal/services/baremetal/tags_test.go b/internal/services/baremetal/tags_test.go new file mode 100644 index 000000000..f091cf2ac --- /dev/null +++ b/internal/services/baremetal/tags_test.go @@ -0,0 +1,188 @@ +// SPDX-FileCopyrightText: 2026 OVH SAS +// +// SPDX-License-Identifier: Apache-2.0 + +package baremetal + +import ( + "encoding/json" + "net/url" + "strings" + "testing" +) + +// The common case is the one nobody should have to look up. +func TestATagWithoutAnOperatorMeansEquals(t *testing.T) { + filters, err := parseTagFilters([]string{"owner=Denis"}) + if err != nil { + t.Fatalf("unexpected refusal: %s", err) + } + + got := filters["owner"] + if len(got) != 1 || got[0].Operator != "EQ" || got[0].Value != "Denis" { + t.Fatalf("got %+v", got) + } +} + +// The operator is spelled with the name the API uses, so a refusal can list the +// ones that exist and completion can offer them. +func TestAnOperatorIsSpelledWhereItIsRead(t *testing.T) { + filters, err := parseTagFilters([]string{"owner:NEQ=Denis", "Project:like=Proof%", "Team:EXISTS"}) + if err != nil { + t.Fatalf("unexpected refusal: %s", err) + } + + if filters["owner"][0].Operator != "NEQ" { + t.Fatalf("got %+v", filters["owner"]) + } + if filters["Project"][0].Operator != "LIKE" || filters["Project"][0].Value != "Proof%" { + t.Fatalf("a lowercase operator must be accepted, and the pattern passed through: %+v", filters["Project"]) + } + if filters["Team"][0].Operator != "EXISTS" || filters["Team"][0].Value != "" { + t.Fatalf("got %+v", filters["Team"]) + } +} + +// An unknown operator is refused with the list, not sent to the API to be +// refused there as a malformed parameter. +func TestAnUnknownOperatorIsRefusedWithTheList(t *testing.T) { + _, err := parseTagFilters([]string{"owner:CONTAINS=Denis"}) + if err == nil { + t.Fatal("an operator the API does not have must be refused") + } + for _, expected := range []string{"EQ", "NEQ", "LIKE", "ILIKE", "EXISTS", "NEXISTS"} { + if !strings.Contains(err.Error(), expected) { + t.Fatalf("the refusal must name %s: %s", expected, err) + } + } +} + +// EXISTS asks whether a key is set at all. A value beside it has nothing to +// compare against, and accepting it would read as a filter that was applied. +func TestAValuelessOperatorRefusesAValue(t *testing.T) { + if _, err := parseTagFilters([]string{"owner:EXISTS=Denis"}); err == nil { + t.Fatal("EXISTS takes no value") + } + if _, err := parseTagFilters([]string{"owner:NEXISTS=Denis"}); err == nil { + t.Fatal("NEXISTS takes no value") + } +} + +// And the other way round: a comparison with nothing to compare against is not +// quietly turned into EXISTS, which would answer a different question. +func TestAComparisonWithoutAValueIsRefused(t *testing.T) { + _, err := parseTagFilters([]string{"owner"}) + if err == nil { + t.Fatal("EQ with no value must be refused") + } + if !strings.Contains(err.Error(), "EXISTS") { + t.Fatalf("the refusal must point at the operator that does answer it: %s", err) + } +} + +func TestATagFilterWithoutAKeyIsRefused(t *testing.T) { + for _, raw := range []string{"=Denis", "", ":EQ=Denis", " =x"} { + if _, err := parseTagFilters([]string{raw}); err == nil { + t.Fatalf("%q names no tag and must be refused", raw) + } + } +} + +// A tag value may contain anything, including another "=" and the wildcards +// LIKE uses. Only the first "=" separates. +func TestAValueIsTakenWhole(t *testing.T) { + filters, err := parseTagFilters([]string{"kernel=vmlinuz=6.1", "empty="}) + if err != nil { + t.Fatalf("unexpected refusal: %s", err) + } + if filters["kernel"][0].Value != "vmlinuz=6.1" { + t.Fatalf("got %q", filters["kernel"][0].Value) + } + if len(filters["empty"]) != 1 || filters["empty"][0].Value != "" { + t.Fatalf("an empty value is a filter; got %+v", filters["empty"]) + } +} + +// Several comparisons on one key are what the API's own shape allows: the +// parameter maps a key to a list of filters, not to one. +func TestSeveralComparisonsOnOneKeyAccumulate(t *testing.T) { + filters, err := parseTagFilters([]string{"owner:NEQ=Denis", "owner:NEQ=Yaniv"}) + if err != nil { + t.Fatalf("unexpected refusal: %s", err) + } + if len(filters["owner"]) != 2 { + t.Fatalf("got %+v", filters["owner"]) + } +} + +// The query is JSON in a query string, and both halves have to survive: a bare +// brace or an unescaped quote produces a 400 the operator cannot read. +func TestTheQueryIsEncodedJson(t *testing.T) { + if query, err := tagQuery(nil); err != nil || query != "" { + t.Fatalf("no filter means no query, got %q / %v", query, err) + } + + filters, err := parseTagFilters([]string{"Compliance=PCI-DSS"}) + if err != nil { + t.Fatal(err) + } + + query, err := tagQuery(filters) + if err != nil { + t.Fatal(err) + } + if !strings.HasPrefix(query, "?iamTags=") { + t.Fatalf("got %q", query) + } + + // The braces and quotes of the JSON have no business travelling raw in a + // query string. QueryUnescape happily returns an unescaped string + // unchanged, so a round trip alone would pass on a query that was never + // escaped at all. + for _, raw := range []string{"{", "}", "\"", "[", "]"} { + if strings.Contains(query, raw) { + t.Fatalf("%q must not travel raw in the query: %s", raw, query) + } + } + + decoded, err := url.QueryUnescape(strings.TrimPrefix(query, "?iamTags=")) + if err != nil { + t.Fatalf("the query must be escaped: %s", err) + } + + var back map[string][]tagFilter + if err := json.Unmarshal([]byte(decoded), &back); err != nil { + t.Fatalf("the API decodes this as JSON: %s", err) + } + if back["Compliance"][0].Value != "PCI-DSS" { + t.Fatalf("got %+v", back) + } +} + +// A tag value may contain a space, and QueryEscape renders one as "+". The +// round trip has to survive it, because a value that comes back as "a+b" +// instead of "a b" is a filter that silently matches nothing. +func TestASpaceInAValueSurvivesTheQuery(t *testing.T) { + filters, err := parseTagFilters([]string{"Project:LIKE=Proof of Concept%"}) + if err != nil { + t.Fatal(err) + } + + query, err := tagQuery(filters) + if err != nil { + t.Fatal(err) + } + + decoded, err := url.QueryUnescape(strings.TrimPrefix(query, "?iamTags=")) + if err != nil { + t.Fatal(err) + } + + var back map[string][]tagFilter + if err := json.Unmarshal([]byte(decoded), &back); err != nil { + t.Fatalf("the API decodes this as JSON: %s", err) + } + if back["Project"][0].Value != "Proof of Concept%" { + t.Fatalf("got %q", back["Project"][0].Value) + } +} From 76ae82aa008dd22b6f1884a319ccf76f542e6daf Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Thu, 20 Aug 2026 16:14:56 +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/services/baremetal/logs_test.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/internal/services/baremetal/logs_test.go b/internal/services/baremetal/logs_test.go index e8f4f3da9..22c29be12 100644 --- a/internal/services/baremetal/logs_test.go +++ b/internal/services/baremetal/logs_test.go @@ -15,7 +15,7 @@ import ( // mistaken for a title triggers a sweep that finds nothing, and a title // mistaken for a UUID is sent to the API as a stream identifier. func TestAStreamIdentifierIsRecognisedByItsShape(t *testing.T) { - if !looksLikeUUID("4459af12-6451-45de-808b-2b959c11a17e") { + if !looksLikeUUID("00000000-6451-45de-808b-2b959c11a17e") { t.Fatal("a real stream identifier must be taken as one") } if !looksLikeUUID("4459AF12-6451-45DE-808B-2B959C11A17E") { @@ -26,9 +26,9 @@ func TestAStreamIdentifierIsRecognisedByItsShape(t *testing.T) { "TO REMOVE 1", "datastream_test", "", - "4459af12-6451-45de-808b-2b959c11a17", // one short - "4459af12-6451-45de-808b-2b959c11a17ee", // one long - "4459af12x6451-45de-808b-2b959c11a17e", // separator moved + "00000000-6451-45de-808b-2b959c11a17", // one short + "00000000-6451-45de-808b-2b959c11a17ee", // one long + "00000000x6451-45de-808b-2b959c11a17e", // separator moved "zzzzzzzz-6451-45de-808b-2b959c11a17e", // not hexadecimal "Prestashop nginx logs (filebeat) padded to 36", // right length, wrong everything } { @@ -42,11 +42,11 @@ func TestAStreamIdentifierIsRecognisedByItsShape(t *testing.T) { // caller already holds is work done to learn nothing, and it is what `-o json` // hands back. func TestAnIdentifierIsUsedWithoutASweep(t *testing.T) { - stream, err := resolveStream("4459af12-6451-45de-808b-2b959c11a17e") + stream, err := resolveStream("00000000-6451-45de-808b-2b959c11a17e") if err != nil { t.Fatalf("an identifier must resolve to itself: %s", err) } - if stream.StreamID != "4459af12-6451-45de-808b-2b959c11a17e" { + if stream.StreamID != "00000000-6451-45de-808b-2b959c11a17e" { t.Fatalf("got %q", stream.StreamID) } if stream.Title != "" || stream.ServiceName != "" { @@ -84,13 +84,13 @@ func TestNearbyTitlesNamesAFewAndCountsTheRest(t *testing.T) { // title, the identifier the API acted on is shown beside it; when they pasted // an identifier, repeating it says nothing. func TestAStreamIsNamedTheWayItWasAskedFor(t *testing.T) { - titled := streamLabel(ldpStream{Title: "TO REMOVE 1", StreamID: "69813166-7b66-4a09-8fcd-c47781d23966", ServiceName: "ldp-nx-19421"}) - if !strings.Contains(titled, "TO REMOVE 1") || !strings.Contains(titled, "ldp-nx-19421") { + titled := streamLabel(ldpStream{Title: "TO REMOVE 1", StreamID: "00000000-0000-4000-8000-000000000001", ServiceName: "ldp-xx-00000"}) + if !strings.Contains(titled, "TO REMOVE 1") || !strings.Contains(titled, "ldp-xx-00000") { t.Fatalf("got %q", titled) } - bare := streamLabel(ldpStream{StreamID: "69813166-7b66-4a09-8fcd-c47781d23966"}) - if bare != "69813166-7b66-4a09-8fcd-c47781d23966" { + bare := streamLabel(ldpStream{StreamID: "00000000-0000-4000-8000-000000000001"}) + if bare != "00000000-0000-4000-8000-000000000001" { t.Fatalf("got %q", bare) } } @@ -125,7 +125,7 @@ func TestAWaitWithNothingToFollowSaysSo(t *testing.T) { if _, err := waitForLogOperation("", "op-1"); err == nil { t.Fatal("a wait without a Log Data Platform service must refuse") } - if _, err := waitForLogOperation("ldp-nx-19421", ""); err == nil { + if _, err := waitForLogOperation("ldp-xx-00000", ""); err == nil { t.Fatal("a wait without an operation must refuse") } } From 53f316cc6988e357db9ceecb91b0414799cc5d6a Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Thu, 20 Aug 2026 16:15:00 +0200 Subject: [PATCH 5/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/baremetal_tag_test.go | 4 ++-- internal/services/baremetal/logs_test.go | 2 +- internal/services/baremetal/tags_test.go | 2 +- internal/services/ip/move_test.go | 2 +- internal/services/vrack/attach_test.go | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/cmd/baremetal_tag_test.go b/internal/cmd/baremetal_tag_test.go index 93f1e8edb..c166763df 100644 --- a/internal/cmd/baremetal_tag_test.go +++ b/internal/cmd/baremetal_tag_test.go @@ -37,7 +37,7 @@ func captureTagQuery(seen *map[string][]map[string]any) { return httpmock.NewStringResponse(200, `[{"id":"ns1.example"}]`), nil }) httpmock.RegisterResponder(http.MethodGet, serversV1+"/ns1.example", - httpmock.NewStringResponder(200, `{"name":"ns1.example","datacenter":"rbx8","region":"eu-west-rbx","os":"debian12","state":"ok","iam":{"displayName":"Paperclip"}}`)) + httpmock.NewStringResponder(200, `{"name":"ns1.example","datacenter":"rbx8","region":"eu-west-rbx","os":"debian12","state":"ok","iam":{"displayName":"Build runner"}}`)) } // The narrowing is asked of the API, in the shape the API takes. @@ -76,7 +76,7 @@ func (ms *MockSuite) TestBaremetalListWithoutTagDoesNotTouchV2(assert, require * httpmock.RegisterResponder(http.MethodGet, serversV1, httpmock.NewStringResponder(200, `["ns1.example"]`)) httpmock.RegisterResponder(http.MethodGet, serversV1+"/ns1.example", - httpmock.NewStringResponder(200, `{"name":"ns1.example","datacenter":"rbx8","region":"eu-west-rbx","os":"debian12","state":"ok","iam":{"displayName":"Paperclip"}}`)) + httpmock.NewStringResponder(200, `{"name":"ns1.example","datacenter":"rbx8","region":"eu-west-rbx","os":"debian12","state":"ok","iam":{"displayName":"Build runner"}}`)) out, err := cmd.Execute("baremetal", "list") diff --git a/internal/services/baremetal/logs_test.go b/internal/services/baremetal/logs_test.go index 22c29be12..1991b25a3 100644 --- a/internal/services/baremetal/logs_test.go +++ b/internal/services/baremetal/logs_test.go @@ -18,7 +18,7 @@ func TestAStreamIdentifierIsRecognisedByItsShape(t *testing.T) { if !looksLikeUUID("00000000-6451-45de-808b-2b959c11a17e") { t.Fatal("a real stream identifier must be taken as one") } - if !looksLikeUUID("4459AF12-6451-45DE-808B-2B959C11A17E") { + if !looksLikeUUID("00000000-6451-45DE-808B-2B959C11A17E") { t.Fatal("the API answers in lowercase but accepts either") } diff --git a/internal/services/baremetal/tags_test.go b/internal/services/baremetal/tags_test.go index f091cf2ac..39f0e75a2 100644 --- a/internal/services/baremetal/tags_test.go +++ b/internal/services/baremetal/tags_test.go @@ -106,7 +106,7 @@ func TestAValueIsTakenWhole(t *testing.T) { // Several comparisons on one key are what the API's own shape allows: the // parameter maps a key to a list of filters, not to one. func TestSeveralComparisonsOnOneKeyAccumulate(t *testing.T) { - filters, err := parseTagFilters([]string{"owner:NEQ=Denis", "owner:NEQ=Yaniv"}) + filters, err := parseTagFilters([]string{"owner:NEQ=Denis", "owner:NEQ=bob"}) if err != nil { t.Fatalf("unexpected refusal: %s", err) } diff --git a/internal/services/ip/move_test.go b/internal/services/ip/move_test.go index 54fc84d24..eee01daa8 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") 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 067a1b5b6b6128a22fd90e3c17edceb4abf61579 Mon Sep 17 00:00:00 2001 From: Denis Hamon Date: Fri, 21 Aug 2026 13:48:25 +0200 Subject: [PATCH 6/9] fix(list --tag): a tag key containing a colon was unreachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The operator was read from the FIRST colon of what was typed, so any key holding one lost everything after it: `--tag ovh:default=true` parsed as key "ovh" with operator "DEFAULT" and came back refused for an unknown operator — one nobody had typed. The schema documents "ovh:" as the prefix of every tag OVHcloud computes itself, so that was the whole computed namespace, and the error pointed at the wrong thing. It now reads the operator from the last colon, and only when what follows is an operator the API declares. That leaves a genuinely ambiguous case, and the CLI does not choose: after the last colon, "owner:CONTAINS=Denis" is either a typo for an operator or a key called "owner:CONTAINS". Reading it as a key would send it to the API, come back with no servers, and print "nothing matches" — a wrong answer wearing the shape of an answer. So it is refused, with both readings named and the operators listed. A key that really does hold a colon is written with its operator spelled out, which is unambiguous. The completer had the same defect and a worse version of it: on a key named "ovh:default" it offered "ovh:EQ", the operator glued to the first segment, which no longer names the key at all — the CLI telling the operator to type something it would then reject. It now offers only forms the parser accepts, and the test feeds every suggestion back through the parser rather than asserting on a hardcoded shape. That test is what caught the first version of this fix, whose completer offered the bare "ovh:default" while the parser refused it. Four tests, three sabotages, three reds. Signed-off-by: Denis Hamon Co-Authored-By: Claude Opus 5 (1M context) --- internal/services/baremetal/tags.go | 87 ++++++++++++---- internal/services/baremetal/tags_test.go | 123 +++++++++++++++++++++++ 2 files changed, 192 insertions(+), 18 deletions(-) diff --git a/internal/services/baremetal/tags.go b/internal/services/baremetal/tags.go index 7f25305b4..f7f9620a6 100644 --- a/internal/services/baremetal/tags.go +++ b/internal/services/baremetal/tags.go @@ -59,6 +59,7 @@ var valuelessOperators = map[string]bool{"EXISTS": true, "NEXISTS": true} // --tag owner:EXISTS set to anything // --tag owner:NEQ=Denis set to something else // --tag Project:LIKE=Proof% the API's pattern syntax, passed through untouched +// --tag ovh:default:EQ=x a key that contains a colon, operator written out // // The operator is spelled with the name the API uses, so a refusal can list the // ones that exist and completion can offer them. @@ -94,9 +95,39 @@ func splitTagFilter(raw string, operators []string) (key, operator, value string head, value, hasValue = raw[:at], raw[at+1:], true } + // The operator is read from the LAST colon, and only when what follows it is + // an operator the API knows. + // + // Read from the first colon, any key containing one lost everything after it: + // "ovh:default=x" parsed as key "ovh" with operator "DEFAULT" and was refused + // as an unknown operator. That is not an exotic key — the schema documents + // "ovh:" as the prefix of every tag OVHcloud computes itself, so the whole + // namespace was unreachable, and the error blamed an operator the operator + // never typed. + // The operator is read from the LAST colon, and only when what follows it is + // an operator the API knows. + // + // Read from the first colon, any key containing one lost everything after it: + // "ovh:default=x" parsed as key "ovh" with operator "DEFAULT" and was refused + // as an unknown operator. That is not an exotic key — the schema documents + // "ovh:" as the prefix of every tag OVHcloud computes itself — so the whole + // namespace was unreachable, and the error blamed an operator nobody typed. + // + // What follows the last colon and is not an operator is genuinely ambiguous: + // "owner:CONTAINS=Denis" is either a typo for an operator or a tag key called + // "owner:CONTAINS". Guessing the key would send it to the API, come back with + // no servers, and read as "nothing matches" — a wrong answer wearing the shape + // of an answer. So it is refused, with both readings named. operator = "EQ" - if at := strings.Index(head, ":"); at >= 0 { - head, operator = head[:at], strings.ToUpper(head[at+1:]) + if at := strings.LastIndex(head, ":"); at >= 0 { + candidate := strings.ToUpper(head[at+1:]) + if !slicesContain(operators, candidate) { + return "", "", "", fmt.Errorf( + "%q is ambiguous: %q is not one of %s, so this can only be read as the tag key %q with no operator.\n"+ + " If that is what you meant, write the operator out: --tag %s:EQ=", + raw, head[at+1:], strings.Join(operators, ", "), head, head) + } + head, operator = head[:at], candidate } key = strings.TrimSpace(head) @@ -155,21 +186,6 @@ func tagQuery(filters map[string][]tagFilter) (string, error) { // The keys come from the same collection the filter runs against, so what is // offered is what exists rather than what somebody documented once. func CompleteBaremetalTag(_ *cobra.Command, _ []string, toComplete string) ([]string, cobra.ShellCompDirective) { - if at := strings.Index(toComplete, ":"); at >= 0 { - operators, err := tagOperators() - if err != nil { - return nil, cobra.ShellCompDirectiveError - } - - prefix := toComplete[:at+1] - suggestions := make([]string, 0, len(operators)) - for _, operator := range operators { - suggestions = append(suggestions, prefix+operator) - } - - return suggestions, cobra.ShellCompDirectiveNoSpace | cobra.ShellCompDirectiveNoFileComp - } - if strings.Contains(toComplete, "=") { return nil, cobra.ShellCompDirectiveNoFileComp } @@ -179,7 +195,42 @@ func CompleteBaremetalTag(_ *cobra.Command, _ []string, toComplete string) ([]st return nil, cobra.ShellCompDirectiveError } - return keys, cobra.ShellCompDirectiveNoSpace | cobra.ShellCompDirectiveNoFileComp + operators, err := tagOperators() + if err != nil { + return nil, cobra.ShellCompDirectiveError + } + + // Every suggestion is a form the parser accepts, which is not a detail: this + // used to offer "ovh:EQ" for a key named "ovh:default" — the operator glued to + // the first segment of the key — and the parser then refused it. A completion + // that cannot be accepted is worse than none. + // + // So a key holding a colon is only ever offered with its operator written out, + // because that is the only form in which such a key is unambiguous. A key + // without one is offered bare, where the implied EQ is unambiguous. + suggestions := make([]string, 0, len(keys)) + for _, key := range keys { + if !strings.HasPrefix(key, toComplete) && !strings.HasPrefix(toComplete, key+":") { + continue + } + if strings.Contains(key, ":") { + for _, operator := range operators { + suggestions = append(suggestions, key+":"+operator) + } + continue + } + suggestions = append(suggestions, key) + } + + // A key already typed in full, followed by a colon: the operator is what is + // being asked for. + if at := strings.LastIndex(toComplete, ":"); at >= 0 && slicesContain(keys, toComplete[:at]) { + for _, operator := range operators { + suggestions = append(suggestions, toComplete[:at]+":"+operator) + } + } + + return suggestions, cobra.ShellCompDirectiveNoSpace | cobra.ShellCompDirectiveNoFileComp } // tagKeysInUse lists the tag keys actually set on the servers of the account. diff --git a/internal/services/baremetal/tags_test.go b/internal/services/baremetal/tags_test.go index 39f0e75a2..53ee42775 100644 --- a/internal/services/baremetal/tags_test.go +++ b/internal/services/baremetal/tags_test.go @@ -9,6 +9,10 @@ import ( "net/url" "strings" "testing" + + "github.com/jarcoal/httpmock" + "github.com/ovh/go-ovh/ovh" + httpLib "github.com/ovh/ovhcloud-cli/internal/http" ) // The common case is the one nobody should have to look up. @@ -186,3 +190,122 @@ func TestASpaceInAValueSurvivesTheQuery(t *testing.T) { t.Fatalf("got %q", back["Project"][0].Value) } } + +// The schema documents "ovh:" as the prefix of every tag OVHcloud computes +// itself — ovh:default on a billing account, ovh:whoisOwner on a domain. Reading +// the operator from the FIRST colon made that whole namespace unreachable: +// "ovh:default=x" parsed as key "ovh" with operator "DEFAULT" and came back +// refused for an operator nobody typed. +func TestATagKeyMayContainAColon(t *testing.T) { + filters, err := parseTagFilters([]string{"ovh:default:EQ=true"}) + if err != nil { + t.Fatalf("unexpected refusal: %s", err) + } + if len(filters["ovh:default"]) != 1 { + t.Fatalf("the key is everything before the operator: %+v", filters) + } + if filters["ovh:default"][0].Operator != "EQ" || filters["ovh:default"][0].Value != "true" { + t.Fatalf("got %+v", filters["ovh:default"][0]) + } + + // And with a valueless operator, which has no "=" to anchor on. + filters, err = parseTagFilters([]string{"ovh:whoisOwner:EXISTS"}) + if err != nil { + t.Fatalf("unexpected refusal: %s", err) + } + if len(filters["ovh:whoisOwner"]) != 1 || filters["ovh:whoisOwner"][0].Operator != "EXISTS" { + t.Fatalf("got %+v", filters) + } +} + +// What follows the last colon and is not an operator is genuinely ambiguous, and +// the CLI must not choose. Reading it as a key would send "owner:CONTAINS" to the +// API, come back with no servers, and print "nothing matches" — a wrong answer +// wearing the shape of an answer. +func TestAnAmbiguousColonIsRefusedWithBothReadings(t *testing.T) { + _, err := parseTagFilters([]string{"owner:CONTAINS=Denis"}) + if err == nil { + t.Fatal("the CLI must not silently pick one of the two readings") + } + if !strings.Contains(err.Error(), "ambiguous") { + t.Fatalf("the refusal has to say so: %s", err) + } + if !strings.Contains(err.Error(), "owner:CONTAINS") { + t.Fatalf("the key reading has to be named: %s", err) + } + if !strings.Contains(err.Error(), "EQ") { + t.Fatalf("and the operators listed: %s", err) + } +} + +// The completer must only ever offer something the parser accepts. On a key +// holding a colon it offered "ovh:EQ" — the operator EQ appended to the first +// segment — which no longer names the key at all. A completion that cannot be +// accepted is worse than none: it is the CLI telling the operator to type +// something it will then reject. +// +// Every suggestion is fed straight back through the parser here, which is the +// only assertion that cannot drift from the parser's actual rules. +func TestTheCompleterOnlyOffersWhatTheParserAccepts(t *testing.T) { + withTagAPI(t, `[{"id": "srv-1", "iam": {"tags": {"ovh:default": "true", "owner": "Denis"}}}]`) + + operators, err := tagOperators() + if err != nil { + t.Fatalf("could not read the operators: %s", err) + } + + for _, toComplete := range []string{"", "ovh:", "ovh:default:", "owner:"} { + suggestions, _ := CompleteBaremetalTag(nil, nil, toComplete) + if len(suggestions) == 0 { + t.Fatalf("nothing offered for %q, so this test would prove nothing", toComplete) + } + for _, suggestion := range suggestions { + candidate := suggestion + if !strings.HasSuffix(candidate, "EXISTS") { + candidate += "=x" + } + if _, _, _, err := splitTagFilter(candidate, operators); err != nil { + t.Fatalf("completing %q offered %q, which the parser refuses: %s", + toComplete, suggestion, err) + } + } + } +} + +// And it has to reach the key, not stop at its first segment: typing "ovh:" and +// pressing tab used to offer six operators and never the key "ovh:default". +func TestTheCompleterReachesAKeyWithAColon(t *testing.T) { + withTagAPI(t, `[{"id": "srv-1", "iam": {"tags": {"ovh:default": "true"}}}]`) + + suggestions, _ := CompleteBaremetalTag(nil, nil, "ovh:") + + var found bool + for _, suggestion := range suggestions { + if strings.HasPrefix(suggestion, "ovh:default:") { + found = true + } + } + if !found { + t.Fatalf("the key has to be reached, got %v", suggestions) + } +} + +// withTagAPI points the shared client at httpmock for the v2 collection the +// completer reads its keys from. +func withTagAPI(t *testing.T, servers string) { + t.Helper() + httpmock.Activate(t) + + origClient := httpLib.Client + client, err := ovh.NewClient("ovh-eu", "app_key", "app_secret", "consumer_key") + if err != nil { + t.Fatalf("could not build a client: %s", err) + } + httpLib.Client = client + t.Cleanup(func() { httpLib.Client = origClient }) + + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/1.0/auth/time", + httpmock.NewStringResponder(200, "0")) + httpmock.RegisterResponder("GET", "https://eu.api.ovh.com/v2/dedicated/server", + httpmock.NewStringResponder(200, servers)) +} 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 26369ae4cad34e2bbf985c311706dc3c2fcf61a3 Mon Sep 17 00:00:00 2001 From: Denis Date: Mon, 24 Aug 2026 12:10:20 +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 +