Skip to content

Commit e1a27b6

Browse files
kangastakangasta
committed
refactor(router): use cobra to validate required flags
1 parent 7c821cf commit e1a27b6

9 files changed

Lines changed: 31 additions & 43 deletions

File tree

‎internal/commands/router/create.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,9 @@ func CreateCommand() commands.Command {
3131
func (s *createCommand) InitCommand() {
3232
fs := &pflag.FlagSet{}
3333
fs.StringVar(&s.name, "name", s.name, "Router name.")
34+
3435
s.AddFlags(fs)
36+
s.Cobra().MarkFlagRequired("name") //nolint:errcheck
3537
}
3638

3739
// MaximumExecutions implements Command.MaximumExecutions
@@ -41,10 +43,6 @@ func (s *createCommand) MaximumExecutions() int {
4143

4244
// ExecuteWithoutArguments implements commands.NoArgumentCommand
4345
func (s *createCommand) ExecuteWithoutArguments(exec commands.Executor) (output.Output, error) {
44-
// TODO: should this be a regular argument?
45-
if s.name == "" {
46-
return nil, fmt.Errorf("name is required")
47-
}
4846
msg := fmt.Sprintf("Creating router %s", s.name)
4947
logline := exec.NewLogEntry(msg)
5048
logline.StartedNow()

‎internal/commands/router/create_test.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,10 @@ import (
66
"github.com/UpCloudLtd/upcloud-cli/internal/commands"
77
"github.com/UpCloudLtd/upcloud-cli/internal/config"
88
smock "github.com/UpCloudLtd/upcloud-cli/internal/mock"
9+
"github.com/UpCloudLtd/upcloud-cli/internal/mockexecute"
910

1011
"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud"
1112
"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud/request"
12-
"github.com/gemalto/flume"
1313
"github.com/stretchr/testify/assert"
1414
)
1515

@@ -27,7 +27,7 @@ func TestCreateCommand(t *testing.T) {
2727
{
2828
name: "name is missing",
2929
flags: []string{},
30-
error: "name is required",
30+
error: `required flag(s) "name" not set`,
3131
},
3232
{
3333
name: "name is passed",
@@ -42,14 +42,14 @@ func TestCreateCommand(t *testing.T) {
4242
conf := config.New()
4343

4444
c := commands.BuildCommand(CreateCommand(), nil, conf)
45-
err := c.Cobra().Flags().Parse(test.flags)
46-
assert.NoError(t, err)
4745

48-
_, err = c.(commands.NoArgumentCommand).ExecuteWithoutArguments(commands.NewExecutor(conf, &mService, flume.New("test")))
46+
c.Cobra().SetArgs(test.flags)
47+
_, err := mockexecute.MockExecute(c, &mService, conf)
4948

5049
if test.error != "" {
51-
assert.Errorf(t, err, test.error)
50+
assert.EqualError(t, err, test.error)
5251
} else {
52+
assert.NoError(t, err)
5353
mService.AssertNumberOfCalls(t, targetMethod, 1)
5454
}
5555
})

‎internal/commands/router/delete_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,9 @@ func TestDeleteCommand(t *testing.T) {
4646
_, err := c.(commands.MultipleArgumentCommand).Execute(commands.NewExecutor(conf, &mService, flume.New("test")), test.arg)
4747

4848
if test.error != "" {
49-
assert.Errorf(t, err, test.error)
49+
assert.EqualError(t, err, test.error)
5050
} else {
51+
assert.NoError(t, err)
5152
mService.AssertNumberOfCalls(t, targetMethod, 1)
5253
}
5354
})

‎internal/commands/router/modify.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,13 @@ func ModifyCommand() commands.Command {
3434
func (s *modifyCommand) InitCommand() {
3535
fs := &pflag.FlagSet{}
3636
fs.StringVar(&s.name, "name", "", "New router name.")
37+
3738
s.AddFlags(fs)
39+
s.Cobra().MarkFlagRequired("name") //nolint:errcheck
3840
}
3941

4042
// ExecuteSingleArgument implements commands.SingleArgumentCommand
4143
func (s *modifyCommand) ExecuteSingleArgument(exec commands.Executor, arg string) (output.Output, error) {
42-
if s.name == "" {
43-
return nil, fmt.Errorf("name is required")
44-
}
4544
msg := fmt.Sprintf("Modifying router %s", s.name)
4645
logline := exec.NewLogEntry(msg)
4746
logline.StartedNow()

‎internal/commands/router/modify_test.go‎

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,10 @@ import (
66
"github.com/UpCloudLtd/upcloud-cli/internal/commands"
77
"github.com/UpCloudLtd/upcloud-cli/internal/config"
88
smock "github.com/UpCloudLtd/upcloud-cli/internal/mock"
9+
"github.com/UpCloudLtd/upcloud-cli/internal/mockexecute"
910

1011
"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud"
1112
"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud/request"
12-
"github.com/gemalto/flume"
1313
"github.com/stretchr/testify/assert"
1414
"github.com/stretchr/testify/mock"
1515
)
@@ -20,29 +20,21 @@ func TestModifyCommand(t *testing.T) {
2020

2121
for _, test := range []struct {
2222
name string
23-
flags []string
24-
arg string
23+
args []string
2524
error string
2625
returns *upcloud.Router
2726
req request.ModifyRouterRequest
2827
}{
29-
{
30-
name: "arg is missing",
31-
flags: []string{},
32-
error: "router is required",
33-
},
3428
{
3529
name: "name is missing",
36-
arg: router.UUID,
37-
flags: []string{},
38-
error: "name is required",
30+
args: []string{router.UUID},
31+
error: `required flag(s) "name" not set`,
3932
},
4033
{
4134
name: "name is passed",
42-
arg: router.UUID,
43-
flags: []string{"--name", "router-2-b"},
35+
args: []string{"--name", "New name", router.UUID},
4436
returns: &modifiedRouter,
45-
req: request.ModifyRouterRequest{Name: "router-2-b", UUID: router.UUID},
37+
req: request.ModifyRouterRequest{Name: "New name", UUID: router.UUID},
4638
},
4739
} {
4840
targetMethod := "ModifyRouter"
@@ -54,14 +46,14 @@ func TestModifyCommand(t *testing.T) {
5446
conf := config.New()
5547

5648
c := commands.BuildCommand(ModifyCommand(), nil, conf)
57-
if err := c.Cobra().Flags().Parse(test.flags); err != nil {
58-
t.Fatal(err)
59-
}
60-
_, err := c.(commands.SingleArgumentCommand).ExecuteSingleArgument(commands.NewExecutor(conf, &mService, flume.New("test")), test.arg)
49+
50+
c.Cobra().SetArgs(test.args)
51+
_, err := mockexecute.MockExecute(c, &mService, conf)
6152

6253
if test.error != "" {
63-
assert.Errorf(t, err, test.error)
54+
assert.EqualError(t, err, test.error)
6455
} else {
56+
assert.NoError(t, err)
6557
mService.AssertNumberOfCalls(t, targetMethod, 1)
6658
}
6759
})

‎internal/commands/router/show_test.go‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,17 @@
11
package router
22

33
import (
4-
"bytes"
54
"testing"
65

76
"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud"
8-
"github.com/gemalto/flume"
97
"github.com/jedib0t/go-pretty/v6/text"
108
"github.com/stretchr/testify/assert"
119
"github.com/stretchr/testify/mock"
1210

1311
"github.com/UpCloudLtd/upcloud-cli/internal/commands"
1412
"github.com/UpCloudLtd/upcloud-cli/internal/config"
1513
smock "github.com/UpCloudLtd/upcloud-cli/internal/mock"
16-
"github.com/UpCloudLtd/upcloud-cli/internal/output"
14+
"github.com/UpCloudLtd/upcloud-cli/internal/mockexecute"
1715
"github.com/UpCloudLtd/upcloud-cli/internal/resolver"
1816
)
1917

@@ -82,11 +80,9 @@ func TestShowCommand(t *testing.T) {
8280
_, err := c.(resolver.ResolutionProvider).Get(&mService)
8381
assert.NoError(t, err)
8482

85-
res, err := c.(commands.MultipleArgumentCommand).Execute(commands.NewExecutor(conf, &mService, flume.New("test")), router.UUID)
86-
assert.NoError(t, err)
83+
c.Cobra().SetArgs([]string{router.UUID})
84+
output, err := mockexecute.MockExecute(c, &mService, conf)
8785

88-
buf := bytes.NewBuffer(nil)
89-
err = output.Render(buf, conf, res)
9086
assert.NoError(t, err)
91-
assert.Equal(t, expected, buf.String())
87+
assert.Equal(t, expected, output)
9288
}

‎internal/commands/runcommand.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ func commandRunE(command Command, service internal.AllServices, config *config.C
3838
cmdLogger.Debug("executing single argument", "arguments", args)
3939
// make sure we have an argument
4040
if len(args) != 1 || args[0] == "" {
41-
return fmt.Errorf("exactly 1 argument is required")
41+
return fmt.Errorf("exactly one argument is required")
4242
}
4343
results, err := execute(typedCommand, executor, args, 1, typedCommand.ExecuteSingleArgument)
4444
if err != nil {

‎internal/commands/runcommand_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ func TestRunCommand(t *testing.T) {
145145
name: "single arg with no args",
146146
args: []string{},
147147
command: &mockSingle{Command: &cobra.Command{}},
148-
expectedRunError: "exactly 1 argument is required",
148+
expectedRunError: "exactly one argument is required",
149149
},
150150
{
151151
name: "multi args",

‎internal/mockexecute/mockexecute.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,10 @@ func mockRunE(command commands.Command, service service.AllServices, config *con
3939
case commands.NoArgumentCommand:
4040
out, err = typedCommand.ExecuteWithoutArguments(executor)
4141
case commands.SingleArgumentCommand:
42+
// Panics if called with nil args
4243
out, err = typedCommand.ExecuteSingleArgument(executor, args[0])
4344
case commands.MultipleArgumentCommand:
45+
// Panics if called with nil args
4446
out, err = typedCommand.Execute(executor, args[0])
4547
}
4648
if err != nil {

0 commit comments

Comments
 (0)