opensource/coder
3
0
Fork 0
mirror of https://github.com/coder/coder.git synced 2026-09-21 20:51:01 +08:00
Code Issues Packages Projects Releases Wiki Activity

feat: add cli first class validation (#8374)

Browse Source 
* feat: add cli first class validation
* feat: add required flag to cli options
* Add unit test to catch invalid and missing flag
This commit is contained in:
Steven Masley
2023-07-11 09:59:55 -04:00
committed by GitHub
parent 3f6a158016
commit bc102d6bd7
10 changed files with 186 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files
cli/clibase/cmd.go
+12
View File
@@ -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.
cli/clibase/cmd_test.go
+87 -3
View File
@@ -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) {
cli/clibase/option.go
+4
View File
@@ -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.
cli/clibase/values.go
+37
View File
@@ -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)
}
coderd/apidoc/docs.go
Generated
+4
View File
@@ -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",
coderd/apidoc/swagger.json
Generated
+4
View File
@@ -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",
docs/api/general.md
+1
View File
@@ -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": "",
docs/api/schemas.md
+19 -15
View File
@@ -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": "",
enterprise/cli/proxyserver.go
+10 -9
View File
@@ -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.
enterprise/cli/workspaceproxy.go
+8 -5
View File
@@ -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",