From e7f9e74d22fa1c712ac8363d30efe0499916e859 Mon Sep 17 00:00:00 2001 From: phm07 <22707808+phm07@users.noreply.github.com> Date: Tue, 2 Apr 2024 12:33:26 +0200 Subject: [PATCH] feat: improve missing argument error messages (#711) This PR adds a custom Cobra command validator that outputs useful errors when positional arguments are missing during command execution. For example, such an error could look like this: ``` $ hcloud server describe hcloud server describe [options] ^^^^^^ hcloud: expected argument server at position 1 ``` Where previously it would look like this: ``` $ hcloud server describe hcloud: accepts 1 arg(s), received 0 ``` Additionally, if there are more arguments provided than necessary, the following error message will appear: ``` $ hcloud server describe arg1 arg2 hcloud server describe [options] ^ hcloud: expected exactly 1 positional argument(s), but got 2 ``` This is done by parsing the `Use` property of a Cobra command with a regular expression and then matching the corresponding missing arguments. Fixes #700 --------- Co-authored-by: pauhull <22707808+pauhull@users.noreply.github.com> --- CONTRIBUTING.md | 24 +++- internal/cmd/all/all.go | 3 +- internal/cmd/base/cmd.go | 2 +- internal/cmd/base/create.go | 2 +- internal/cmd/base/delete.go | 5 +- internal/cmd/base/describe.go | 5 +- internal/cmd/base/labels.go | 8 +- internal/cmd/base/list.go | 1 + internal/cmd/base/set_rdns.go | 5 +- internal/cmd/base/update.go | 5 +- internal/cmd/certificate/certificate.go | 3 +- internal/cmd/certificate/create.go | 1 - internal/cmd/completion/completion.go | 3 +- internal/cmd/context/active.go | 3 +- internal/cmd/context/context.go | 3 +- internal/cmd/context/create.go | 3 +- internal/cmd/context/delete.go | 3 +- internal/cmd/context/list.go | 2 +- internal/cmd/context/use.go | 3 +- internal/cmd/datacenter/datacenter.go | 3 +- internal/cmd/firewall/add_rule.go | 1 - internal/cmd/firewall/apply_to_resource.go | 1 - internal/cmd/firewall/create.go | 1 - internal/cmd/firewall/delete_rule.go | 1 - internal/cmd/firewall/firewall.go | 2 +- internal/cmd/firewall/remove_from_resource.go | 1 - internal/cmd/firewall/replace_rules.go | 1 - internal/cmd/floatingip/assign.go | 1 - internal/cmd/floatingip/create.go | 1 - internal/cmd/floatingip/disable_protection.go | 1 - internal/cmd/floatingip/enable_protection.go | 1 - internal/cmd/floatingip/floatingip.go | 2 +- internal/cmd/floatingip/unassign.go | 1 - internal/cmd/image/disable_protection.go | 1 - internal/cmd/image/enable_protection.go | 1 - internal/cmd/image/image.go | 2 +- internal/cmd/iso/iso.go | 3 +- internal/cmd/loadbalancer/add_service.go | 1 - internal/cmd/loadbalancer/add_target.go | 1 - .../cmd/loadbalancer/attach_to_network.go | 1 - internal/cmd/loadbalancer/change_algorithm.go | 1 - internal/cmd/loadbalancer/change_type.go | 1 - internal/cmd/loadbalancer/create.go | 1 - internal/cmd/loadbalancer/delete_service.go | 1 - .../cmd/loadbalancer/detach_from_network.go | 1 - .../cmd/loadbalancer/disable_protection.go | 1 - .../loadbalancer/disable_public_interface.go | 1 - .../cmd/loadbalancer/enable_protection.go | 1 - .../loadbalancer/enable_public_interface.go | 1 - internal/cmd/loadbalancer/load_balancer.go | 2 +- internal/cmd/loadbalancer/metrics.go | 1 - internal/cmd/loadbalancer/remove_target.go | 1 - internal/cmd/loadbalancer/update_service.go | 1 - .../loadbalancertype/load_balancer_type.go | 3 +- internal/cmd/location/location.go | 3 +- internal/cmd/network/add_route.go | 1 - internal/cmd/network/add_subnet.go | 1 - internal/cmd/network/change_ip_range.go | 1 - internal/cmd/network/create.go | 1 - internal/cmd/network/disable_protection.go | 1 - internal/cmd/network/enable_protection.go | 1 - .../cmd/network/expose_routes_to_vswitch.go | 1 - internal/cmd/network/network.go | 2 +- internal/cmd/network/remove_route.go | 1 - internal/cmd/network/remove_subnet.go | 1 - internal/cmd/placementgroup/placementgroup.go | 3 +- internal/cmd/primaryip/assign.go | 1 - internal/cmd/primaryip/create.go | 1 - internal/cmd/primaryip/disable_protection.go | 1 - internal/cmd/primaryip/enable_protection.go | 1 - internal/cmd/primaryip/primaryip.go | 2 +- internal/cmd/primaryip/unassign.go | 1 - internal/cmd/server/add_to_placement_group.go | 1 - internal/cmd/server/attach_iso.go | 1 - internal/cmd/server/attach_to_network.go | 1 - internal/cmd/server/change_alias_ips.go | 1 - internal/cmd/server/change_type.go | 1 - internal/cmd/server/create_image.go | 1 - internal/cmd/server/detach_from_network.go | 1 - internal/cmd/server/detach_iso.go | 1 - internal/cmd/server/disable_backup.go | 1 - internal/cmd/server/disable_protection.go | 1 - internal/cmd/server/disable_rescue.go | 1 - internal/cmd/server/enable_backup.go | 1 - internal/cmd/server/enable_protection.go | 1 - internal/cmd/server/enable_rescue.go | 1 - internal/cmd/server/ip.go | 1 - internal/cmd/server/metrics.go | 1 - internal/cmd/server/poweroff.go | 1 - internal/cmd/server/poweron.go | 1 - internal/cmd/server/reboot.go | 1 - internal/cmd/server/rebuild.go | 1 - .../cmd/server/remove_from_placement_group.go | 1 - internal/cmd/server/request_console.go | 1 - internal/cmd/server/reset.go | 1 - internal/cmd/server/reset_password.go | 1 - internal/cmd/server/server.go | 2 +- internal/cmd/server/shutdown.go | 1 - internal/cmd/server/ssh.go | 1 - internal/cmd/servertype/server_type.go | 3 +- internal/cmd/sshkey/create.go | 1 - internal/cmd/sshkey/sshkey.go | 3 +- internal/cmd/util/util.go | 5 + internal/cmd/util/util_internal_test.go | 5 + internal/cmd/util/validation.go | 73 ++++++++++++ internal/cmd/util/validation_test.go | 108 ++++++++++++++++++ internal/cmd/version/version.go | 3 +- internal/cmd/volume/attach.go | 1 - internal/cmd/volume/create.go | 1 - internal/cmd/volume/detach.go | 1 - internal/cmd/volume/disable_protection.go | 1 - internal/cmd/volume/enable_protection.go | 1 - internal/cmd/volume/resize.go | 1 - internal/cmd/volume/volume.go | 2 +- 114 files changed, 270 insertions(+), 120 deletions(-) create mode 100644 internal/cmd/util/validation.go create mode 100644 internal/cmd/util/validation_test.go diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 93267bbf..db27c4cc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -139,8 +139,30 @@ Additional Commands: ssh Spawn an SSH connection for the server ``` +### Command validation + +Please use the `util.Validate` method to make sure the command is validated against its usage string during runtime. +This can be done by setting the `Args` field of the `cobra.Command` struct to `util.Validate`: + +```go +cmd := &cobra.Command{ + Use: "my-command [options]", + Args: util.Validate, + Run: func(cmd *cobra.Command, args []string) { + // ... + }, +} +``` + +If you are using base commands like `base.Cmd` or `base.ListCmd` etc., this is already done for you. + +**Note:** Your command usage needs to follow the [docopt](http://docopt.org/) syntax for this to work. +If your command uses optional positional arguments or other complicated usage that necessitates to disable +argument count checking, you use `util.ValidateLenient` instead. It will not throw an error if there are +more arguments than expected. + ### Generated files -Generated files (that are created by running `go generate`) should be prefixed with `zz_`. This is to group them +Generated files (that are created by running `go generate`) should be prefixed with `zz_`. This is to group them together in the file list and to easily identify them as generated files. Also, it allows the CI to check if the generated files are up-to-date. diff --git a/internal/cmd/all/all.go b/internal/cmd/all/all.go index 08e11b91..87182076 100644 --- a/internal/cmd/all/all.go +++ b/internal/cmd/all/all.go @@ -3,6 +3,7 @@ package all import ( "github.com/spf13/cobra" + "github.com/hetznercloud/cli/internal/cmd/util" "github.com/hetznercloud/cli/internal/state" ) @@ -10,7 +11,7 @@ func NewCommand(s state.State) *cobra.Command { cmd := &cobra.Command{ Use: "all", Short: "Commands that apply to all resources", - Args: cobra.NoArgs, + Args: util.Validate, TraverseChildren: true, DisableFlagsInUseLine: true, } diff --git a/internal/cmd/base/cmd.go b/internal/cmd/base/cmd.go index 079680d3..765aa42f 100644 --- a/internal/cmd/base/cmd.go +++ b/internal/cmd/base/cmd.go @@ -19,7 +19,7 @@ func (gc *Cmd) CobraCommand(s state.State) *cobra.Command { cmd := gc.BaseCobraCommand(s.Client()) if cmd.Args == nil { - cmd.Args = cobra.NoArgs + cmd.Args = util.Validate } cmd.TraverseChildren = true diff --git a/internal/cmd/base/create.go b/internal/cmd/base/create.go index cbd4460e..934f5928 100644 --- a/internal/cmd/base/create.go +++ b/internal/cmd/base/create.go @@ -27,7 +27,7 @@ func (cc *CreateCmd) CobraCommand(s state.State) *cobra.Command { output.AddFlag(cmd, output.OptionJSON(), output.OptionYAML()) if cmd.Args == nil { - cmd.Args = cobra.NoArgs + cmd.Args = util.Validate } cmd.TraverseChildren = true diff --git a/internal/cmd/base/delete.go b/internal/cmd/base/delete.go index 1f215805..1fb0b3b0 100644 --- a/internal/cmd/base/delete.go +++ b/internal/cmd/base/delete.go @@ -3,7 +3,6 @@ package base import ( "fmt" "reflect" - "strings" "github.com/spf13/cobra" @@ -32,9 +31,9 @@ func (dc *DeleteCmd) CobraCommand(s state.State) *cobra.Command { } cmd := &cobra.Command{ - Use: fmt.Sprintf("delete %s<%s>", opts, strings.ToLower(dc.ResourceNameSingular)), + Use: fmt.Sprintf("delete %s<%s>", opts, util.ToKebabCase(dc.ResourceNameSingular)), Short: dc.ShortDescription, - Args: cobra.ExactArgs(1), + Args: util.Validate, ValidArgsFunction: cmpl.SuggestArgs(cmpl.SuggestCandidatesF(dc.NameSuggestions(s.Client()))), TraverseChildren: true, DisableFlagsInUseLine: true, diff --git a/internal/cmd/base/describe.go b/internal/cmd/base/describe.go index 0e3b7bcd..c3c6c733 100644 --- a/internal/cmd/base/describe.go +++ b/internal/cmd/base/describe.go @@ -4,7 +4,6 @@ import ( "fmt" "os" "reflect" - "strings" "github.com/spf13/cobra" @@ -33,9 +32,9 @@ type DescribeCmd struct { // CobraCommand creates a command that can be registered with cobra. func (dc *DescribeCmd) CobraCommand(s state.State) *cobra.Command { cmd := &cobra.Command{ - Use: fmt.Sprintf("describe [options] <%s>", strings.ToLower(dc.ResourceNameSingular)), + Use: fmt.Sprintf("describe [options] <%s>", util.ToKebabCase(dc.ResourceNameSingular)), Short: dc.ShortDescription, - Args: cobra.ExactArgs(1), + Args: util.Validate, ValidArgsFunction: cmpl.SuggestArgs(cmpl.SuggestCandidatesF(dc.NameSuggestions(s.Client()))), TraverseChildren: true, DisableFlagsInUseLine: true, diff --git a/internal/cmd/base/labels.go b/internal/cmd/base/labels.go index a3aa4dbd..5feadeff 100644 --- a/internal/cmd/base/labels.go +++ b/internal/cmd/base/labels.go @@ -27,9 +27,9 @@ type LabelCmds struct { // AddCobraCommand creates a command that can be registered with cobra. func (lc *LabelCmds) AddCobraCommand(s state.State) *cobra.Command { cmd := &cobra.Command{ - Use: fmt.Sprintf("add-label [--overwrite] <%s>