Merge branch 'flags-refactor-2' into 'develop'

Put all the flag types into one place

See merge request open-source/bytemark-client!301

.gitlab-ci.yml

@@ -1,7 +1,7 @@
 variables:
   GO15VENDOREXPERIMENT: "1"
   GOPATH: /go
-  GOVERSION: "1.8"
+  GOVERSION: "1.11"
 
 stages:
   - test

HACKING

@@ -89,8 +89,11 @@ Here's a tree of the folders in this repo with descriptions of what each is for
 │       │   │          arguments into flags
 │       │   ├── auth - Authentication routine for the client, called via
 │       │   │          with.EnsureAuth
-│       │   ├── flagsets - common flags & code to support reading complex objects
-│       │   │              from them. e.g. VirtualMachineSpec & ImageInstall
+│       │   ├── flags - new flag types that are used in the client (e.g.
+│       │   │           VirtualMachineName, SizeSpec, ResizeSpec, AccountName,
+│       │   │           Privilege)
+│       │   ├── flagsets - common sets of flags & code to support reading complex
+│       │   │              objects from them. e.g. VirtualMachineSpec & ImageInstall
 │       │   ├── wait - functions which wait for a condition to be true before
 │       │   │          returning
 │       │   └── with - functions for chaining together using app.Action to
@@ -225,4 +228,4 @@ To generate the hash, use the following command.
 
   find . -type d \! -path './.*' \! -path './vendor/*' | sort | shasum -a 256
 
-# sha256sum: 4c172559ebbe8e0e565de3dd72236d56bb85bbed9271249804b22dcef3a006d1
+# sha256sum: 2a2e8078c018b791fb8f4749ae2a3afec81a6eacec6caf36cfaa094ede7d82c5

cmd/bytemark/add.go

@@ -5,16 +5,15 @@ import (
 
 	"github.com/BytemarkHosting/bytemark-client/cmd/bytemark/app"
 	"github.com/BytemarkHosting/bytemark-client/cmd/bytemark/app/args"
+	"github.com/BytemarkHosting/bytemark-client/cmd/bytemark/app/flags"
 	"github.com/BytemarkHosting/bytemark-client/cmd/bytemark/app/with"
-	"github.com/BytemarkHosting/bytemark-client/cmd/bytemark/util"
 	"github.com/BytemarkHosting/bytemark-client/lib/brain"
 	brainRequests "github.com/BytemarkHosting/bytemark-client/lib/requests/brain"
 	"github.com/BytemarkHosting/bytemark-client/util/log"
 	"github.com/urfave/cli"
 )
 
 func init() {
-	publicKeyFile := util.FileFlag{}
 	commands = append(commands, cli.Command{
 		Name:        "add",
 		Usage:       "add SSH keys to a user / IPs to a server",
@@ -38,26 +37,28 @@ func init() {
 				cli.GenericFlag{
 					Name:  "public-key-file",
 					Usage: "The public key file to add to the account",
-					Value: &publicKeyFile,
+					Value: &flags.FileFlag{},
 				},
 			},
 			Action: app.Action(args.Join("public-key"), with.Auth, func(ctx *app.Context) (err error) {
 				user := ctx.String("user")
 				if user == "" {
 					user = ctx.Config().GetIgnoreErr("user")
 				}
+				publicKeyFileContents := flags.FileContents(ctx, "public-key-file")
 
 				key := strings.TrimSpace(ctx.String("public-key"))
 				if key == "" {
-					if publicKeyFile.Value == "" {
+
+					if publicKeyFileContents == "" {
 						return ctx.Help("Please specify a key")
 					}
-					key = publicKeyFile.Value
+					key = publicKeyFileContents
 				} else {
 					// if public-key is not blank, try to use it as a filename
 					// FileFlag does some nice ~-substitution which is why we use it rather than the infinitely more normal-looking ioutil.ReadFile
-					publicKeyFile = util.FileFlag{FileName: key}
-					if err := publicKeyFile.Set(key); err == nil {
+					publicKeyFile := flags.FileFlag{FileName: key}
+					if err = publicKeyFile.Set(key); err == nil {
 						key = publicKeyFile.Value
 					}
 				}
@@ -99,7 +100,7 @@ func init() {
 				cli.GenericFlag{
 					Name:  "server",
 					Usage: "The server to add IPs to",
-					Value: new(app.VirtualMachineNameFlag),
+					Value: new(flags.VirtualMachineNameFlag),
 				},
 			},
 			Action: app.Action(args.Optional("server"), with.RequiredFlags("server"), with.Auth, func(c *app.Context) error {
@@ -128,7 +129,7 @@ func init() {
 					Reason:     reason,
 					Contiguous: c.Bool("contiguous"),
 				}
-				vmName := c.VirtualMachineName("server")
+				vmName := flags.VirtualMachineName(c, "server")
 				ips, err := c.Client().AddIP(vmName, ipcr)
 				if err != nil {
 					return err

cmd/bytemark/add_test.go

@@ -25,6 +25,11 @@ func TestAddKeyCommand(t *testing.T) {
 	}
 
 	t.Run("Key in command line", func(t *testing.T) {
+		defer func() {
+			if err := recover(); err != nil {
+				t.Error(err)
+			}
+		}()
 		_, c, app := testutil.BaseTestAuthSetup(t, false, commands)
 		c.When("GetUser", "test-user").Return(brain.User{Username: "test-user"}).Times(1)
 		c.MockRequest = &mocks.Request{
@@ -40,12 +45,17 @@ func TestAddKeyCommand(t *testing.T) {
 			Username:       "test-user",
 			AuthorizedKeys: brain.Keys{brain.Key{Key: "ssh-rsa aaaaawhartevervAsde fake key"}},
 		})
-		if ok, err := c.Verify(); !ok {
-			t.Fatal(err)
+		if ok, vErr := c.Verify(); !ok {
+			t.Fatal(vErr)
 		}
 
 	})
 	t.Run("Key in file", func(t *testing.T) {
+		defer func() {
+			if err := recover(); err != nil {
+				t.Error(err)
+			}
+		}()
 		_, c, app := testutil.BaseTestAuthSetup(t, false, commands)
 		c.When("GetUser", "test-user").Return(brain.User{Username: "test-user"}).Times(1)
 		c.MockRequest = &mocks.Request{
@@ -61,13 +71,18 @@ func TestAddKeyCommand(t *testing.T) {
 			Username:       "test-user",
 			AuthorizedKeys: brain.Keys{brain.Key{Key: "ssh-rsa aaaaawhartevervAsde fake key"}},
 		})
-		if ok, err := c.Verify(); !ok {
-			t.Fatal(err)
+		if ok, vErr := c.Verify(); !ok {
+			t.Fatal(vErr)
 		}
 
 	})
 
 	t.Run("Key in file using flag", func(t *testing.T) {
+		defer func() {
+			if err := recover(); err != nil {
+				t.Error(err)
+			}
+		}()
 		_, c, app := testutil.BaseTestAuthSetup(t, false, commands)
 		c.When("GetUser", "test-user").Return(brain.User{Username: "test-user"}).Times(1)
 		c.MockRequest = &mocks.Request{
@@ -82,13 +97,18 @@ func TestAddKeyCommand(t *testing.T) {
 			Username:       "test-user",
 			AuthorizedKeys: brain.Keys{brain.Key{Key: "ssh-rsa aaaaawhartevervAsde fake key"}},
 		})
-		if ok, err := c.Verify(); !ok {
-			t.Fatal(err)
+		if ok, vErr := c.Verify(); !ok {
+			t.Fatal(vErr)
 		}
 
 	})
 
 	t.Run("dont allow private key", func(t *testing.T) {
+		defer func() {
+			if err := recover(); err != nil {
+				t.Error(err)
+			}
+		}()
 		_, c, app := testutil.BaseTestAuthSetup(t, false, commands)
 		err = app.Run([]string{"bytemark", "add", "key", "--user", "test-user", "--public-key-file", "testkey"})
 		if err == nil {

cmd/bytemark/app/auth/authenticator_test.go

@@ -1220,8 +1220,8 @@ func TestAuthenticate(t *testing.T) {
 			err := Authenticator{
 				client:       client,
 				config:       config,
-				prompter:     prompter,
-				passPrompter: prompter,
+				prompter:     &prompter,
+				passPrompter: &prompter,
 			}.Authenticate()
 			if test.expectingError && err == nil {
 				t.Error("Expecting Authenticate to error, but it didn't")

cmd/bytemark/app/base_test.go

@@ -14,7 +14,7 @@ func baseTestSetup(t *testing.T, admin bool, commands []cli.Command) (conf *mock
 	conf = new(mocks.Config)
 	client = new(mocks.Client)
 	conf.When("GetBool", "admin").Return(admin, nil)
-	conf.When("GetV", "output-format").Return(config.Var{"output-format", "human", "CODE"})
+	conf.When("GetV", "output-format").Return(config.Var{Name: "output-format", Value: "human", Source: "CODE"})
 
 	app, err := BaseAppSetup(GlobalFlags(), commands)
 	if err != nil {

cmd/bytemark/app/context.go

@@ -2,7 +2,6 @@ package app
 
 import (
 	"io"
-	"net"
 
 	"github.com/BytemarkHosting/bytemark-client/cmd/bytemark/config"
 	"github.com/BytemarkHosting/bytemark-client/cmd/bytemark/util"
@@ -120,53 +119,14 @@ func (c *Context) IsSet(flagName string) bool {
 	return c.Context.IsSet(flagName)
 }
 
-// flags below
+// Flags provided by urfave/cli below.
+// For all the flags in the flags package, see flags/accessors.go
 
 // Bool returns the value of the named flag as a bool
 func (c *Context) Bool(flagname string) bool {
 	return c.Context.Bool(flagname)
 }
 
-// Discs returns the discs passed along as the named flag.
-// I can't imagine why I'd ever name a disc flag anything other than --disc, but the flexibility is there just in case.
-func (c *Context) Discs(flagname string) []brain.Disc {
-	disc, ok := c.Context.Generic(flagname).(*util.DiscSpecFlag)
-	if ok {
-		return []brain.Disc(*disc)
-	}
-	return []brain.Disc{}
-}
-
-// FileName returns the name of the file given by the named flag
-func (c *Context) FileName(flagname string) string {
-	file, ok := c.Context.Generic(flagname).(*util.FileFlag)
-	if ok {
-		return file.FileName
-	}
-	return ""
-}
-
-// FileContents returns the contents of the file given by the named flag.
-func (c *Context) FileContents(flagname string) string {
-	file, ok := c.Context.Generic(flagname).(*util.FileFlag)
-	if ok {
-		return file.Value
-	}
-	return ""
-}
-
-// GroupName returns the named flag as a lib.GroupName
-func (c *Context) GroupName(flagname string) (gp lib.GroupName) {
-	gpNameFlag, ok := c.Context.Generic(flagname).(*GroupNameFlag)
-	if !ok {
-		return lib.GroupName{}
-	}
-	if gpNameFlag.GroupName == nil {
-		return lib.GroupName{}
-	}
-	return *gpNameFlag.GroupName
-}
-
 // Int returns the value of the named flag as an int
 func (c *Context) Int(flagname string) int {
 	return c.Context.Int(flagname)
@@ -177,24 +137,6 @@ func (c *Context) Int64(flagname string) int64 {
 	return c.Context.Int64(flagname)
 }
 
-// IPs returns the ips passed along as the named flag.
-func (c *Context) IPs(flagname string) []net.IP {
-	ips, ok := c.Context.Generic(flagname).(*util.IPFlag)
-	if ok {
-		return []net.IP(*ips)
-	}
-	return []net.IP{}
-}
-
-// PrivilegeFlag returns the named flag as a PrivilegeFlag
-func (c *Context) PrivilegeFlag(flagname string) PrivilegeFlag {
-	priv, ok := c.Context.Generic(flagname).(*PrivilegeFlag)
-	if ok {
-		return *priv
-	}
-	return PrivilegeFlag{}
-}
-
 // String returns the value of the named flag as a string
 func (c *Context) String(flagname string) string {
 	if c.Context.IsSet(flagname) || c.Context.String(flagname) != "" {
@@ -211,34 +153,3 @@ func (c *Context) StringSlice(flagname string) []string {
 	}
 	return c.Context.GlobalStringSlice(flagname)
 }
-
-// Size returns the value of the named SizeSpecFlag as an int in megabytes
-func (c *Context) Size(flagname string) int {
-	size, ok := c.Context.Generic(flagname).(*util.SizeSpecFlag)
-	if ok {
-		return int(*size)
-	}
-	return 0
-}
-
-// ResizeFlag returns the named ResizeFlag
-func (c *Context) ResizeFlag(flagname string) ResizeFlag {
-	size, ok := c.Context.Generic(flagname).(*ResizeFlag)
-	if ok {
-		return *size
-	}
-	return ResizeFlag{}
-}
-
-// VirtualMachineName returns the named flag as a lib.VirtualMachineName
-func (c *Context) VirtualMachineName(flagname string) (vm lib.VirtualMachineName) {
-	vmNameFlag, ok := c.Context.Generic(flagname).(*VirtualMachineNameFlag)
-	if !ok {
-		return c.Config().GetVirtualMachine()
-	}
-	if vmNameFlag.VirtualMachineName == nil {
-		return lib.VirtualMachineName{}
-	}
-
-	return *vmNameFlag.VirtualMachineName
-}

cmd/bytemark/app/context_test.go

@@ -25,7 +25,7 @@ func TestOutput(t *testing.T) {
 	}{
 		{ // 0
 			// default to human output
-			ConfigFormat: config.Var{"output-format", "human", "CODE"},
+			ConfigFormat: config.Var{Name: "output-format", Value: "human", Source: "CODE"},
 			Object: brain.Disc{
 				Label:        "disk-1",
 				StorageGrade: "sata",
@@ -35,13 +35,13 @@ func TestOutput(t *testing.T) {
 			Expected: "disk-1 - 25GiB, sata grade\n",
 		}, { // 1
 			// default to human output with an error
-			ConfigFormat: config.Var{"output-format", "human", "CODE"},
+			ConfigFormat: config.Var{Name: "output-format", Value: "human", Source: "CODE"},
 			Object:       nil,
 			Expected:     "",
 			ShouldErr:    true,
 		}, { // 2
 			// when there's a default format specific to the command, use that instead of the uber-default
-			ConfigFormat:  config.Var{"output-format", "human", "CODE"},
+			ConfigFormat:  config.Var{Name: "output-format", Value: "human", Source: "CODE"},
 			DefaultFormat: []output.Format{output.Table},
 			Object: brain.Disc{
 				StorageGrade: "sata",
@@ -52,7 +52,7 @@ func TestOutput(t *testing.T) {
 			Expected:    "+-----+\n| ID  |\n+-----+\n| 123 |\n+-----+\n",
 		}, { // 3
 			// except when the JSON flag is set, then output JSON
-			ConfigFormat:  config.Var{"output-format", "human", "CODE"},
+			ConfigFormat:  config.Var{Name: "output-format", Value: "human", Source: "CODE"},
 			DefaultFormat: []output.Format{output.Table},
 			JSONFlag:      true,
 			Object: brain.Group{
@@ -62,7 +62,7 @@ func TestOutput(t *testing.T) {
 			Expected: "{\n    \"name\": \"my-cool-group\",\n    \"account_id\": 0,\n    \"id\": 11323,\n    \"virtual_machines\": null\n}\n",
 		}, { // 4
 			// or if output-format is set by a FILE
-			ConfigFormat:  config.Var{"output-format", "json", "FILE"},
+			ConfigFormat:  config.Var{Name: "output-format", Value: "json", Source: "FILE"},
 			DefaultFormat: []output.Format{output.Table},
 			Object: brain.Group{
 				Name: "my-cool-group",
@@ -71,7 +71,7 @@ func TestOutput(t *testing.T) {
 			Expected: "{\n    \"name\": \"my-cool-group\",\n    \"account_id\": 0,\n    \"id\": 11323,\n    \"virtual_machines\": null\n}\n",
 			// but the table and json flags should have precedence in every situation
 		}, { // 5
-			ConfigFormat:  config.Var{"output-format", "json", "FILE"},
+			ConfigFormat:  config.Var{Name: "output-format", Value: "json", Source: "FILE"},
 			DefaultFormat: []output.Format{output.Human},
 			TableFlag:     true,
 			Object: brain.Group{
@@ -81,7 +81,7 @@ func TestOutput(t *testing.T) {
 			Expected: "+---------------+-----------------+\n|     Name      | VirtualMachines |\n+---------------+-----------------+\n| my-cool-group |                 |\n+---------------+-----------------+\n",
 			// also, --table-fields being non-empty should imply --table and be case insensitive
 		}, { // 6
-			ConfigFormat:  config.Var{"output-format", "json", "FILE"},
+			ConfigFormat:  config.Var{Name: "output-format", Value: "json", Source: "FILE"},
 			DefaultFormat: []output.Format{output.Human},
 			TableFlag:     false,
 			TableFields:   "AccountID,ID,Name,VirtualMachines",