refactor(server): use cobra to validate required flags in server network-interface

Author: kangasta

internal/commands/networkinterface/delete.go

@@ -1,7 +1,6 @@
 package networkinterface
 
 import (
-	"errors"
 	"fmt"
 
 	"github.com/UpCloudLtd/upcloud-cli/internal/commands"
@@ -35,7 +34,9 @@ func DeleteCommand() commands.Command {
 func (s *deleteCommand) InitCommand() {
 	fs := &pflag.FlagSet{}
 	fs.IntVar(&s.interfaceIndex, "index", 0, "Interface index.")
+
 	s.AddFlags(fs)
+	s.Cobra().MarkFlagRequired("index") //nolint:errcheck
 }
 
 // MaximumExecutions implements Command.MaximumExecutions
@@ -45,12 +46,6 @@ func (s *deleteCommand) MaximumExecutions() int {
 
 // ExecuteSingleArgument implements commands.SingleArgumentCommand
 func (s *deleteCommand) ExecuteSingleArgument(exec commands.Executor, arg string) (output.Output, error) {
-	if s.interfaceIndex == 0 {
-		return nil, fmt.Errorf("interface index is required")
-	}
-	if arg == "" {
-		return nil, errors.New("single server uuid is required")
-	}
 	msg := fmt.Sprintf("Deleting network interface %d of server %s", s.interfaceIndex, arg)
 	logline := exec.NewLogEntry(msg)
 	logline.StartedNow()

internal/commands/networkinterface/delete_test.go

@@ -6,10 +6,10 @@ import (
 	"github.com/UpCloudLtd/upcloud-cli/internal/commands"
 	"github.com/UpCloudLtd/upcloud-cli/internal/config"
 	smock "github.com/UpCloudLtd/upcloud-cli/internal/mock"
+	"github.com/UpCloudLtd/upcloud-cli/internal/mockexecute"
 
 	"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud"
 	"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud/request"
-	"github.com/gemalto/flume"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -32,15 +32,10 @@ func TestDeleteCommand(t *testing.T) {
 			flags: []string{"--index", "4"},
 			req:   request.DeleteNetworkInterfaceRequest{ServerUUID: server.UUID, Index: 4},
 		},
-		{
-			name:  "server is missing",
-			flags: []string{"--index", "4"},
-			error: "single server uuid is required",
-		},
 		{
 			name:  "index is missing",
 			arg:   server.UUID,
-			error: "interface index is required",
+			error: `required flag(s) "index" not set`,
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
@@ -51,14 +46,14 @@ func TestDeleteCommand(t *testing.T) {
 			conf := config.New()
 
 			c := commands.BuildCommand(DeleteCommand(), nil, conf)
-			err := c.Cobra().Flags().Parse(test.flags)
-			assert.NoError(t, err)
 
-			_, err = c.(commands.SingleArgumentCommand).ExecuteSingleArgument(commands.NewExecutor(conf, &mService, flume.New("test")), test.arg)
+			c.Cobra().SetArgs(append(test.flags, test.arg))
+			_, err := mockexecute.MockExecute(c, &mService, conf)
 
 			if test.error != "" {
 				assert.EqualError(t, err, test.error)
 			} else {
+				assert.NoError(t, err)
 				mService.AssertNumberOfCalls(t, targetMethod, 1)
 			}
 		})

internal/commands/networkinterface/modify.go

@@ -44,7 +44,9 @@ func (s *modifyCommand) InitCommand() {
 	fs.StringVar(&s.bootable, "bootable", s.bootable, "Whether to try booting through the interface.")
 	fs.StringVar(&s.sourceIPfiltering, "source-ip-filtering", s.sourceIPfiltering, "Whether source IP filtering is enabled on the interface. Disabling it is allowed only for SDN private interfaces.")
 	fs.StringSliceVar(&s.ipAddresses, "ip-addresses", s.ipAddresses, "A comma-separated list of IP addresses, multiple can be declared\nUsage: --ip-address address=94.237.112.143,family=IPv4")
-	s.AddFlags(fs) // TODO(ana): replace usage with examples once the refactor is done.
+
+	s.AddFlags(fs)                      // TODO(ana): replace usage with examples once the refactor is done.
+	s.Cobra().MarkFlagRequired("index") //nolint:errcheck
 }
 
 // MaximumExecutions implements Command.MaximumExecutions
@@ -54,9 +56,6 @@ func (s *modifyCommand) MaximumExecutions() int {
 
 // ExecuteSingleArgument implements commands.SingleArgumentCommand
 func (s *modifyCommand) ExecuteSingleArgument(exec commands.Executor, arg string) (output.Output, error) {
-	if s.currentIndex == 0 {
-		return nil, fmt.Errorf("index is required")
-	}
 	ipAddresses, err := mapIPAddressesToRequest(s.ipAddresses)
 	if err != nil {
 		return nil, err

internal/commands/networkinterface/modify_test.go

@@ -6,10 +6,10 @@ import (
 	"github.com/UpCloudLtd/upcloud-cli/internal/commands"
 	"github.com/UpCloudLtd/upcloud-cli/internal/config"
 	smock "github.com/UpCloudLtd/upcloud-cli/internal/mock"
+	"github.com/UpCloudLtd/upcloud-cli/internal/mockexecute"
 
 	"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud"
 	"github.com/UpCloudLtd/upcloud-go-api/v4/upcloud/request"
-	"github.com/gemalto/flume"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -30,7 +30,7 @@ func TestModifyCommand(t *testing.T) {
 		{
 			name:  "index is missing",
 			flags: []string{},
-			error: "index is required",
+			error: `required flag(s) "index" not set`,
 		},
 		{
 			name: "index is present, using default values",
@@ -71,14 +71,14 @@ func TestModifyCommand(t *testing.T) {
 			conf := config.New()
 
 			c := commands.BuildCommand(ModifyCommand(), nil, conf)
-			err := c.Cobra().Flags().Parse(test.flags)
-			assert.NoError(t, err)
 
-			_, err = c.(commands.SingleArgumentCommand).ExecuteSingleArgument(commands.NewExecutor(conf, &mService, flume.New("test")), server.UUID)
+			c.Cobra().SetArgs(append(test.flags, server.UUID))
+			_, err := mockexecute.MockExecute(c, &mService, conf)
 
 			if test.error != "" {
 				assert.EqualError(t, err, test.error)
 			} else {
+				assert.NoError(t, err)
 				mService.AssertNumberOfCalls(t, targetMethod, 1)
 			}
 		})

internal/commands/runcommand.go

@@ -38,7 +38,7 @@ func commandRunE(command Command, service internal.AllServices, config *config.C
 		cmdLogger.Debug("executing single argument", "arguments", args)
 		// make sure we have an argument
 		if len(args) != 1 || args[0] == "" {
-			return fmt.Errorf("exactly one argument is required")
+			return fmt.Errorf("exactly one positional argument is required")
 		}
 		results, err := execute(typedCommand, executor, args, 1, typedCommand.ExecuteSingleArgument)
 		if err != nil {
@@ -49,7 +49,7 @@ func commandRunE(command Command, service internal.AllServices, config *config.C
 		cmdLogger.Debug("executing multi argument", "arguments", args)
 		// make sure we have arguments
 		if len(args) < 1 {
-			return fmt.Errorf("at least one argument is required")
+			return fmt.Errorf("at least one positional argument is required")
 		}
 		results, err := execute(typedCommand, executor, args, typedCommand.MaximumExecutions(), typedCommand.Execute)
 		if err != nil {

internal/commands/runcommand_test.go

@@ -145,7 +145,7 @@ func TestRunCommand(t *testing.T) {
 			name:             "single arg with no args",
 			args:             []string{},
 			command:          &mockSingle{Command: &cobra.Command{}},
-			expectedRunError: "exactly one argument is required",
+			expectedRunError: "exactly one positional argument is required",
 		},
 		{
 			name:           "multi args",
@@ -157,7 +157,7 @@ func TestRunCommand(t *testing.T) {
 			name:             "multiarg with no args",
 			args:             []string{},
 			command:          &mockMulti{Command: &cobra.Command{}},
-			expectedRunError: "at least one argument is required",
+			expectedRunError: "at least one positional argument is required",
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {