Merge lp:~jtv/gwacl/storage-tool-command-line into lp:gwacl

[Jeroen T. Vermeulen]

Clean up command-line parsing in storage tool.

Actually I just wanted to add a "storage" option in order to fix bug 1213059: StorageContext.AzureEndpoint is now required. But along the way I found that this program wasn't very welcoming to change. Any new operation would have to be defined in too many places (help string for valid operations; switch for arguments checking; help output; switch for actual implementation) and the descriptions for each were hard to find. This branch defines all that in a single array of structs describing the available operations.

(Go being Go, of course there's still some distraction in the code from complex, sophisticated procedures such as "now give me just the names").

And while working on that, I got irritated by the hopeless error output: you'd get one line of error, and then it would be completely drowned out by the usage output. And then, even if you were just asking for the usage information, you'd get a message about exit status. (Also, Go's flags package doesn't add the conventional "Usage:" to the usage output). So I added a -h option (Go's flags package doesn't seem to welcome GNU-style long and short forms for the same option, so no --help).

Now, invoking the program with -h prints usage and exits successfully. Getting the options wrong prints just a one-line pointer to the -h option, the actual error, and the exit status.

By the way, the Usage function was written as a variable instead of a regular function definition — probably because we were all new to Go at the time, but possibly also because the idea was to invoke the function through the flags package. In practice I find that the flags package adds no value at all (it doesn't add that "Usage:"!) so I simplified that a bit.

In some languages, you might wonder if "operation" shouldn't be a base class with different implementation types, or in Go, whether it should be an interface with different implementing structs. But that just adds massive overhead, for no particularly good reason. Yes, each has its own implementations of one or two "methods" but in this case I think it's perfectly valid to see that as an attribute rather than a type difference.

Branch with the actual fix for bug 1213059 follows.

Jeroen

[lp:~jtv/gwacl/storage-tool-command-line]

220. By Jeroen T. Vermeulen on 2013-08-19

Put option variables in a better place, group them for clarity, and document what they are.

221. By Jeroen T. Vermeulen on 2013-08-19

Identify an internal error as such.

[Julian Edwards (julian-edwards)]

Thanks for doing this, it was a bit messy in there. I've not given it massive scrutiny, since it's just an example program. If it runs and works that's fine by me.

You have two init() functions. I don't know if that's allowed or not but either way it's darn confusing, please collapse into a single func!

review: Approve

[lp:~jtv/gwacl/storage-tool-command-line]

222. By Jeroen T. Vermeulen on 2013-08-19

Review change: combine init() functions.

223. By Jeroen T. Vermeulen on 2013-08-19

A change leaked between branches. I miss distributed revision control.

[Jeroen T. Vermeulen (jtv)]

Thanks. I combined the init() functions, although in the long run there's no avoiding the fact that they're a weird special case in the language and have their own semantics.

[Gavin Panella (allenap)]

Next time we have flag issues we could consider using go-flags <https://github.com/jessevdk/go-flags> or gnuflag <https://launchpad.net/gnuflag>. The latter is used in Juju, and was created by Roger Peppe. The Go standard library's flag package feels about as modern as pocket protectors and should be deprecated with fire.

=== modified file 'example/storage/run.go'
--- example/storage/run.go	2013-07-12 14:42:08 +0000
+++ example/storage/run.go	2013-08-19 05:20:32 +0000
@@ -15,33 +15,170 @@
     "io/ioutil"
     "launchpad.net/gwacl"
     "os"
+    "strings"
 )
 
-var account string
-var key string
-var filename string
-var container string
-var prefix string
-var blobname string
-var blobtype string
-var size int
-var pagerange string
-var acl string
-
-func checkContainerAndFilename() error {
-    if container == "" || filename == "" {
-        return fmt.Errorf("Must supply container and filename")
-    }
-    return nil
-}
-
-var VALID_CMDS string = "<listcontainers|containeracl|list|getblob|addblock|deleteblob|putblob|putpage>"
-
 func badOperationError() error {
-    return fmt.Errorf("Must specify one of %s", VALID_CMDS)
+    return fmt.Errorf("Must specify one of %v", operationNames)
+}
+
+// operation is something you can instruct this program to do, by specifying
+// its name on the command line.
+type operation struct {
+    // name is the operation name as used on the command line.
+    name string
+    // description holds a description of what the operation does.
+    description string
+    // example illustrates how the operation is used.
+    example string
+    // requiredArgs lists the command-line options that are required for this
+    // operation.
+    requiredArgs []string
+    // validate is an optional callback to perform more detailed checking on
+    // the operation's arguments.
+    validate func() error
+    // implementation is a function that performs the operation.  If it fails,
+    // it just panics.
+    implementation func(gwacl.StorageContext)
+}
+
+// operations defines what operations are available to be invoked from the
+// command line.
+var operations = []operation{
+    {
+        name:           "listcontainers",
+        description:    "Show existing storage containers",
+        example:        "listcontainers",
+        implementation: listcontainers,
+    },
+    {
+        name:           "list",
+        description:    "List files in a container",
+        example:        "-container=<container> list",
+        requiredArgs:   []string{"list"},
+        implementation: list,
+    },
+    {
+        name:         "containeracl",
+        description:  "Set access on a container",
+        example:      "-container=<container> -acl <container|blob|private> containeracl",
+        requiredArgs: []string{"container", "key", "acl"},
+        validate: func() error {
+            if acl != "container" && acl != "blob" && acl != "private" {
+                return fmt.Errorf("Usage: containeracl -container=<container> <container|blob|private>")
+            }
+            return nil
+        },
+        implementation: containeracl,
+    },
+    {
+        name:           "getblob",
+        description:    "Get a file from a container (it's returned on stdout)",
+        example:        "-container=<container> -filename=<filename> getblob",
+        requiredArgs:   []string{"container", "filename"},
+        implementation: getblob,
+    },
+    {
+        name:           "addblock",
+        description:    "Upload a file to a block blob",
+        example:        "-container=<container> -filename=<filename> addblock",
+        requiredArgs:   []string{"key", "container", "filename"},
+        implementation: addblock,
+    },
+    {
+        name:           "deleteblob",
+        description:    "Delete a blob",
+        example:        "-container=<container> -filename=<filename> deleteblob",
+        requiredArgs:   []string{"key", "container", "filename"},
+        implementation: deleteblob,
+    },
+    {
+        name:        "putblob",
+        description: "Create an empty page blob",
+        example: "-container=<container> -blobname=<blobname> -size=<bytes> " +
+            "-blobtype=\"page\" putblob",
+        requiredArgs:   []string{"key", "blobname", "blobtype", "container", "size"},
+        implementation: putblob,
+    },
+    {
+        name: "putpage",
+        description: "Upload a file to a page blob's page.  The range parameters must " +
+            "be (modulo 512)-(modulo 512 -1), eg: -pagerange=0-511",
+        example: "-container=<container> -blobname=<blobname> -pagerange=<N-N> " +
+            "-filename=<local file> putpage",
+        requiredArgs:   []string{"key", "blobname", "container", "pagerange", "filename"},
+        implementation: putpage,
+    },
+}
+
+// operationsByName maps each opeation name to an operation struct that
+// describes it.
+var operationsByName map[string]operation
+
+// operationNames lists just the names of the oeprations, in the order in which
+// they are listed in "operations."
+var operationNames []string
+
+func init() {
+    operationsByName = make(map[string]operation, len(operations))
+    for _, op := range operations {
+        operationsByName[op.name] = op
+    }
+
+    operationNames = make([]string, len(operations))
+    for index, op := range operations {
+        operationNames[index] = op.name
+    }
+}
+
+// Variables set by command-line options.
+var (
+    help      bool
+    account   string
+    location  string
+    key       string
+    filename  string
+    container string
+    prefix    string
+    blobname  string
+    blobtype  string
+    size      int
+    pagerange string
+    acl       string
+)
+
+// argumentGiven returns whether the named argument was specified on the
+// command line.
+func argumentGiven(name string) bool {
+    // This is stupid.  There must be a way to ask the flag module directly!
+    switch name {
+    case "account":
+        return account != ""
+    case "key":
+        return key != ""
+    case "container":
+        return container != ""
+    case "filename":
+        return filename != ""
+    case "prefix":
+        return prefix != ""
+    case "blobname":
+        return blobname != ""
+    case "blobtype":
+        return blobtype != ""
+    case "size":
+        return size != 0
+    case "pagerange":
+        return pagerange != ""
+    case "acl":
+        return acl != ""
+    }
+    panic(fmt.Errorf("internal error: unknown command-line option: %s", name))
 }
 
 func getParams() (string, error) {
+    flag.BoolVar(&help, "h", false, "Show usage and exit")
+
     flag.StringVar(&account, "account", "", "Storage account name")
     flag.StringVar(&key, "key", "", "A valid storage account key (base64 encoded), defaults to the empty string (i.e. anonymous access)")
     flag.StringVar(&container, "container", "", "Name of the container to use")
@@ -54,6 +191,15 @@
     flag.StringVar(&acl, "acl", "", "When using 'containeracl', specify an ACL type")
     flag.Parse()
 
+    if help {
+        return "", nil
+    }
+
+    opName := flag.Arg(0)
+    if opName == "" {
+        return "", fmt.Errorf("No operation specified")
+    }
+
     if account == "" {
         return "", fmt.Errorf("Must supply account parameter")
     }
@@ -62,71 +208,33 @@
         return "", badOperationError()
     }
 
-    switch flag.Arg(0) {
-    case "listcontainers":
-        return "listcontainers", nil
-    case "containeracl":
-        if container == "" {
-            return "", fmt.Errorf("Must supply container with containeracl")
-        }
-        if key == "" {
-            return "", fmt.Errorf("Must supply key with containeracl")
-        }
-        if acl != "container" && acl != "blob" && acl != "private" {
-            return "", fmt.Errorf("Usage: containeracl -container=<container> <container|blob|private>")
-        }
-        return "containeracl", nil
-    case "list":
-        if container == "" {
-            return "", fmt.Errorf("Must supply container with 'list'")
-        }
-        return "list", nil
-    case "getblob":
-        if container == "" || filename == "" {
-            return "", fmt.Errorf("Must supply container and filename with 'list'")
-        }
-        return "getblob", nil
-    case "addblock":
-        if key == "" {
-            return "", fmt.Errorf("Must supply key with addblock")
-        }
-        err := checkContainerAndFilename()
-        if err != nil {
-            return "", err
-        }
-        return "addblock", nil
-    case "deleteblob":
-        if key == "" {
-            return "", fmt.Errorf("Must supply key with deleteblob")
-        }
-        err := checkContainerAndFilename()
-        if err != nil {
-            return "", err
-        }
-        return "deleteblob", nil
-    case "putblob":
-        if key == "" {
-            return "", fmt.Errorf("Must supply key with putblob")
-        }
-        if blobname == "" || blobtype == "" || container == "" || size == 0 {
-            return "", fmt.Errorf("Must supply container, blobname, blobtype and size with 'putblob'")
-        }
-        return "putblob", nil
-    case "putpage":
-        if key == "" {
-            return "", fmt.Errorf("Must supply key with putpage")
-        }
-        if blobname == "" || container == "" || pagerange == "" || filename == "" {
-            return "", fmt.Errorf("Must supply container, blobname, pagerange and filename")
-        }
-        return "putpage", nil
-    }
-
-    return "", badOperationError()
+    op, isDefined := operationsByName[opName]
+    if !isDefined {
+        return "", badOperationError()
+    }
+
+    for _, arg := range op.requiredArgs {
+        if !argumentGiven(arg) {
+            return "", fmt.Errorf("%q requires these options: %v", op.name, op.requiredArgs)
+        }
+    }
+
+    if op.validate != nil {
+        err := op.validate()
+        if err != nil {
+            return "", err
+        }
+    }
+
+    return op.name, nil
 }
 
-var Usage = func() {
-    fmt.Fprintf(os.Stderr, "%s [args] %s\n", os.Args[0], VALID_CMDS)
+func Usage() {
+    fmt.Fprintf(
+        os.Stderr,
+        "Usage:\n    %s [args] <%s>\n",
+        os.Args[0],
+        strings.Join(operationNames, "|"))
     flag.PrintDefaults()
 
     fmt.Fprintf(os.Stderr, `
@@ -138,34 +246,11 @@
     operations that change things, (get these from the Azure web UI) otherwise
     anonymous access is made.  Additionally there are the following command
     invocation parameters:
-
-    Show existing storage containers:
-        listcontainers
-
-    List files in a container:
-        -container=<container> list
-
-    Set access on a container:
-        -container=<container> -acl <container|blob|private> containeracl
-
-    Get a file from a container (it's returned on stdout):
-        -container=<container> -filename=<filename> getblob
-
-    Upload a file to a block blob:
-        -container=<container> -filename=<filename> addblock
-
-    Delete a blob:
-        -container=<container> -filename=<filename> deleteblob
-
-    Create an empty page blob:
-        -container=<container> -blobname=<blobname> -size=<bytes>
-                -blobtype="page" putblob
-
-    Upload a file to a page blob's page.  The range parameters must be
-    (modulo 512)-(modulo 512 -1), eg: -pagerange=0-511
-        -container=<container> -blobname=<blobname> -pagerange=<N-N>
-        -filename=<local file> putpage
     `)
+
+    for _, op := range operations {
+        fmt.Fprintf(os.Stderr, "\n    %s:\n        %s\n", op.description, op.example)
+    }
 }
 
 func dumpError(err error) {
@@ -285,31 +370,19 @@
     op, err := getParams()
     if err != nil {
         fmt.Fprintf(os.Stderr, "%s\n", err.Error())
-        flag.Usage()
+        fmt.Fprintf(os.Stderr, "Use -h for help with using this program.\n")
         os.Exit(1)
     }
+    if help {
+        Usage()
+        os.Exit(0)
+    }
 
     storage := gwacl.StorageContext{
         Account: account,
         Key:     key,
     }
 
-    switch op {
-    case "listcontainers":
-        listcontainers(storage)
-    case "containeracl":
-        containeracl(storage)
-    case "list":
-        list(storage)
-    case "addblock":
-        addblock(storage)
-    case "deleteblob":
-        deleteblob(storage)
-    case "getblob":
-        getblob(storage)
-    case "putblob":
-        putblob(storage)
-    case "putpage":
-        putpage(storage)
-    }
+    perform := operationsByName[op].implementation
+    perform(storage)
 }