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

Proposed by Jeroen T. Vermeulen on 2013-08-19
Status: Merged
Approved by: Jeroen T. Vermeulen on 2013-08-19
Approved revision: 223
Merged at revision: 219
Proposed branch: lp:~jtv/gwacl/storage-tool-command-line
Merge into: lp:gwacl
Diff against target: 386 lines (+203/-130)
1 file modified
example/storage/run.go (+203/-130)
To merge this branch: bzr merge lp:~jtv/gwacl/storage-tool-command-line
Reviewer Review Type Date Requested Status
Julian Edwards (community) 2013-08-19 Approve on 2013-08-19
Review via email: mp+180765@code.launchpad.net

Commit message

Clean up command-line parsing in storage tool.

Description of the change

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

To post a comment you must log in.
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) wrote :

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
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) wrote :

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) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'example/storage/run.go'
2--- example/storage/run.go 2013-07-12 14:42:08 +0000
3+++ example/storage/run.go 2013-08-19 05:20:32 +0000
4@@ -15,33 +15,170 @@
5 "io/ioutil"
6 "launchpad.net/gwacl"
7 "os"
8+ "strings"
9 )
10
11-var account string
12-var key string
13-var filename string
14-var container string
15-var prefix string
16-var blobname string
17-var blobtype string
18-var size int
19-var pagerange string
20-var acl string
21-
22-func checkContainerAndFilename() error {
23- if container == "" || filename == "" {
24- return fmt.Errorf("Must supply container and filename")
25- }
26- return nil
27-}
28-
29-var VALID_CMDS string = "<listcontainers|containeracl|list|getblob|addblock|deleteblob|putblob|putpage>"
30-
31 func badOperationError() error {
32- return fmt.Errorf("Must specify one of %s", VALID_CMDS)
33+ return fmt.Errorf("Must specify one of %v", operationNames)
34+}
35+
36+// operation is something you can instruct this program to do, by specifying
37+// its name on the command line.
38+type operation struct {
39+ // name is the operation name as used on the command line.
40+ name string
41+ // description holds a description of what the operation does.
42+ description string
43+ // example illustrates how the operation is used.
44+ example string
45+ // requiredArgs lists the command-line options that are required for this
46+ // operation.
47+ requiredArgs []string
48+ // validate is an optional callback to perform more detailed checking on
49+ // the operation's arguments.
50+ validate func() error
51+ // implementation is a function that performs the operation. If it fails,
52+ // it just panics.
53+ implementation func(gwacl.StorageContext)
54+}
55+
56+// operations defines what operations are available to be invoked from the
57+// command line.
58+var operations = []operation{
59+ {
60+ name: "listcontainers",
61+ description: "Show existing storage containers",
62+ example: "listcontainers",
63+ implementation: listcontainers,
64+ },
65+ {
66+ name: "list",
67+ description: "List files in a container",
68+ example: "-container=<container> list",
69+ requiredArgs: []string{"list"},
70+ implementation: list,
71+ },
72+ {
73+ name: "containeracl",
74+ description: "Set access on a container",
75+ example: "-container=<container> -acl <container|blob|private> containeracl",
76+ requiredArgs: []string{"container", "key", "acl"},
77+ validate: func() error {
78+ if acl != "container" && acl != "blob" && acl != "private" {
79+ return fmt.Errorf("Usage: containeracl -container=<container> <container|blob|private>")
80+ }
81+ return nil
82+ },
83+ implementation: containeracl,
84+ },
85+ {
86+ name: "getblob",
87+ description: "Get a file from a container (it's returned on stdout)",
88+ example: "-container=<container> -filename=<filename> getblob",
89+ requiredArgs: []string{"container", "filename"},
90+ implementation: getblob,
91+ },
92+ {
93+ name: "addblock",
94+ description: "Upload a file to a block blob",
95+ example: "-container=<container> -filename=<filename> addblock",
96+ requiredArgs: []string{"key", "container", "filename"},
97+ implementation: addblock,
98+ },
99+ {
100+ name: "deleteblob",
101+ description: "Delete a blob",
102+ example: "-container=<container> -filename=<filename> deleteblob",
103+ requiredArgs: []string{"key", "container", "filename"},
104+ implementation: deleteblob,
105+ },
106+ {
107+ name: "putblob",
108+ description: "Create an empty page blob",
109+ example: "-container=<container> -blobname=<blobname> -size=<bytes> " +
110+ "-blobtype=\"page\" putblob",
111+ requiredArgs: []string{"key", "blobname", "blobtype", "container", "size"},
112+ implementation: putblob,
113+ },
114+ {
115+ name: "putpage",
116+ description: "Upload a file to a page blob's page. The range parameters must " +
117+ "be (modulo 512)-(modulo 512 -1), eg: -pagerange=0-511",
118+ example: "-container=<container> -blobname=<blobname> -pagerange=<N-N> " +
119+ "-filename=<local file> putpage",
120+ requiredArgs: []string{"key", "blobname", "container", "pagerange", "filename"},
121+ implementation: putpage,
122+ },
123+}
124+
125+// operationsByName maps each opeation name to an operation struct that
126+// describes it.
127+var operationsByName map[string]operation
128+
129+// operationNames lists just the names of the oeprations, in the order in which
130+// they are listed in "operations."
131+var operationNames []string
132+
133+func init() {
134+ operationsByName = make(map[string]operation, len(operations))
135+ for _, op := range operations {
136+ operationsByName[op.name] = op
137+ }
138+
139+ operationNames = make([]string, len(operations))
140+ for index, op := range operations {
141+ operationNames[index] = op.name
142+ }
143+}
144+
145+// Variables set by command-line options.
146+var (
147+ help bool
148+ account string
149+ location string
150+ key string
151+ filename string
152+ container string
153+ prefix string
154+ blobname string
155+ blobtype string
156+ size int
157+ pagerange string
158+ acl string
159+)
160+
161+// argumentGiven returns whether the named argument was specified on the
162+// command line.
163+func argumentGiven(name string) bool {
164+ // This is stupid. There must be a way to ask the flag module directly!
165+ switch name {
166+ case "account":
167+ return account != ""
168+ case "key":
169+ return key != ""
170+ case "container":
171+ return container != ""
172+ case "filename":
173+ return filename != ""
174+ case "prefix":
175+ return prefix != ""
176+ case "blobname":
177+ return blobname != ""
178+ case "blobtype":
179+ return blobtype != ""
180+ case "size":
181+ return size != 0
182+ case "pagerange":
183+ return pagerange != ""
184+ case "acl":
185+ return acl != ""
186+ }
187+ panic(fmt.Errorf("internal error: unknown command-line option: %s", name))
188 }
189
190 func getParams() (string, error) {
191+ flag.BoolVar(&help, "h", false, "Show usage and exit")
192+
193 flag.StringVar(&account, "account", "", "Storage account name")
194 flag.StringVar(&key, "key", "", "A valid storage account key (base64 encoded), defaults to the empty string (i.e. anonymous access)")
195 flag.StringVar(&container, "container", "", "Name of the container to use")
196@@ -54,6 +191,15 @@
197 flag.StringVar(&acl, "acl", "", "When using 'containeracl', specify an ACL type")
198 flag.Parse()
199
200+ if help {
201+ return "", nil
202+ }
203+
204+ opName := flag.Arg(0)
205+ if opName == "" {
206+ return "", fmt.Errorf("No operation specified")
207+ }
208+
209 if account == "" {
210 return "", fmt.Errorf("Must supply account parameter")
211 }
212@@ -62,71 +208,33 @@
213 return "", badOperationError()
214 }
215
216- switch flag.Arg(0) {
217- case "listcontainers":
218- return "listcontainers", nil
219- case "containeracl":
220- if container == "" {
221- return "", fmt.Errorf("Must supply container with containeracl")
222- }
223- if key == "" {
224- return "", fmt.Errorf("Must supply key with containeracl")
225- }
226- if acl != "container" && acl != "blob" && acl != "private" {
227- return "", fmt.Errorf("Usage: containeracl -container=<container> <container|blob|private>")
228- }
229- return "containeracl", nil
230- case "list":
231- if container == "" {
232- return "", fmt.Errorf("Must supply container with 'list'")
233- }
234- return "list", nil
235- case "getblob":
236- if container == "" || filename == "" {
237- return "", fmt.Errorf("Must supply container and filename with 'list'")
238- }
239- return "getblob", nil
240- case "addblock":
241- if key == "" {
242- return "", fmt.Errorf("Must supply key with addblock")
243- }
244- err := checkContainerAndFilename()
245- if err != nil {
246- return "", err
247- }
248- return "addblock", nil
249- case "deleteblob":
250- if key == "" {
251- return "", fmt.Errorf("Must supply key with deleteblob")
252- }
253- err := checkContainerAndFilename()
254- if err != nil {
255- return "", err
256- }
257- return "deleteblob", nil
258- case "putblob":
259- if key == "" {
260- return "", fmt.Errorf("Must supply key with putblob")
261- }
262- if blobname == "" || blobtype == "" || container == "" || size == 0 {
263- return "", fmt.Errorf("Must supply container, blobname, blobtype and size with 'putblob'")
264- }
265- return "putblob", nil
266- case "putpage":
267- if key == "" {
268- return "", fmt.Errorf("Must supply key with putpage")
269- }
270- if blobname == "" || container == "" || pagerange == "" || filename == "" {
271- return "", fmt.Errorf("Must supply container, blobname, pagerange and filename")
272- }
273- return "putpage", nil
274- }
275-
276- return "", badOperationError()
277+ op, isDefined := operationsByName[opName]
278+ if !isDefined {
279+ return "", badOperationError()
280+ }
281+
282+ for _, arg := range op.requiredArgs {
283+ if !argumentGiven(arg) {
284+ return "", fmt.Errorf("%q requires these options: %v", op.name, op.requiredArgs)
285+ }
286+ }
287+
288+ if op.validate != nil {
289+ err := op.validate()
290+ if err != nil {
291+ return "", err
292+ }
293+ }
294+
295+ return op.name, nil
296 }
297
298-var Usage = func() {
299- fmt.Fprintf(os.Stderr, "%s [args] %s\n", os.Args[0], VALID_CMDS)
300+func Usage() {
301+ fmt.Fprintf(
302+ os.Stderr,
303+ "Usage:\n %s [args] <%s>\n",
304+ os.Args[0],
305+ strings.Join(operationNames, "|"))
306 flag.PrintDefaults()
307
308 fmt.Fprintf(os.Stderr, `
309@@ -138,34 +246,11 @@
310 operations that change things, (get these from the Azure web UI) otherwise
311 anonymous access is made. Additionally there are the following command
312 invocation parameters:
313-
314- Show existing storage containers:
315- listcontainers
316-
317- List files in a container:
318- -container=<container> list
319-
320- Set access on a container:
321- -container=<container> -acl <container|blob|private> containeracl
322-
323- Get a file from a container (it's returned on stdout):
324- -container=<container> -filename=<filename> getblob
325-
326- Upload a file to a block blob:
327- -container=<container> -filename=<filename> addblock
328-
329- Delete a blob:
330- -container=<container> -filename=<filename> deleteblob
331-
332- Create an empty page blob:
333- -container=<container> -blobname=<blobname> -size=<bytes>
334- -blobtype="page" putblob
335-
336- Upload a file to a page blob's page. The range parameters must be
337- (modulo 512)-(modulo 512 -1), eg: -pagerange=0-511
338- -container=<container> -blobname=<blobname> -pagerange=<N-N>
339- -filename=<local file> putpage
340 `)
341+
342+ for _, op := range operations {
343+ fmt.Fprintf(os.Stderr, "\n %s:\n %s\n", op.description, op.example)
344+ }
345 }
346
347 func dumpError(err error) {
348@@ -285,31 +370,19 @@
349 op, err := getParams()
350 if err != nil {
351 fmt.Fprintf(os.Stderr, "%s\n", err.Error())
352- flag.Usage()
353+ fmt.Fprintf(os.Stderr, "Use -h for help with using this program.\n")
354 os.Exit(1)
355 }
356+ if help {
357+ Usage()
358+ os.Exit(0)
359+ }
360
361 storage := gwacl.StorageContext{
362 Account: account,
363 Key: key,
364 }
365
366- switch op {
367- case "listcontainers":
368- listcontainers(storage)
369- case "containeracl":
370- containeracl(storage)
371- case "list":
372- list(storage)
373- case "addblock":
374- addblock(storage)
375- case "deleteblob":
376- deleteblob(storage)
377- case "getblob":
378- getblob(storage)
379- case "putblob":
380- putblob(storage)
381- case "putpage":
382- putpage(storage)
383- }
384+ perform := operationsByName[op].implementation
385+ perform(storage)
386 }

Subscribers

People subscribed via source and target branches