Skip to content

Commit bc102d6

Browse files
EmyrkEmyrk
authored
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
1 parent 3f6a158 commit bc102d6

File tree

10 files changed

+186
-32
lines changed

10 files changed

+186
-32
lines changed

‎cli/clibase/cmd.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,18 @@ func (inv *Invocation) run(state *runState) error {
333333
)
334334
}
335335

336+
// All options should be set. Check all required options have sources,
337+
// meaning they were set by the user in some way (env, flag, etc).
338+
var missing []string
339+
for _, opt := range inv.Command.Options {
340+
if opt.Required && opt.ValueSource == ValueSourceNone {
341+
missing = append(missing, opt.Flag)
342+
}
343+
}
344+
if len(missing) > 0 {
345+
return xerrors.Errorf("Missing values for the required flags: %s", strings.Join(missing, ", "))
346+
}
347+
336348
if inv.Command.RawArgs {
337349
// If we're at the root command, then the name is omitted
338350
// from the arguments, so we can just use the entire slice.

‎cli/clibase/cmd_test.go

Lines changed: 87 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ func TestCommand(t *testing.T) {
3838
verbose bool
3939
lower bool
4040
prefix string
41+
reqBool bool
42+
reqStr string
4143
)
4244
return &clibase.Cmd{
4345
Use: "root [subcommand]",
@@ -54,6 +56,34 @@ func TestCommand(t *testing.T) {
5456
},
5557
},
5658
Children: []*clibase.Cmd{
59+
{
60+
Use: "required-flag --req-bool=true --req-string=foo",
61+
Short: "Example with required flags",
62+
Options: clibase.OptionSet{
63+
clibase.Option{
64+
Name: "req-bool",
65+
Flag: "req-bool",
66+
Value: clibase.BoolOf(&reqBool),
67+
Required: true,
68+
},
69+
clibase.Option{
70+
Name: "req-string",
71+
Flag: "req-string",
72+
Value: clibase.Validate(clibase.StringOf(&reqStr), func(value *clibase.String) error {
73+
ok := strings.Contains(value.String(), " ")
74+
if !ok {
75+
return xerrors.Errorf("string must contain a space")
76+
}
77+
return nil
78+
}),
79+
Required: true,
80+
},
81+
},
82+
Handler: func(i *clibase.Invocation) error {
83+
_, _ = i.Stdout.Write([]byte(fmt.Sprintf("%s-%t", reqStr, reqBool)))
84+
return nil
85+
},
86+
},
5787
{
5888
Use: "toupper [word]",
5989
Short: "Converts a word to upper case",
@@ -68,8 +98,8 @@ func TestCommand(t *testing.T) {
6898
Value: clibase.BoolOf(&lower),
6999
},
70100
},
71-
Handler: (func(i *clibase.Invocation) error {
72-
i.Stdout.Write([]byte(prefix))
101+
Handler: func(i *clibase.Invocation) error {
102+
_, _ = i.Stdout.Write([]byte(prefix))
73103
w := i.Args[0]
74104
if lower {
75105
w = strings.ToLower(w)
@@ -85,7 +115,7 @@ func TestCommand(t *testing.T) {
85115
i.Stdout.Write([]byte("!!!"))
86116
}
87117
return nil
88-
}),
118+
},
89119
},
90120
},
91121
}
@@ -213,6 +243,60 @@ func TestCommand(t *testing.T) {
213243
fio := fakeIO(i)
214244
require.Error(t, i.Run(), fio.Stdout.String())
215245
})
246+
247+
t.Run("RequiredFlagsMissing", func(t *testing.T) {
248+
t.Parallel()
249+
i := cmd().Invoke(
250+
"required-flag",
251+
)
252+
fio := fakeIO(i)
253+
err := i.Run()
254+
require.Error(t, err, fio.Stdout.String())
255+
require.ErrorContains(t, err, "Missing values")
256+
})
257+
258+
t.Run("RequiredFlagsMissingBool", func(t *testing.T) {
259+
t.Parallel()
260+
i := cmd().Invoke(
261+
"required-flag", "--req-string", "foo bar",
262+
)
263+
fio := fakeIO(i)
264+
err := i.Run()
265+
require.Error(t, err, fio.Stdout.String())
266+
require.ErrorContains(t, err, "Missing values for the required flags: req-bool")
267+
})
268+
269+
t.Run("RequiredFlagsMissingString", func(t *testing.T) {
270+
t.Parallel()
271+
i := cmd().Invoke(
272+
"required-flag", "--req-bool", "true",
273+
)
274+
fio := fakeIO(i)
275+
err := i.Run()
276+
require.Error(t, err, fio.Stdout.String())
277+
require.ErrorContains(t, err, "Missing values for the required flags: req-string")
278+
})
279+
280+
t.Run("RequiredFlagsInvalid", func(t *testing.T) {
281+
t.Parallel()
282+
i := cmd().Invoke(
283+
"required-flag", "--req-string", "nospace",
284+
)
285+
fio := fakeIO(i)
286+
err := i.Run()
287+
require.Error(t, err, fio.Stdout.String())
288+
require.ErrorContains(t, err, "string must contain a space")
289+
})
290+
291+
t.Run("RequiredFlagsOK", func(t *testing.T) {
292+
t.Parallel()
293+
i := cmd().Invoke(
294+
"required-flag", "--req-bool", "true", "--req-string", "foo bar",
295+
)
296+
fio := fakeIO(i)
297+
err := i.Run()
298+
require.NoError(t, err, fio.Stdout.String())
299+
})
216300
}
217301

218302
func TestCommand_DeepNest(t *testing.T) {

‎cli/clibase/option.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,10 @@ const (
2323
type Option struct {
2424
Name string `json:"name,omitempty"`
2525
Description string `json:"description,omitempty"`
26+
// Required means this value must be set by some means. It requires
27+
// `ValueSource != ValueSourceNone`
28+
// If `Default` is set, then `Required` is ignored.
29+
Required bool `json:"required,omitempty"`
2630

2731
// Flag is the long name of the flag used to configure this option. If unset,
2832
// flag configuring is disabled.

‎cli/clibase/values.go

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,40 @@ type NoOptDefValuer interface {
2424
NoOptDefValue() string
2525
}
2626

27+
// Validator is a wrapper around a pflag.Value that allows for validation
28+
// of the value after or before it has been set.
29+
type Validator[T pflag.Value] struct {
30+
Value T
31+
// validate is called after the value is set.
32+
validate func(T) error
33+
}
34+
35+
func Validate[T pflag.Value](opt T, validate func(value T) error) *Validator[T] {
36+
return &Validator[T]{Value: opt, validate: validate}
37+
}
38+
39+
func (i *Validator[T]) String() string {
40+
return i.Value.String()
41+
}
42+
43+
func (i *Validator[T]) Set(input string) error {
44+
err := i.Value.Set(input)
45+
if err != nil {
46+
return err
47+
}
48+
if i.validate != nil {
49+
err = i.validate(i.Value)
50+
if err != nil {
51+
return err
52+
}
53+
}
54+
return nil
55+
}
56+
57+
func (i *Validator[T]) Type() string {
58+
return i.Value.Type()
59+
}
60+
2761
// values.go contains a standard set of value types that can be used as
2862
// Option Values.
2963

@@ -329,10 +363,12 @@ type Struct[T any] struct {
329363
Value T
330364
}
331365

366+
//nolint:revive
332367
func (s *Struct[T]) Set(v string) error {
333368
return yaml.Unmarshal([]byte(v), &s.Value)
334369
}
335370

371+
//nolint:revive
336372
func (s *Struct[T]) String() string {
337373
byt, err := yaml.Marshal(s.Value)
338374
if err != nil {
@@ -361,6 +397,7 @@ func (s *Struct[T]) UnmarshalYAML(n *yaml.Node) error {
361397
return n.Decode(&s.Value)
362398
}
363399

400+
//nolint:revive
364401
func (s *Struct[T]) Type() string {
365402
return fmt.Sprintf("struct[%T]", s.Value)
366403
}

‎coderd/apidoc/docs.go

Lines changed: 4 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json

Lines changed: 4 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎docs/api/general.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -398,6 +398,7 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \
398398
},
399399
"hidden": true,
400400
"name": "string",
401+
"required": true,
401402
"use_instead": [{}],
402403
"value": null,
403404
"value_source": "",

‎docs/api/schemas.md

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -533,6 +533,7 @@
533533
},
534534
"hidden": true,
535535
"name": "string",
536+
"required": true,
536537
"use_instead": [
537538
{
538539
"annotations": {
@@ -557,6 +558,7 @@
557558
},
558559
"hidden": true,
559560
"name": "string",
561+
"required": true,
560562
"use_instead": [],
561563
"value": null,
562564
"value_source": "",
@@ -571,21 +573,22 @@
571573

572574
### Properties
573575

574-
| Name | Type | Required | Restrictions | Description |
575-
| ---------------- | ------------------------------------------ | -------- | ------------ | ------------------------------------------------------------------------------------------------------------------------------ |
576-
| `annotations` | [clibase.Annotations](#clibaseannotations) | false | | Annotations enable extensions to clibase higher up in the stack. It's useful for help formatting and documentation generation. |
577-
| `default` | string | false | | Default is parsed into Value if set. |
578-
| `description` | string | false | | |
579-
| `env` | string | false | | Env is the environment variable used to configure this option. If unset, environment configuring is disabled. |
580-
| `flag` | string | false | | Flag is the long name of the flag used to configure this option. If unset, flag configuring is disabled. |
581-
| `flag_shorthand` | string | false | | Flag shorthand is the one-character shorthand for the flag. If unset, no shorthand is used. |
582-
| `group` | [clibase.Group](#clibasegroup) | false | | Group is a group hierarchy that helps organize this option in help, configs and other documentation. |
583-
| `hidden` | boolean | false | | |
584-
| `name` | string | false | | |
585-
| `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. |
586-
| `value` | any | false | | Value includes the types listed in values.go. |
587-
| `value_source` | [clibase.ValueSource](#clibasevaluesource) | false | | |
588-
| `yaml` | string | false | | Yaml is the YAML key used to configure this option. If unset, YAML configuring is disabled. |
576+
| Name | Type | Required | Restrictions | Description |
577+
| ---------------- | ------------------------------------------ | -------- | ------------ | -------------------------------------------------------------------------------------------------------------------------------------------------- |
578+
| `annotations` | [clibase.Annotations](#clibaseannotations) | false | | Annotations enable extensions to clibase higher up in the stack. It's useful for help formatting and documentation generation. |
579+
| `default` | string | false | | Default is parsed into Value if set. |
580+
| `description` | string | false | | |
581+
| `env` | string | false | | Env is the environment variable used to configure this option. If unset, environment configuring is disabled. |
582+
| `flag` | string | false | | Flag is the long name of the flag used to configure this option. If unset, flag configuring is disabled. |
583+
| `flag_shorthand` | string | false | | Flag shorthand is the one-character shorthand for the flag. If unset, no shorthand is used. |
584+
| `group` | [clibase.Group](#clibasegroup) | false | | Group is a group hierarchy that helps organize this option in help, configs and other documentation. |
585+
| `hidden` | boolean | false | | |
586+
| `name` | string | false | | |
587+
| `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. |
588+
| `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. |
589+
| `value` | any | false | | Value includes the types listed in values.go. |
590+
| `value_source` | [clibase.ValueSource](#clibasevaluesource) | false | | |
591+
| `yaml` | string | false | | Yaml is the YAML key used to configure this option. If unset, YAML configuring is disabled. |
589592

590593
## clibase.Struct-array_codersdk_GitAuthConfig
591594

@@ -2099,6 +2102,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in
20992102
},
21002103
"hidden": true,
21012104
"name": "string",
2105+
"required": true,
21022106
"use_instead": [{}],
21032107
"value": null,
21042108
"value_source": "",

‎enterprise/cli/proxyserver.go

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ func (*RootCmd) proxyServer() *clibase.Cmd {
6565
Flag: "proxy-session-token",
6666
Env: "CODER_PROXY_SESSION_TOKEN",
6767
YAML: "proxySessionToken",
68-
Default: "",
68+
Required: true,
6969
Value: &proxySessionToken,
7070
Group: &externalProxyOptionGroup,
7171
Hidden: false,
@@ -77,10 +77,15 @@ func (*RootCmd) proxyServer() *clibase.Cmd {
7777
Flag: "primary-access-url",
7878
Env: "CODER_PRIMARY_ACCESS_URL",
7979
YAML: "primaryAccessURL",
80-
Default: "",
81-
Value: &primaryAccessURL,
82-
Group: &externalProxyOptionGroup,
83-
Hidden: false,
80+
Required: true,
81+
Value: clibase.Validate(&primaryAccessURL, func(value *clibase.URL) error {
82+
if !(value.Scheme == "http" || value.Scheme == "https") {
83+
return xerrors.Errorf("'--primary-access-url' value must be http or https: url=%s", primaryAccessURL.String())
84+
}
85+
return nil
86+
}),
87+
Group: &externalProxyOptionGroup,
88+
Hidden: false,
8489
},
8590
)
8691

@@ -94,10 +99,6 @@ func (*RootCmd) proxyServer() *clibase.Cmd {
9499
clibase.RequireNArgs(0),
95100
),
96101
Handler: func(inv *clibase.Invocation) error {
97-
if !(primaryAccessURL.Scheme == "http" || primaryAccessURL.Scheme == "https") {
98-
return xerrors.Errorf("'--primary-access-url' value must be http or https: url=%s", primaryAccessURL.String())
99-
}
100-
101102
var closers closers
102103
// Main command context for managing cancellation of running
103104
// services.

‎enterprise/cli/workspaceproxy.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,12 @@ func (r *RootCmd) createProxy() *clibase.Cmd {
235235
noPrompts bool
236236
formatter = newUpdateProxyResponseFormatter()
237237
)
238+
validateIcon := func(s *clibase.String) error {
239+
if !(strings.HasPrefix(s.Value(), "/emojis/") || strings.HasPrefix(s.Value(), "http")) {
240+
return xerrors.New("icon must be a relative path to an emoji or a publicly hosted image URL")
241+
}
242+
return nil
243+
}
238244

239245
client := new(codersdk.Client)
240246
cmd := &clibase.Cmd{
@@ -271,10 +277,7 @@ func (r *RootCmd) createProxy() *clibase.Cmd {
271277
Text: "Icon URL:",
272278
Default: "/emojis/1f5fa.png",
273279
Validate: func(s string) error {
274-
if !(strings.HasPrefix(s, "/emojis/") || strings.HasPrefix(s, "http")) {
275-
return xerrors.New("icon must be a relative path to an emoji or a publicly hosted image URL")
276-
}
277-
return nil
280+
return validateIcon(clibase.StringOf(&s))
278281
},
279282
})
280283
if err != nil {
@@ -319,7 +322,7 @@ func (r *RootCmd) createProxy() *clibase.Cmd {
319322
clibase.Option{
320323
Flag: "icon",
321324
Description: "Display icon of the proxy.",
322-
Value: clibase.StringOf(&proxyIcon),
325+
Value: clibase.Validate(clibase.StringOf(&proxyIcon), validateIcon),
323326
},
324327
clibase.Option{
325328
Flag: "no-prompt",

0 commit comments

Comments
 (0)