refactor(server): use cobra to validate required flags in server firewall

Also adds helper to execute cobra command and capture its output and errors to
help, for example, checking that cobra validates given args correctly.

Author: kangasta

internal/commands/serverfirewall/create.go

@@ -76,38 +76,18 @@ Use --position parameter or create default rule after other rules.`
 	flagSet.StringVar(&s.comment, "comment", "", "Freeform comment that can include 0-250 characters.")
 
 	s.AddFlags(flagSet)
+	s.Cobra().MarkFlagRequired("direction") //nolint:errcheck
+	s.Cobra().MarkFlagRequired("action")    //nolint:errcheck
+	s.Cobra().MarkFlagsRequiredTogether("destination-port-start", "destination-port-end")
+	s.Cobra().MarkFlagsRequiredTogether("source-port-start", "source-port-end")
 }
 
 // Execute implements commands.MultipleArgumentCommand
 func (s *createCommand) Execute(exec commands.Executor, arg string) (output.Output, error) {
-	if s.direction == "" {
-		return nil, fmt.Errorf("direction is required")
-	}
-
-	if s.action == "" {
-		return nil, fmt.Errorf("action is required")
-	}
-
 	if s.family != "" && s.family != "IPv4" && s.family != "IPv6" {
 		return nil, fmt.Errorf("invalid family, use either IPv4 or IPv6")
 	}
 
-	if s.destinationPortStart == "" && s.destinationPortEnd != "" {
-		return nil, fmt.Errorf("destination-port-start is required if destination-port-end is set")
-	}
-
-	if s.destinationPortEnd == "" && s.destinationPortStart != "" {
-		return nil, fmt.Errorf("destination-port-end is required if destination-port-start is set")
-	}
-
-	if s.sourcePortStart == "" && s.sourcePortEnd != "" {
-		return nil, fmt.Errorf("source-port-start is required if source-port-end is set")
-	}
-
-	if s.sourcePortEnd == "" && s.sourcePortStart != "" {
-		return nil, fmt.Errorf("source-port-end is required if source-port-start is set")
-	}
-
 	var (
 		destinationNetwork      *cidr.ParsedCIDR
 		sourceNetwork           *cidr.ParsedCIDR

internal/commands/serverfirewall/create_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"
 	"github.com/stretchr/testify/mock"
 )
@@ -40,7 +40,7 @@ func TestCreateFirewallRuleCommand(t *testing.T) {
 			name:  "Empty info",
 			flags: []string{},
 			arg:   Server1.UUID,
-			error: "direction is required",
+			error: `required flag(s) "direction", "action" not set`,
 		},
 		{
 			name: "Action is required",
@@ -49,7 +49,7 @@ func TestCreateFirewallRuleCommand(t *testing.T) {
 				"--direction", "in",
 			},
 			arg:   Server1.UUID,
-			error: "action is required",
+			error: `required flag(s) "action" not set`,
 		},
 		{
 			name: "FirewallRule, drop incoming by default",
@@ -96,10 +96,10 @@ func TestCreateFirewallRuleCommand(t *testing.T) {
 
 			conf := config.New()
 			cc := commands.BuildCommand(CreateCommand(), nil, conf)
-			err := cc.Cobra().Flags().Parse(test.flags)
-			assert.NoError(t, err)
 
-			_, err = cc.(commands.MultipleArgumentCommand).Execute(commands.NewExecutor(conf, &mService, flume.New("test")), test.arg)
+			cc.Cobra().SetArgs(append(test.flags, test.arg))
+			_, err := mockexecute.MockExecute(cc, &mService, conf)
+
 			if test.error != "" {
 				assert.Error(t, err)
 				assert.Equal(t, test.error, err.Error())

internal/commands/serverfirewall/delete.go

@@ -34,13 +34,12 @@ func (s *deleteCommand) InitCommand() {
 	flagSet := &pflag.FlagSet{}
 	flagSet.IntVar(&s.rulePosition, "position", 0, "Rule position. Available: 1-1000")
 	s.AddFlags(flagSet)
+
+	s.Cobra().MarkFlagRequired("position") //nolint:errcheck
 }
 
 // Execute implements commands.MultipleArgumentCommand
 func (s *deleteCommand) Execute(exec commands.Executor, arg string) (output.Output, error) {
-	if s.rulePosition == 0 {
-		return nil, fmt.Errorf("position is required")
-	}
 	if s.rulePosition < 1 || s.rulePosition > 1000 {
 		return nil, fmt.Errorf("invalid position (1-1000 allowed)")
 	}

internal/commands/serverfirewall/delete_test.go

@@ -7,9 +7,9 @@ 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/gemalto/flume"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 )
@@ -37,7 +37,7 @@ func TestDeleteServerFirewallRuleCommand(t *testing.T) {
 		{
 			name:  "no position",
 			flags: []string{},
-			error: "position is required",
+			error: `required flag(s) "position" not set`,
 		},
 		{
 			name:  "position 1",
@@ -56,10 +56,10 @@ func TestDeleteServerFirewallRuleCommand(t *testing.T) {
 
 			conf := config.New()
 			cc := commands.BuildCommand(DeleteCommand(), nil, conf)
-			err := cc.Cobra().Flags().Parse(test.flags)
-			assert.NoError(t, err)
 
-			_, err = cc.(commands.MultipleArgumentCommand).Execute(commands.NewExecutor(conf, mService, flume.New("test")), Server1.UUID)
+			cc.Cobra().SetArgs(append(test.flags, Server1.UUID))
+			_, err := mockexecute.MockExecute(cc, mService, conf)
+
 			if test.error != "" {
 				fmt.Println("ERROR", test.error, "==", err)
 				assert.EqualError(t, err, test.error)

internal/mockexecute/mockexecute.go

@@ -0,0 +1,45 @@
+package mockexecute
+
+import (
+	"bytes"
+
+	"github.com/gemalto/flume"
+	"github.com/spf13/cobra"
+
+	"github.com/UpCloudLtd/upcloud-cli/internal/commands"
+	"github.com/UpCloudLtd/upcloud-cli/internal/config"
+	"github.com/UpCloudLtd/upcloud-cli/internal/output"
+	"github.com/UpCloudLtd/upcloud-cli/internal/service"
+)
+
+func MockExecute(command commands.Command, service service.AllServices, config *config.Config) (string, error) {
+	buf := bytes.NewBuffer(nil)
+	command.Cobra().SetErr(buf)
+	command.Cobra().SetOut(buf)
+
+	command.Cobra().RunE = func(_ *cobra.Command, args []string) error {
+		return mockRunE(command, service, config, args)
+	}
+	err := command.Cobra().Execute()
+
+	return buf.String(), err
+}
+
+func mockRunE(command commands.Command, service service.AllServices, config *config.Config, args []string) error {
+	executor := commands.NewExecutor(config, service, flume.New("test"))
+
+	var err error
+	var out output.Output
+	switch typedCommand := command.(type) {
+	case commands.NoArgumentCommand:
+		out, err = typedCommand.ExecuteWithoutArguments(executor)
+	case commands.SingleArgumentCommand:
+		out, err = typedCommand.ExecuteSingleArgument(executor, args[0])
+	case commands.MultipleArgumentCommand:
+		out, err = typedCommand.Execute(executor, args[0])
+	}
+
+	_ = output.Render(command.Cobra().OutOrStdout(), config, out)
+
+	return err
+}