refactor(network): use cobra to validate required flags

Author: kangasta

internal/commands/network/create.go

@@ -41,7 +41,7 @@ func (s *createCommand) InitCommand() {
 	fs.StringVar(&s.name, "name", s.name, "Names the network.")
 	fs.StringVar(&s.zone, "zone", s.zone, "The zone in which the network is configured.")
 	fs.StringVar(&s.router, "router", s.router, "Add this network to an existing router.")
-	//XXX: handle multiline flag doc (try nested flags)
+	// TODO: handle multiline flag doc (try nested flags)
 	fs.StringArrayVar(&s.networks, "ip-network", s.networks, "A network interface for the server, multiple can be declared.\n\n "+
 		"Fields: \n"+
 		"  address: string \n"+
@@ -50,7 +50,11 @@ func (s *createCommand) InitCommand() {
 		"  dhcp: true/false \n"+
 		"  dhcp-default-route: true/false \n"+
 		"  dhcp-dns: array of strings")
+
 	s.AddFlags(fs)
+	s.Cobra().MarkFlagRequired("name")       //nolint:errcheck
+	s.Cobra().MarkFlagRequired("zone")       //nolint:errcheck
+	s.Cobra().MarkFlagRequired("ip-network") //nolint:errcheck
 }
 
 // MaximumExecutions implements Command.MaximumExecutions
@@ -89,16 +93,6 @@ func (s *createCommand) buildRequest() (*request.CreateNetworkRequest, error) {
 
 // ExecuteWithoutArguments implements commands.NoArgumentCommand
 func (s *createCommand) ExecuteWithoutArguments(exec commands.Executor) (output.Output, error) {
-	// TODO: should we, for example, accept name as the first argument instead of as a flag?
-	if s.name == "" {
-		return nil, fmt.Errorf("name is required")
-	}
-	if s.zone == "" {
-		return nil, fmt.Errorf("zone is required")
-	}
-	if len(s.networks) == 0 {
-		return nil, fmt.Errorf("at least one IP network is required")
-	}
 	svc := exec.Network()
 
 	msg := fmt.Sprintf("Creating network %v", s.name)

internal/commands/network/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"
 )
 
@@ -32,17 +32,17 @@ func TestCreateCommand(t *testing.T) {
 		{
 			name:  "name is missing",
 			args:  []string{"--zone", n.Zone},
-			error: "name is required",
+			error: `required flag(s) "name", "ip-network" not set`,
 		},
 		{
 			name:  "zone is missing",
 			args:  []string{"--name", n.Name},
-			error: "zone is required",
+			error: `required flag(s) "zone", "ip-network" not set`,
 		},
 		{
 			name:  "without network",
 			args:  []string{"--name", n.Name, "--zone", n.Zone},
-			error: "at least one IP network is required",
+			error: `required flag(s) "ip-network" not set`,
 		},
 		{
 			name: "with single network",
@@ -96,15 +96,16 @@ func TestCreateCommand(t *testing.T) {
 			mService := smock.Service{}
 			mService.On(targetMethod, &test.expected).Return(&upcloud.Network{}, nil)
 			conf := config.New()
+
 			c := commands.BuildCommand(CreateCommand(), nil, conf)
-			err := c.Cobra().Flags().Parse(test.args)
-			assert.NoError(t, err)
 
-			_, err = c.(commands.NoArgumentCommand).ExecuteWithoutArguments(commands.NewExecutor(conf, &mService, flume.New("test")))
+			c.Cobra().SetArgs(test.args)
+			_, err := mockexecute.MockExecute(c, &mService, conf)
 
-			if err != nil {
-				assert.Equal(t, test.error, err.Error())
+			if test.error != "" {
+				assert.EqualError(t, err, test.error)
 			} else {
+				assert.NoError(t, err)
 				mService.AssertNumberOfCalls(t, targetMethod, 1)
 			}
 		})

internal/commands/network/show_test.go

@@ -1,19 +1,17 @@
 package network
 
 import (
-	"bytes"
 	"testing"
 
 	"github.com/jedib0t/go-pretty/v6/text"
 
 	"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/output"
+	"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"
 )
 
@@ -114,22 +112,17 @@ func TestShowCommand(t *testing.T) {
 	}
 
 	conf := config.New()
-	// force human output
-	conf.Viper().Set(config.KeyOutput, config.ValueOutputHuman)
-
 	command := commands.BuildCommand(ShowCommand(), nil, conf)
 
 	// get resolver to initialize command cache
 	_, err := command.(*showCommand).Get(&mService)
 	if err != nil {
 		t.Fatal(err)
 	}
-	res, err := command.(commands.MultipleArgumentCommand).Execute(commands.NewExecutor(conf, &mService, flume.New("test")), network.UUID)
 
-	assert.Nil(t, err)
+	command.Cobra().SetArgs([]string{network.UUID})
+	output, err := mockexecute.MockExecute(command, &mService, conf)
 
-	buf := bytes.NewBuffer(nil)
-	err = output.Render(buf, conf, res)
 	assert.NoError(t, err)
-	assert.Equal(t, expected, buf.String())
+	assert.Equal(t, expected, output)
 }