Skip to content

Commit ed6ccb0

Browse files
WanzenBugWanzenBug
committed
CLI: Make advanced arg parsing reusable
The --disk flag is special and uses go tags and some reflection to do the actual argument parsing. This is useful for other flags that will be added in the near future. As such the actual magic of splitting the argument and parsing it into a tagged struct has been extracted into a new package. The tag mini-language has been cleaned up a bit to bring it in line with similar tags such as `json` and `toml`. The new parser supports filling the struct directly from a string without having to manually implement conversion from a map. It also supports any field type implementing encoding.TextUnmarshaler.
1 parent 9c55905 commit ed6ccb0

File tree

4 files changed

+334
-136
lines changed

4 files changed

+334
-136
lines changed

‎cmd/disk.go

+24-91
Original file line numberDiff line numberDiff line change
@@ -2,111 +2,44 @@ package cmd
22

33
import (
44
"fmt"
5-
"reflect"
6-
"strings"
7-
5+
"github.com/LINBIT/virter/pkg/cliutils"
86
"github.com/rck/unit"
9-
log "github.com/sirupsen/logrus"
107
)
118

129
// DiskArg represents a disk that can be passed to virter via a command line argument.
1310
type DiskArg struct {
14-
Name string `key:"name" required:"true"`
15-
SizeKiB uint64 `key:"size" required:"true"`
16-
Format string `key:"format" default:"qcow2"`
17-
Bus string `key:"bus" default:"virtio"`
11+
Name string `arg:"name"`
12+
Size Size `arg:"size"`
13+
Format string `arg:"format,qcow2"`
14+
Bus string `arg:"bus,virtio"`
1815
}
1916

20-
func (d *DiskArg) GetName() string { return d.Name }
21-
func (d *DiskArg) GetSizeKiB() uint64 { return d.SizeKiB }
22-
func (d *DiskArg) GetFormat() string { return d.Format }
23-
func (d *DiskArg) GetBus() string { return d.Bus }
24-
25-
func parseArgMap(str string) (map[string]string, error) {
26-
result := map[string]string{}
27-
28-
kvpairs := strings.Split(str, ",")
29-
for _, kvpair := range kvpairs {
30-
if kvpair == "" {
31-
continue
32-
}
33-
kv := strings.Split(kvpair, "=")
34-
if len(kv) != 2 {
35-
return nil, fmt.Errorf("malformed key/value pair '%s': expected exactly one '='", kvpair)
36-
}
37-
38-
key := strings.TrimSpace(kv[0])
39-
value := strings.TrimSpace(kv[1])
40-
if key == "" {
41-
return nil, fmt.Errorf("malformed key/value pair '%s': key cannot be empty", kvpair)
42-
}
43-
44-
result[key] = value
45-
}
46-
47-
return result, nil
17+
type Size struct {
18+
KiB uint64
4819
}
4920

50-
func fillDefaultValues(p map[string]string) (map[string]string, error) {
51-
t := reflect.TypeOf(DiskArg{})
52-
for i := 0; i < t.NumField(); i++ {
53-
field := t.Field(i)
54-
key := field.Tag.Get("key")
55-
required := field.Tag.Get("required")
56-
defaultValue := field.Tag.Get("default")
57-
58-
if _, ok := p[key]; !ok {
59-
if required == "true" {
60-
return nil, fmt.Errorf("missing required parameter '%s'", key)
61-
} else {
62-
p[key] = defaultValue
63-
}
64-
}
21+
func (s *Size) UnmarshalText(text []byte) error {
22+
u := unit.MustNewUnit(sizeUnits)
23+
val, err := u.ValueFromString(string(text))
24+
if err != nil {
25+
return fmt.Errorf("invalid size: %w", err)
6526
}
66-
67-
return p, nil
27+
signedSizeKiB := val.Value / sizeUnits["K"]
28+
if signedSizeKiB < 0 {
29+
return fmt.Errorf("invalid size: must be positive number")
30+
}
31+
s.KiB = uint64(signedSizeKiB)
32+
return nil
6833
}
6934

35+
func (d *DiskArg) GetName() string { return d.Name }
36+
func (d *DiskArg) GetSizeKiB() uint64 { return d.Size.KiB }
37+
func (d *DiskArg) GetFormat() string { return d.Format }
38+
func (d *DiskArg) GetBus() string { return d.Bus }
39+
7040
// Set implements flag.Value.Set.
7141
func (d *DiskArg) Set(str string) error {
72-
if len(str) == 0 {
73-
return fmt.Errorf("invalid empty disk specification")
74-
}
75-
76-
params, err := parseArgMap(str)
77-
if err != nil {
78-
return fmt.Errorf("failed to parse disk specification: %w", err)
79-
}
80-
81-
params, err = fillDefaultValues(params)
82-
if err != nil {
83-
return fmt.Errorf("failed to parse disk specification: %w", err)
84-
}
85-
86-
for k, v := range params {
87-
switch k {
88-
case "name":
89-
d.Name = v
90-
case "size":
91-
u := unit.MustNewUnit(sizeUnits)
92-
val, err := u.ValueFromString(v)
93-
if err != nil {
94-
return fmt.Errorf("invalid size: %w", err)
95-
}
96-
signedSizeKiB := val.Value / sizeUnits["K"]
97-
if signedSizeKiB < 0 {
98-
return fmt.Errorf("invalid size: must be positive number")
99-
}
100-
d.SizeKiB = uint64(signedSizeKiB)
101-
case "format":
102-
d.Format = v
103-
case "bus":
104-
d.Bus = v
105-
default:
106-
log.Debugf("ignoring unknown disk key: %v", k)
107-
}
108-
}
109-
return nil
42+
return cliutils.Parse(str, d)
11043
}
11144

11245
// Type implements pflag.Value.Type.

‎cmd/disk_test.go

+78-45
Original file line numberDiff line numberDiff line change
@@ -1,83 +1,116 @@
1-
package cmd
1+
package cmd_test
22

33
import (
4+
"github.com/LINBIT/virter/cmd"
5+
"github.com/rck/unit"
46
"reflect"
57
"testing"
68
)
79

8-
func TestParseArgMap(t *testing.T) {
10+
func TestFromFlag(t *testing.T) {
911
cases := []struct {
12+
name string
1013
input string
11-
expect map[string]string
14+
expect cmd.DiskArg
1215
expectError bool
1316
}{
1417
{
15-
input: "",
16-
expect: map[string]string{},
18+
name: "empty arg is error",
19+
input: "",
20+
expectError: true,
1721
}, {
22+
name: "full spec parses",
1823
input: "name=test,size=5GiB,format=qcow2,bus=virtio",
19-
expect: map[string]string{
20-
"name": "test",
21-
"size": "5GiB",
22-
"format": "qcow2",
23-
"bus": "virtio",
24+
expect: cmd.DiskArg{
25+
Name: "test",
26+
Size: cmd.Size{KiB: uint64(5 * unit.G / unit.K)},
27+
Format: "qcow2",
28+
Bus: "virtio",
2429
},
2530
}, {
26-
input: "name=test",
27-
expect: map[string]string{
28-
"name": "test",
31+
name: "only required args parses",
32+
input: "name=test,size=10G",
33+
expect: cmd.DiskArg{
34+
Name: "test",
35+
Size: cmd.Size{KiB: uint64(10 * unit.G / unit.K)},
36+
Format: "qcow2",
37+
Bus: "virtio",
2938
},
3039
}, {
31-
input: ",,,",
32-
expect: map[string]string{},
40+
name: "empty but different is error",
41+
input: ",,,",
42+
expectError: true,
3343
}, {
34-
input: ",name=test,",
35-
expect: map[string]string{
36-
"name": "test",
44+
name: "only required with empty kv-pairs parses",
45+
input: ",name=test,,,size=1G",
46+
expect: cmd.DiskArg{
47+
Name: "test",
48+
Size: cmd.Size{KiB: uint64(1 * unit.G / unit.K)},
49+
Format: "qcow2",
50+
Bus: "virtio",
3751
},
3852
}, {
53+
name: "nonsence input is error",
3954
input: "x,y,z",
4055
expectError: true,
4156
}, {
42-
input: "name=",
43-
expect: map[string]string{
44-
"name": "",
45-
},
57+
name: "missing size is error",
58+
input: "name=test",
59+
expectError: true,
4660
}, {
47-
input: "=test",
61+
name: "missing key is error",
62+
input: "=test,name=bla,size=1G",
4863
expectError: true,
4964
}, {
65+
name: "multi-value is error",
5066
input: "name=test=hello",
5167
expectError: true,
5268
}, {
53-
input: "name=test hello",
54-
expect: map[string]string{
55-
"name": "test hello",
69+
name: "whitepace is okay",
70+
input: "name=test hello,size=1G",
71+
expect: cmd.DiskArg{
72+
Name: "test hello",
73+
Size: cmd.Size{KiB: uint64(1 * unit.G / unit.K)},
74+
Format: "qcow2",
75+
Bus: "virtio",
5676
},
5777
}, {
58-
input: "name=test,name=hello",
59-
expect: map[string]string{
60-
"name": "hello",
78+
name: "repeated keys are okay",
79+
input: "name=test,name=hello,size=1G",
80+
expect: cmd.DiskArg{
81+
Name: "hello",
82+
Size: cmd.Size{KiB: uint64(1 * unit.G / unit.K)},
83+
Format: "qcow2",
84+
Bus: "virtio",
6185
},
6286
},
6387
}
6488

65-
for _, c := range cases {
66-
actual, err := parseArgMap(c.input)
67-
if !c.expectError && err != nil {
68-
t.Errorf("on input '%s':", c.input)
69-
t.Fatalf("unexpected error: %v", err)
70-
}
71-
if c.expectError && err == nil {
72-
t.Errorf("on input '%s':", c.input)
73-
t.Fatal("expected error, got nil")
74-
}
89+
t.Parallel()
90+
for i := range cases {
91+
c := cases[i]
92+
t.Run(c.name, func(t *testing.T) {
93+
actual := cmd.DiskArg{}
94+
err := actual.Set(c.input)
95+
if err != nil {
96+
if !c.expectError {
97+
t.Errorf("on input '%s':", c.input)
98+
t.Fatalf("unexpected error: %v", err)
99+
}
100+
return
101+
}
102+
103+
if c.expectError {
104+
t.Errorf("on input '%s':", c.input)
105+
t.Fatal("expected error, got nil")
106+
}
75107

76-
if !reflect.DeepEqual(actual, c.expect) {
77-
t.Errorf("on input '%s':a", c.input)
78-
t.Errorf("unexpected map contents")
79-
t.Errorf("expected: %+v", c.expect)
80-
t.Errorf("actual: %+v", actual)
81-
}
108+
if !reflect.DeepEqual(actual, c.expect) {
109+
t.Errorf("on input '%s'", c.input)
110+
t.Errorf("unexpected arg contents")
111+
t.Errorf("expected: %+v", c.expect)
112+
t.Errorf("actual: %+v", actual)
113+
}
114+
})
82115
}
83116
}

0 commit comments

Comments
 (0)