Looks good! All the registering stuff looks a bit heavy to me but I suppose it's the price you pay for making stuff extendable. Same as all the API helpers methods that we've created ( ;) ), I don't expect us having to work too much in that area once that'll be working as expected though. This definitely gives us something to work on so I'm approving this even if this needs some more work. This obviously won't break anything as it's a completely new functionality. As per our discussion and what I gathered reading that code: Things we need to do (I think): - Add support for the metadata api - Add support for multiple arguments (api/?arg=a&arg=b). - Fix encode_multipart_data so that it won't blow up when encountering a non-ASCII string. Nice to have: - Tab completion (that would be really nice to help humans interacting with the API via this CLI). - Better separation between anonymous commands and logged-in commands. Also, the testing coverage seems to be still pretty limited but we definitely can iterate on that. [0] 398 + if response["content-type"] != "application/json": 399 + raise CommandError( 400 + "Expected application/json, got: %(content-type)s" % response) 401 + return json.loads(content) I'd also add a try/except block to cope with the possibility of the response to be wrongly formatted json. [1] 421 + "http://example.com/api/1.0/")) Maybe change that to "http://example.com/MAAS/api/1.0/" since the default package uses a '/MAAS' prefix. [2] 424 + "The credentials, also known as the API key, for the " 425 + "remote MAAS server. These can be found in the user " 426 + "preferences page in the web UI." One will be tempted to use his login/password so I'd be a bit more precise here to describe what the credentials are (a big string), maybe by giving an example of what they look like. [3] 687 + "CREATE TABLE IF NOT EXISTS profiles " 688 + "(id INTEGER PRIMARY KEY," 689 + " name TEXT NOT NULL UNIQUE," 690 + " data BLOB)") I wonder why you choose to use that (a sqlite db) over serializing a json dictionary into a file? That would have all the nice properties that you have here (locking using file locking mechanism, etc.) and it would be more simple (to test) and more easy to extend. [4] 250 + if isinstance(module, (str, unicode)): isinstance(obj, basestring) is a shorter equivalent. [5] $ ./bin/maascli api admin node read node-33b55e28-4671-11e1-93b8-00225f89f211 200 OK Content-Location: http://192.168.0.3:5240/api/1.0/nodes/node-33b55e28-4671-11e1-93b8-00225f89f211/ Content-Type: application/json; charset=utf-8 Date: Tue, 18 Sep 2012 11:10:34 GMT Server: WSGIServer/0.1 Python/2.7.3 Status: 200 Transfer-Encoding: chunked Vary: Authorization,Cookie [...] I don't think this should be displayed unless in a verbose mode. I'm talking about the "200 OK" bit and the headers. 595 + """Show an HTTP response in a human-friendly way.""" I suspect that CLI will be used by humans but also a lot by scripts… [6] $ ./bin/maascli api admin node read node-33b55e28-4671-11e1-93b8-00225f89f211s 404 NOT FOUND Content-Type: text/plain Date: Tue, 18 Sep 2012 11:12:27 GMT Server: WSGIServer/0.1 Python/2.7.3 Status: 404 Transfer-Encoding: chunked Vary: Authorization,Cookie Not Found Same here, the last line "Not Found" should probably be enough unless the -v switch is on. [7] 299 + name[4:]: command for name, command in vars(module).items() I advise writing 'name[4:]' as 'name[:len("cmd_")]' for clarity. [8] 521 + # TODO: this is el-cheapo URI Template […] I think it would be worth converting the not-so-important TODOs as simple comments and the important ones as bug references. [9] 534 + # Sign request if credentials have been provided. 535 + self.maybe_sign(uri, headers) […] 589 + if self.credentials is not None: I think it would be clearer to move that check up the stack and rename 'maybe_sign' as 'sign' :) [10] $ ./bin/maascli usage: ./bin/maascli [-h] {api} ... ./bin/maascli: error: too few arguments Maybe an invocation without any argument should be a little bit more verbose. [11] I think it's important to mention the CLI in the docs. That can be very minimal right now and we will iterate on it but now that the team is expanding, people rely on things being (at least mentioned) in the docs. Also, it will get people to try it out and report problems. [12] I'm thinking about people using the CLI in scripts… is there a nicer way to log-in as an anonymous user than to do: echo "" | ./bin/maascli api login anon http://0.0.0.0:5240/api/1.0/ - ?