From bc102d6bd701c2ccacb26d5b7d8fce7c9f5b278d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 11 Jul 2023 09:59:55 -0400 Subject: [PATCH] feat: add cli first class validation (#8374) * feat: add cli first class validation * feat: add required flag to cli options * Add unit test to catch invalid and missing flag --- cli/clibase/cmd.go | 12 +++++ cli/clibase/cmd_test.go | 90 ++++++++++++++++++++++++++++++-- cli/clibase/option.go | 4 ++ cli/clibase/values.go | 37 +++++++++++++ coderd/apidoc/docs.go | 4 ++ coderd/apidoc/swagger.json | 4 ++ docs/api/general.md | 1 + docs/api/schemas.md | 34 ++++++------ enterprise/cli/proxyserver.go | 19 +++---- enterprise/cli/workspaceproxy.go | 13 +++-- 10 files changed, 186 insertions(+), 32 deletions(-) diff --git a/cli/clibase/cmd.go b/cli/clibase/cmd.go index 0d4258d460..3e7dfe3903 100644 --- a/cli/clibase/cmd.go +++ b/cli/clibase/cmd.go @@ -333,6 +333,18 @@ func (inv *Invocation) run(state *runState) error { ) } + // All options should be set. Check all required options have sources, + // meaning they were set by the user in some way (env, flag, etc). + var missing []string + for _, opt := range inv.Command.Options { + if opt.Required && opt.ValueSource == ValueSourceNone { + missing = append(missing, opt.Flag) + } + } + if len(missing) > 0 { + return xerrors.Errorf("Missing values for the required flags: %s", strings.Join(missing, ", ")) + } + if inv.Command.RawArgs { // If we're at the root command, then the name is omitted // from the arguments, so we can just use the entire slice. diff --git a/cli/clibase/cmd_test.go b/cli/clibase/cmd_test.go index 686ddf9ed9..fedbcd0cf2 100644 --- a/cli/clibase/cmd_test.go +++ b/cli/clibase/cmd_test.go @@ -38,6 +38,8 @@ func TestCommand(t *testing.T) { verbose bool lower bool prefix string + reqBool bool + reqStr string ) return &clibase.Cmd{ Use: "root [subcommand]", @@ -54,6 +56,34 @@ func TestCommand(t *testing.T) { }, }, Children: []*clibase.Cmd{ + { + Use: "required-flag --req-bool=true --req-string=foo", + Short: "Example with required flags", + Options: clibase.OptionSet{ + clibase.Option{ + Name: "req-bool", + Flag: "req-bool", + Value: clibase.BoolOf(&reqBool), + Required: true, + }, + clibase.Option{ + Name: "req-string", + Flag: "req-string", + Value: clibase.Validate(clibase.StringOf(&reqStr), func(value *clibase.String) error { + ok := strings.Contains(value.String(), " ") + if !ok { + return xerrors.Errorf("string must contain a space") + } + return nil + }), + Required: true, + }, + }, + Handler: func(i *clibase.Invocation) error { + _, _ = i.Stdout.Write([]byte(fmt.Sprintf("%s-%t", reqStr, reqBool))) + return nil + }, + }, { Use: "toupper [word]", Short: "Converts a word to upper case", @@ -68,8 +98,8 @@ func TestCommand(t *testing.T) { Value: clibase.BoolOf(&lower), }, }, - Handler: (func(i *clibase.Invocation) error { - i.Stdout.Write([]byte(prefix)) + Handler: func(i *clibase.Invocation) error { + _, _ = i.Stdout.Write([]byte(prefix)) w := i.Args[0] if lower { w = strings.ToLower(w) @@ -85,7 +115,7 @@ func TestCommand(t *testing.T) { i.Stdout.Write([]byte("!!!")) } return nil - }), + }, }, }, } @@ -213,6 +243,60 @@ func TestCommand(t *testing.T) { fio := fakeIO(i) require.Error(t, i.Run(), fio.Stdout.String()) }) + + t.Run("RequiredFlagsMissing", func(t *testing.T) { + t.Parallel() + i := cmd().Invoke( + "required-flag", + ) + fio := fakeIO(i) + err := i.Run() + require.Error(t, err, fio.Stdout.String()) + require.ErrorContains(t, err, "Missing values") + }) + + t.Run("RequiredFlagsMissingBool", func(t *testing.T) { + t.Parallel() + i := cmd().Invoke( + "required-flag", "--req-string", "foo bar", + ) + fio := fakeIO(i) + err := i.Run() + require.Error(t, err, fio.Stdout.String()) + require.ErrorContains(t, err, "Missing values for the required flags: req-bool") + }) + + t.Run("RequiredFlagsMissingString", func(t *testing.T) { + t.Parallel() + i := cmd().Invoke( + "required-flag", "--req-bool", "true", + ) + fio := fakeIO(i) + err := i.Run() + require.Error(t, err, fio.Stdout.String()) + require.ErrorContains(t, err, "Missing values for the required flags: req-string") + }) + + t.Run("RequiredFlagsInvalid", func(t *testing.T) { + t.Parallel() + i := cmd().Invoke( + "required-flag", "--req-string", "nospace", + ) + fio := fakeIO(i) + err := i.Run() + require.Error(t, err, fio.Stdout.String()) + require.ErrorContains(t, err, "string must contain a space") + }) + + t.Run("RequiredFlagsOK", func(t *testing.T) { + t.Parallel() + i := cmd().Invoke( + "required-flag", "--req-bool", "true", "--req-string", "foo bar", + ) + fio := fakeIO(i) + err := i.Run() + require.NoError(t, err, fio.Stdout.String()) + }) } func TestCommand_DeepNest(t *testing.T) { diff --git a/cli/clibase/option.go b/cli/clibase/option.go index ed0ea17bf8..8c7fed92e2 100644 --- a/cli/clibase/option.go +++ b/cli/clibase/option.go @@ -23,6 +23,10 @@ const ( type Option struct { Name string `json:"name,omitempty"` Description string `json:"description,omitempty"` + // Required means this value must be set by some means. It requires + // `ValueSource != ValueSourceNone` + // If `Default` is set, then `Required` is ignored. + Required bool `json:"required,omitempty"` // Flag is the long name of the flag used to configure this option. If unset, // flag configuring is disabled. diff --git a/cli/clibase/values.go b/cli/clibase/values.go index 55f808ad31..288a7c372b 100644 --- a/cli/clibase/values.go +++ b/cli/clibase/values.go @@ -24,6 +24,40 @@ type NoOptDefValuer interface { NoOptDefValue() string } +// Validator is a wrapper around a pflag.Value that allows for validation +// of the value after or before it has been set. +type Validator[T pflag.Value] struct { + Value T + // validate is called after the value is set. + validate func(T) error +} + +func Validate[T pflag.Value](opt T, validate func(value T) error) *Validator[T] { + return &Validator[T]{Value: opt, validate: validate} +} + +func (i *Validator[T]) String() string { + return i.Value.String() +} + +func (i *Validator[T]) Set(input string) error { + err := i.Value.Set(input) + if err != nil { + return err + } + if i.validate != nil { + err = i.validate(i.Value) + if err != nil { + return err + } + } + return nil +} + +func (i *Validator[T]) Type() string { + return i.Value.Type() +} + // values.go contains a standard set of value types that can be used as // Option Values. @@ -329,10 +363,12 @@ type Struct[T any] struct { Value T } +//nolint:revive func (s *Struct[T]) Set(v string) error { return yaml.Unmarshal([]byte(v), &s.Value) } +//nolint:revive func (s *Struct[T]) String() string { byt, err := yaml.Marshal(s.Value) if err != nil { @@ -361,6 +397,7 @@ func (s *Struct[T]) UnmarshalYAML(n *yaml.Node) error { return n.Decode(&s.Value) } +//nolint:revive func (s *Struct[T]) Type() string { return fmt.Sprintf("struct[%T]", s.Value) } diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 47ea6ba415..69fbc969e2 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -6239,6 +6239,10 @@ const docTemplate = `{ "name": { "type": "string" }, + "required": { + "description": "Required means this value must be set by some means. It requires\n` + "`" + `ValueSource != ValueSourceNone` + "`" + `\nIf ` + "`" + `Default` + "`" + ` is set, then ` + "`" + `Required` + "`" + ` is ignored.", + "type": "boolean" + }, "use_instead": { "description": "UseInstead is a list of options that should be used instead of this one.\nThe field is used to generate a deprecation warning.", "type": "array", diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index e0428e6676..ce6a2a67a4 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -5529,6 +5529,10 @@ "name": { "type": "string" }, + "required": { + "description": "Required means this value must be set by some means. It requires\n`ValueSource != ValueSourceNone`\nIf `Default` is set, then `Required` is ignored.", + "type": "boolean" + }, "use_instead": { "description": "UseInstead is a list of options that should be used instead of this one.\nThe field is used to generate a deprecation warning.", "type": "array", diff --git a/docs/api/general.md b/docs/api/general.md index 45b12d7304..9ac3ff3808 100644 --- a/docs/api/general.md +++ b/docs/api/general.md @@ -398,6 +398,7 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \ }, "hidden": true, "name": "string", + "required": true, "use_instead": [{}], "value": null, "value_source": "", diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 7cde3c40f9..f68d5bae1b 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -533,6 +533,7 @@ }, "hidden": true, "name": "string", + "required": true, "use_instead": [ { "annotations": { @@ -557,6 +558,7 @@ }, "hidden": true, "name": "string", + "required": true, "use_instead": [], "value": null, "value_source": "", @@ -571,21 +573,22 @@ ### Properties -| Name | Type | Required | Restrictions | Description | -| ---------------- | ------------------------------------------ | -------- | ------------ | ------------------------------------------------------------------------------------------------------------------------------ | -| `annotations` | [clibase.Annotations](#clibaseannotations) | false | | Annotations enable extensions to clibase higher up in the stack. It's useful for help formatting and documentation generation. | -| `default` | string | false | | Default is parsed into Value if set. | -| `description` | string | false | | | -| `env` | string | false | | Env is the environment variable used to configure this option. If unset, environment configuring is disabled. | -| `flag` | string | false | | Flag is the long name of the flag used to configure this option. If unset, flag configuring is disabled. | -| `flag_shorthand` | string | false | | Flag shorthand is the one-character shorthand for the flag. If unset, no shorthand is used. | -| `group` | [clibase.Group](#clibasegroup) | false | | Group is a group hierarchy that helps organize this option in help, configs and other documentation. | -| `hidden` | boolean | false | | | -| `name` | string | false | | | -| `use_instead` | array of [clibase.Option](#clibaseoption) | false | | Use instead is a list of options that should be used instead of this one. The field is used to generate a deprecation warning. | -| `value` | any | false | | Value includes the types listed in values.go. | -| `value_source` | [clibase.ValueSource](#clibasevaluesource) | false | | | -| `yaml` | string | false | | Yaml is the YAML key used to configure this option. If unset, YAML configuring is disabled. | +| Name | Type | Required | Restrictions | Description | +| ---------------- | ------------------------------------------ | -------- | ------------ | -------------------------------------------------------------------------------------------------------------------------------------------------- | +| `annotations` | [clibase.Annotations](#clibaseannotations) | false | | Annotations enable extensions to clibase higher up in the stack. It's useful for help formatting and documentation generation. | +| `default` | string | false | | Default is parsed into Value if set. | +| `description` | string | false | | | +| `env` | string | false | | Env is the environment variable used to configure this option. If unset, environment configuring is disabled. | +| `flag` | string | false | | Flag is the long name of the flag used to configure this option. If unset, flag configuring is disabled. | +| `flag_shorthand` | string | false | | Flag shorthand is the one-character shorthand for the flag. If unset, no shorthand is used. | +| `group` | [clibase.Group](#clibasegroup) | false | | Group is a group hierarchy that helps organize this option in help, configs and other documentation. | +| `hidden` | boolean | false | | | +| `name` | string | false | | | +| `required` | boolean | false | | Required means this value must be set by some means. It requires `ValueSource != ValueSourceNone` If `Default` is set, then `Required` is ignored. | +| `use_instead` | array of [clibase.Option](#clibaseoption) | false | | Use instead is a list of options that should be used instead of this one. The field is used to generate a deprecation warning. | +| `value` | any | false | | Value includes the types listed in values.go. | +| `value_source` | [clibase.ValueSource](#clibasevaluesource) | false | | | +| `yaml` | string | false | | Yaml is the YAML key used to configure this option. If unset, YAML configuring is disabled. | ## clibase.Struct-array_codersdk_GitAuthConfig @@ -2099,6 +2102,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in }, "hidden": true, "name": "string", + "required": true, "use_instead": [{}], "value": null, "value_source": "", diff --git a/enterprise/cli/proxyserver.go b/enterprise/cli/proxyserver.go index e48f0fbdef..afef827e6c 100644 --- a/enterprise/cli/proxyserver.go +++ b/enterprise/cli/proxyserver.go @@ -65,7 +65,7 @@ func (*RootCmd) proxyServer() *clibase.Cmd { Flag: "proxy-session-token", Env: "CODER_PROXY_SESSION_TOKEN", YAML: "proxySessionToken", - Default: "", + Required: true, Value: &proxySessionToken, Group: &externalProxyOptionGroup, Hidden: false, @@ -77,10 +77,15 @@ func (*RootCmd) proxyServer() *clibase.Cmd { Flag: "primary-access-url", Env: "CODER_PRIMARY_ACCESS_URL", YAML: "primaryAccessURL", - Default: "", - Value: &primaryAccessURL, - Group: &externalProxyOptionGroup, - Hidden: false, + Required: true, + Value: clibase.Validate(&primaryAccessURL, func(value *clibase.URL) error { + if !(value.Scheme == "http" || value.Scheme == "https") { + return xerrors.Errorf("'--primary-access-url' value must be http or https: url=%s", primaryAccessURL.String()) + } + return nil + }), + Group: &externalProxyOptionGroup, + Hidden: false, }, ) @@ -94,10 +99,6 @@ func (*RootCmd) proxyServer() *clibase.Cmd { clibase.RequireNArgs(0), ), Handler: func(inv *clibase.Invocation) error { - if !(primaryAccessURL.Scheme == "http" || primaryAccessURL.Scheme == "https") { - return xerrors.Errorf("'--primary-access-url' value must be http or https: url=%s", primaryAccessURL.String()) - } - var closers closers // Main command context for managing cancellation of running // services. diff --git a/enterprise/cli/workspaceproxy.go b/enterprise/cli/workspaceproxy.go index 484bbf5742..6c0f7aa06e 100644 --- a/enterprise/cli/workspaceproxy.go +++ b/enterprise/cli/workspaceproxy.go @@ -235,6 +235,12 @@ func (r *RootCmd) createProxy() *clibase.Cmd { noPrompts bool formatter = newUpdateProxyResponseFormatter() ) + validateIcon := func(s *clibase.String) error { + if !(strings.HasPrefix(s.Value(), "/emojis/") || strings.HasPrefix(s.Value(), "http")) { + return xerrors.New("icon must be a relative path to an emoji or a publicly hosted image URL") + } + return nil + } client := new(codersdk.Client) cmd := &clibase.Cmd{ @@ -271,10 +277,7 @@ func (r *RootCmd) createProxy() *clibase.Cmd { Text: "Icon URL:", Default: "/emojis/1f5fa.png", Validate: func(s string) error { - if !(strings.HasPrefix(s, "/emojis/") || strings.HasPrefix(s, "http")) { - return xerrors.New("icon must be a relative path to an emoji or a publicly hosted image URL") - } - return nil + return validateIcon(clibase.StringOf(&s)) }, }) if err != nil { @@ -319,7 +322,7 @@ func (r *RootCmd) createProxy() *clibase.Cmd { clibase.Option{ Flag: "icon", Description: "Display icon of the proxy.", - Value: clibase.StringOf(&proxyIcon), + Value: clibase.Validate(clibase.StringOf(&proxyIcon), validateIcon), }, clibase.Option{ Flag: "no-prompt",