Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the ovhcloud cloud instance CLI feature set by adding commands and output templates for instance groups, automated backup workflows (“autobackup”), and application access credential retrieval.
Changes:
- Add
cloud instance groupsubcommands (list/get/create/delete) backed by new rendering template(s). - Add
cloud instance autobackupsubcommands (list/get/create/delete) backed by a new rendering template. - Add
cloud instance application-accesssubcommand backed by a new rendering template, plus docs and tests updates.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/services/cloud/templates/cloud_instance_group.tmpl | Adds human-readable output template for instance group details. |
| internal/services/cloud/templates/cloud_instance_autobackup.tmpl | Adds human-readable output template for autobackup workflow details. |
| internal/services/cloud/templates/cloud_instance_application_access.tmpl | Adds human-readable output template for application access credentials. |
| internal/services/cloud/cloud_instance.go | Implements API calls/handlers for application access, autobackup workflows, and instance groups. |
| internal/cmd/cloud_instance.go | Wires new Cobra subcommands/flags into the CLI under cloud instance. |
| internal/cmd/cloud_instance_test.go | Adds command-level tests for new subcommands (currently partial coverage). |
| doc/ovhcloud_cloud_instance.md | Registers new subcommand docs in the instance command index. |
| doc/ovhcloud_cloud_instance_group.md | Adds generated documentation for cloud instance group. |
| doc/ovhcloud_cloud_instance_group_list.md | Adds generated documentation for cloud instance group list. |
| doc/ovhcloud_cloud_instance_group_get.md | Adds generated documentation for cloud instance group get. |
| doc/ovhcloud_cloud_instance_group_delete.md | Adds generated documentation for cloud instance group delete. |
| doc/ovhcloud_cloud_instance_group_create.md | Adds generated documentation for cloud instance group create. |
| doc/ovhcloud_cloud_instance_autobackup.md | Adds generated documentation for cloud instance autobackup. |
| doc/ovhcloud_cloud_instance_autobackup_list.md | Adds generated documentation for cloud instance autobackup list. |
| doc/ovhcloud_cloud_instance_autobackup_get.md | Adds generated documentation for cloud instance autobackup get. |
| doc/ovhcloud_cloud_instance_autobackup_delete.md | Adds generated documentation for cloud instance autobackup delete. |
| doc/ovhcloud_cloud_instance_autobackup_create.md | Adds generated documentation for cloud instance autobackup create. |
| doc/ovhcloud_cloud_instance_application-access.md | Adds generated documentation for cloud instance application-access. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| createAutobackupCmd.Flags().StringVar(&cloud.AutobackupCreateParams.Cron, "cron", "", "Unix Cron pattern (e.g. '0 0 * * *')") | ||
| createAutobackupCmd.Flags().IntVar(&cloud.AutobackupCreateParams.Rotation, "rotation", 0, "Number of backups to keep") | ||
| createAutobackupCmd.Flags().StringVar(&cloud.AutobackupCreateParams.Name, "name", "", "Workflow name") |
There was a problem hiding this comment.
The autobackup creation request requires --cron, --rotation, and --name (per the API schema), but the command currently allows running without setting them (defaults: empty string / 0). This will consistently lead to API-side validation errors; please mark these flags as required (and/or validate before sending the request) so users get fast feedback and a clear usage message.
| createAutobackupCmd.Flags().StringVar(&cloud.AutobackupCreateParams.Name, "name", "", "Workflow name") | |
| createAutobackupCmd.Flags().StringVar(&cloud.AutobackupCreateParams.Name, "name", "", "Workflow name") | |
| _ = createAutobackupCmd.MarkFlagRequired("cron") | |
| _ = createAutobackupCmd.MarkFlagRequired("rotation") | |
| _ = createAutobackupCmd.MarkFlagRequired("name") |
| AutobackupCreateParams.InstanceID = args[0] | ||
|
|
||
| endpoint := fmt.Sprintf("/v1/cloud/project/%s/region/%s/workflow/backup", projectID, url.PathEscape(region)) | ||
|
|
||
| var response map[string]any | ||
| if err := httpLib.Client.Post(endpoint, AutobackupCreateParams, &response); err != nil { |
There was a problem hiding this comment.
CreateAutobackup mutates the package-level AutobackupCreateParams (AutobackupCreateParams.InstanceID = args[0]). Since this struct is also bound to Cobra flags, mutating it inside the handler can leak state across command invocations/tests and is a potential data race if commands are executed concurrently. Prefer building a local request body (copy the struct, set InstanceID on the copy) before calling the API.
| AutobackupCreateParams.InstanceID = args[0] | |
| endpoint := fmt.Sprintf("/v1/cloud/project/%s/region/%s/workflow/backup", projectID, url.PathEscape(region)) | |
| var response map[string]any | |
| if err := httpLib.Client.Post(endpoint, AutobackupCreateParams, &response); err != nil { | |
| params := AutobackupCreateParams | |
| params.InstanceID = args[0] | |
| endpoint := fmt.Sprintf("/v1/cloud/project/%s/region/%s/workflow/backup", projectID, url.PathEscape(region)) | |
| var response map[string]any | |
| if err := httpLib.Client.Post(endpoint, params, &response); err != nil { |
| groupCmd.AddCommand(&cobra.Command{ | ||
| Use: "get <group_id>", | ||
| Short: "Get a specific instance group", | ||
| Run: cloud.GetInstanceGroup, | ||
| Args: cobra.ExactArgs(1), | ||
| }) | ||
|
|
||
| createGroupCmd := &cobra.Command{ | ||
| Use: "create <name> <region>", | ||
| Short: "Create an instance group", | ||
| Run: cloud.CreateInstanceGroup, | ||
| Args: cobra.ExactArgs(2), | ||
| } | ||
| createGroupCmd.Flags().StringVarP(&cloud.InstanceGroupType, "type", "t", "affinity", "Group type: affinity or anti-affinity (default is affinity)") | ||
| groupCmd.AddCommand(createGroupCmd) | ||
|
|
||
| groupCmd.AddCommand(&cobra.Command{ | ||
| Use: "delete <group_id>", | ||
| Short: "Delete a specific instance group", | ||
| Run: cloud.DeleteInstanceGroup, | ||
| Args: cobra.ExactArgs(1), | ||
| }) | ||
|
|
||
| // Autobackup subcommands | ||
| autobackupCmd := &cobra.Command{ | ||
| Use: "autobackup", | ||
| Short: "Manage automatic backup workflows for instances", | ||
| } | ||
| instanceCmd.AddCommand(autobackupCmd) | ||
|
|
||
| autobackupCmd.AddCommand(withFilterFlag(&cobra.Command{ | ||
| Use: "list <instance_id>", | ||
| Aliases: []string{"ls"}, | ||
| Short: "List automatic backup workflows for the given instance", | ||
| Run: cloud.ListAutobackups, | ||
| Args: cobra.ExactArgs(1), | ||
| })) | ||
|
|
||
| autobackupCmd.AddCommand(&cobra.Command{ | ||
| Use: "get <instance_id> <backup_workflow_id>", | ||
| Short: "Get details of an automatic backup workflow", | ||
| Run: cloud.GetAutobackup, | ||
| Args: cobra.ExactArgs(2), | ||
| }) | ||
|
|
||
| createAutobackupCmd := &cobra.Command{ | ||
| Use: "create <instance_id>", | ||
| Short: "Create an automatic backup workflow for the given instance", | ||
| Run: cloud.CreateAutobackup, | ||
| Args: cobra.ExactArgs(1), | ||
| } | ||
| createAutobackupCmd.Flags().StringVar(&cloud.AutobackupCreateParams.Cron, "cron", "", "Unix Cron pattern (e.g. '0 0 * * *')") | ||
| createAutobackupCmd.Flags().IntVar(&cloud.AutobackupCreateParams.Rotation, "rotation", 0, "Number of backups to keep") | ||
| createAutobackupCmd.Flags().StringVar(&cloud.AutobackupCreateParams.Name, "name", "", "Workflow name") | ||
| autobackupCmd.AddCommand(createAutobackupCmd) | ||
|
|
||
| autobackupCmd.AddCommand(&cobra.Command{ | ||
| Use: "delete <instance_id> <backup_workflow_id>", | ||
| Short: "Delete an automatic backup workflow", | ||
| Run: cloud.DeleteAutobackup, | ||
| Args: cobra.ExactArgs(2), | ||
| }) |
There was a problem hiding this comment.
New subcommands add multiple behaviors (instance group: get/create; autobackup: get/create) but the test suite only covers list/delete for these features. Please add command-level tests for the missing subcommands (including request/URL construction and basic output assertions) to match the coverage level of other instance commands in this test file.
Signed-off-by: Romain Dupont <romain.dupont-2@ovhcloud.com>
be14ed0 to
03ab350
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | Type | URL | Login | Password | Database | | ||
| | --- | --- | --- | --- | --- | | ||
| {{ range index .Result "accesses" }}| {{.type}} | {{.url}} | {{.login}} | {{.password}} | {{.database}} | | ||
| {{end}} |
There was a problem hiding this comment.
The markdown table header/separator lines start with ||, which creates an extra empty column and can prevent the table from being parsed/rendered consistently. Use a single leading | (as done in other templates like internal/services/cloud/templates/cloud_instance.tmpl) so the header and data rows have the same number of columns.
Add missing instance features