Skip to content

Commit ce8dfa3

Browse files
kangastakangasta
committed
refactor(storage): use cobra to validate required flags
1 parent 6aad179 commit ce8dfa3

12 files changed

Lines changed: 105 additions & 123 deletions

‎internal/commands/storage/backup_create.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,15 +48,13 @@ func (s *createBackupCommand) InitCommand() {
4848
flagSet.StringVar(&s.params.Title, "title", defaultCreateBackupParams.Title, "A short, informational description.")
4949

5050
s.AddFlags(flagSet)
51+
s.Cobra().MarkFlagRequired("title") //nolint:errcheck
5152
}
5253

5354
// Execute implements commands.MultipleArgumentCommand
5455
func (s *createBackupCommand) Execute(exec commands.Executor, uuid string) (output.Output, error) {
5556
svc := exec.Storage()
5657

57-
if s.params.Title == "" {
58-
return nil, fmt.Errorf("title is required")
59-
}
6058
msg := fmt.Sprintf("Backing up storage %v to %v", uuid, s.params.Title)
6159
logline := exec.NewLogEntry(msg)
6260
logline.StartedNow()

‎internal/commands/storage/backup_create_test.go‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@ 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
internal "github.com/UpCloudLtd/upcloud-cli/internal/service"
1011

1112
"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud"
1213
"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud/request"
13-
"github.com/gemalto/flume"
1414
"github.com/stretchr/testify/assert"
1515
"github.com/stretchr/testify/mock"
1616
)
@@ -49,7 +49,7 @@ func TestCreateBackupCommand(t *testing.T) {
4949
{
5050
name: "title is missing",
5151
args: []string{},
52-
error: "title is required",
52+
error: `required flag(s) "title" not set`,
5353
},
5454
{
5555
name: "title is provided",
@@ -67,14 +67,15 @@ func TestCreateBackupCommand(t *testing.T) {
6767
mService.On(targetMethod, mock.Anything).Return(&details, nil)
6868

6969
c := commands.BuildCommand(CreateBackupCommand(), nil, conf)
70-
err := c.Cobra().Flags().Parse(test.args)
71-
assert.NoError(t, err)
7270

73-
_, err = c.(commands.MultipleArgumentCommand).Execute(commands.NewExecutor(conf, mService, flume.New("test")), Storage2.UUID)
71+
c.Cobra().SetArgs(append(test.args, Storage2.UUID))
72+
_, err := mockexecute.MockExecute(c, mService, conf)
7473

7574
if test.error != "" {
76-
assert.Errorf(t, err, test.error)
75+
assert.Error(t, err)
76+
assert.Equal(t, test.error, err.Error())
7777
} else {
78+
assert.NoError(t, err)
7879
mService.AssertNumberOfCalls(t, targetMethod, 1)
7980
}
8081
})

‎internal/commands/storage/clone.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,14 +53,12 @@ func (s *cloneCommand) InitCommand() {
5353
flagSet.StringVar(&s.params.Zone, "zone", defaultCloneParams.Zone, "The zone in which the storage will be created, e.g. fi-hel1.")
5454

5555
s.AddFlags(flagSet)
56+
s.Cobra().MarkFlagRequired("title") //nolint:errcheck
57+
s.Cobra().MarkFlagRequired("zone") //nolint:errcheck
5658
}
5759

5860
// Execute implements commands.MultipleArgumentCommand
5961
func (s *cloneCommand) Execute(exec commands.Executor, uuid string) (output.Output, error) {
60-
if s.params.Zone == "" || s.params.Title == "" {
61-
return nil, fmt.Errorf("title and zone are required")
62-
}
63-
6462
svc := exec.Storage()
6563
req := s.params.CloneStorageRequest
6664
req.UUID = uuid

‎internal/commands/storage/clone_test.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@ 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
internal "github.com/UpCloudLtd/upcloud-cli/internal/service"
1011

1112
"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud"
1213
"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud/request"
13-
"github.com/gemalto/flume"
1414
"github.com/stretchr/testify/assert"
1515
"github.com/stretchr/testify/mock"
1616
)
@@ -72,14 +72,14 @@ func TestCloneCommand(t *testing.T) {
7272
args: []string{
7373
"--zone", "zone",
7474
},
75-
error: "title and zone are required",
75+
error: `required flag(s) "title" not set`,
7676
},
7777
{
7878
name: "zone is missing",
7979
args: []string{
8080
"--title", "title",
8181
},
82-
error: "title and zone are required",
82+
error: `required flag(s) "zone" not set`,
8383
},
8484
} {
8585
t.Run(test.name, func(t *testing.T) {
@@ -93,14 +93,15 @@ func TestCloneCommand(t *testing.T) {
9393
mService.On("GetStorages", mock.Anything).Return(&upcloud.Storages{Storages: []upcloud.Storage{Storage1, Storage2}}, nil)
9494

9595
c := commands.BuildCommand(CloneCommand(), nil, conf)
96-
err := c.Cobra().Flags().Parse(test.args)
97-
assert.NoError(t, err)
9896

99-
_, err = c.(commands.MultipleArgumentCommand).Execute(commands.NewExecutor(conf, mService, flume.New("test")), Storage2.UUID)
97+
c.Cobra().SetArgs(append(test.args, Storage2.UUID))
98+
_, err := mockexecute.MockExecute(c, mService, conf)
10099

101100
if test.error != "" {
102-
assert.Errorf(t, err, test.error)
101+
assert.Error(t, err)
102+
assert.Equal(t, test.error, err.Error())
103103
} else {
104+
assert.NoError(t, err)
104105
mService.AssertNumberOfCalls(t, targetMethod, 1)
105106
}
106107
})

‎internal/commands/storage/create.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,17 +83,16 @@ func (s *createCommand) InitCommand() {
8383
s.flagSet = &pflag.FlagSet{}
8484
s.params = newCreateParams()
8585
applyCreateFlags(s.flagSet, &s.params, defaultCreateParams)
86+
8687
s.AddFlags(s.flagSet)
88+
s.Cobra().MarkFlagRequired("title") //nolint:errcheck
89+
s.Cobra().MarkFlagRequired("zone") //nolint:errcheck
8790
}
8891

8992
// ExecuteWithoutArguments implements commands.NoArgumentCommand
9093
func (s *createCommand) ExecuteWithoutArguments(exec commands.Executor) (output.Output, error) {
9194
svc := exec.Storage()
9295

93-
if s.params.Size == 0 || s.params.Zone == "" || s.params.Title == "" {
94-
return nil, fmt.Errorf("size, title and zone are required")
95-
}
96-
9796
if err := s.params.processParams(); err != nil {
9897
return nil, err
9998
}

‎internal/commands/storage/create_test.go‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@ 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
internal "github.com/UpCloudLtd/upcloud-cli/internal/service"
1011

1112
"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud"
1213
"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud/request"
13-
"github.com/gemalto/flume"
1414
"github.com/stretchr/testify/assert"
1515
)
1616

@@ -92,15 +92,15 @@ func TestCreateCommand(t *testing.T) {
9292
"--size", "10",
9393
"--zone", "zone",
9494
},
95-
error: "size, title and zone are required",
95+
error: `required flag(s) "title" not set`,
9696
},
9797
{
9898
name: "zone is missing",
9999
args: []string{
100100
"--title", "title",
101101
"--size", "10",
102102
},
103-
error: "size, title and zone are required",
103+
error: `required flag(s) "zone" not set`,
104104
},
105105
} {
106106
t.Run(test.name, func(t *testing.T) {
@@ -112,13 +112,15 @@ func TestCreateCommand(t *testing.T) {
112112
mService.On(targetMethod, &test.expected).Return(&details, nil)
113113

114114
c := commands.BuildCommand(testCmd, nil, config.New())
115-
err := c.Cobra().Flags().Parse(test.args)
116-
assert.NoError(t, err)
117115

118-
_, err = c.(commands.NoArgumentCommand).ExecuteWithoutArguments(commands.NewExecutor(conf, mService, flume.New("test")))
116+
c.Cobra().SetArgs(test.args)
117+
_, err := mockexecute.MockExecute(c, mService, conf)
118+
119119
if test.error != "" {
120-
assert.Errorf(t, err, test.error)
120+
assert.Error(t, err)
121+
assert.Equal(t, test.error, err.Error())
121122
} else {
123+
assert.NoError(t, err)
122124
mService.AssertNumberOfCalls(t, targetMethod, 1)
123125
}
124126
})

‎internal/commands/storage/import.go‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,9 @@ func (s *importCommand) InitCommand() {
6969
flagSet.StringVar(&s.existingStorageUUIDOrName, "storage", "", "Import to an existing storage. Storage must be large enough and must be undetached or the server where the storage is attached must be in shutdown state.")
7070
config.AddToggleFlag(flagSet, &s.noWait, "no-wait", false, "Do not wait until the import finishes. Only applicable when importing from a remote URL.")
7171
applyCreateFlags(flagSet, &s.createParams, defaultCreateParams)
72+
7273
s.AddFlags(flagSet)
74+
s.Cobra().MarkFlagRequired("source-location") //nolint:errcheck
7375
}
7476

7577
type storageImportStatus struct {
@@ -83,11 +85,6 @@ type storageImportStatus struct {
8385

8486
// ExecuteWithoutArguments implements commands.NoArgumentCommand
8587
func (s *importCommand) ExecuteWithoutArguments(exec commands.Executor) (output.Output, error) {
86-
// initial argument validation
87-
if s.sourceLocation == "" {
88-
return nil, fmt.Errorf("source-location required")
89-
}
90-
9188
if s.existingStorageUUIDOrName == "" {
9289
if s.createParams.Zone == "" || s.createParams.Title == "" {
9390
return nil, fmt.Errorf("either existing storage or zone and title for a new storage to be created required")

‎internal/commands/storage/import_test.go‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,11 @@ import (
1313
"github.com/UpCloudLtd/upcloud-cli/internal/commands"
1414
"github.com/UpCloudLtd/upcloud-cli/internal/config"
1515
smock "github.com/UpCloudLtd/upcloud-cli/internal/mock"
16+
"github.com/UpCloudLtd/upcloud-cli/internal/mockexecute"
1617
internal "github.com/UpCloudLtd/upcloud-cli/internal/service"
1718

1819
"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud"
1920
"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud/request"
20-
"github.com/gemalto/flume"
2121
"github.com/stretchr/testify/assert"
2222
"github.com/stretchr/testify/mock"
2323
)
@@ -68,6 +68,11 @@ func TestImportCommand(t *testing.T) {
6868
State: upcloud.StorageImportStateCompleted,
6969
}
7070

71+
fileNotFoundErr := "cannot get file size: stat testfile: no such file or directory"
72+
if runtime.GOOS == "windows" {
73+
fileNotFoundErr = "cannot get file size: CreateFile testfile: The system cannot find the file specified."
74+
}
75+
7176
for _, test := range []struct {
7277
name string
7378
args []string
@@ -95,7 +100,7 @@ func TestImportCommand(t *testing.T) {
95100
"--zone", "fi-hel1",
96101
"--title", "test-1",
97102
},
98-
error: "source-location required",
103+
error: `required flag(s) "source-location" not set`,
99104
},
100105
{
101106
name: "http import",
@@ -119,8 +124,7 @@ func TestImportCommand(t *testing.T) {
119124
"--zone", "fi-hel1",
120125
"--title", "test-2",
121126
},
122-
error: "cannot get file size: stat testfile: no such file or directory",
123-
windowsError: "cannot get file size: CreateFile testfile: The system cannot find the file specified.",
127+
error: fileNotFoundErr,
124128
},
125129
} {
126130
t.Run(test.name, func(t *testing.T) {
@@ -136,17 +140,12 @@ func TestImportCommand(t *testing.T) {
136140
mService.On("CreateStorage", mock.Anything).Return(&StorageDetails1, nil)
137141

138142
c := commands.BuildCommand(ImportCommand(), nil, conf)
139-
err := c.Cobra().Flags().Parse(test.args)
140-
assert.NoError(t, err)
141143

142-
_, err = c.(commands.NoArgumentCommand).ExecuteWithoutArguments(commands.NewExecutor(conf, mService, flume.New("test")))
144+
c.Cobra().SetArgs(test.args)
145+
_, err := mockexecute.MockExecute(c, mService, conf)
143146

144147
if test.error != "" {
145-
if test.windowsError != "" && runtime.GOOS == "windows" {
146-
assert.EqualError(t, err, test.windowsError)
147-
} else {
148-
assert.EqualError(t, err, test.error)
149-
}
148+
assert.EqualError(t, err, test.error)
150149
} else {
151150
mService.AssertNumberOfCalls(t, "CreateStorageImport", 1)
152151
mService.AssertNumberOfCalls(t, "GetStorageImportDetails", 1)

0 commit comments

Comments
 (0)