Merge lp:~allenap/maas/command-line into lp:maas/trunk
| Status: | Merged |
|---|---|
| Approved by: | Gavin Panella on 2012-09-18 |
| Approved revision: | 1069 |
| Merged at revision: | 1015 |
| Proposed branch: | lp:~allenap/maas/command-line |
| Merge into: | lp:maas/trunk |
| Diff against target: |
1434 lines (+1227/-53) 17 files modified
buildout.cfg (+2/-0) docs/index.rst (+8/-0) required-packages/base (+1/-0) src/apiclient/maas_client.py (+0/-11) src/apiclient/tests/test_maas_client.py (+0/-14) src/apiclient/tests/test_utils.py (+29/-0) src/apiclient/utils.py (+28/-0) src/maascli/__init__.py (+119/-23) src/maascli/api.py (+336/-0) src/maascli/config.py (+89/-0) src/maascli/tests/test_api.py (+122/-0) src/maascli/tests/test_config.py (+102/-0) src/maascli/tests/test_init.py (+113/-0) src/maascli/tests/test_utils.py (+189/-0) src/maascli/utils.py (+88/-0) src/provisioningserver/tftp.py (+1/-1) versions.cfg (+0/-4) |
| To merge this branch: | bzr merge lp:~allenap/maas/command-line |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Raphaël Badin (community) | 2012-09-17 | Approve on 2012-09-18 | |
|
Review via email:
|
|||
Commit Message
MAAS command-line client.
Implements support for interacting with MAAS from the command-line. All of MAAS's API is exposed: 'maascli api login ...' connects to a remote server and obtains its API description, automatically augmenting the local client. It's also discoverable: -h/--help can be used to get information on the operations available.
Description of the Change
The first iteration of the MAAS command-line API client. It's not fully tested, but hopefully we can all chip in once it's in the tree.
| Gavin Panella (allenap) wrote : | # |
Thank you enormously!
> 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_
> 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[
> 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.
For now I think that can just bubble up. We can iterate on this.
>
> [1]
>
> 421 + "http://
>
> Maybe change that to "http://
> package uses a '/MAAS' prefix.
Good idea.
>
> [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.
I've added a description, but we can refine this later.
>
> [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.
I did that first, and I ended up writing code for locking and atomic
writing, and it occurred to me that sqlite provides this all for free.
Fwiw, Zed Shaw came to a similar conclusion when writing the config
system for Mongrel2, and I was inspired by that.
>
> [4]
>
> 250 + if isinstance(module, (str, unicode)):
>
> isinstance(obj, basestring) is a shorter equivalent.
This was my attempt to be forwards compatible, but unicode is not
(yet) an alias for str in Python 3. So...
- 1064. By Gavin Panella on 2012-09-18
-
Update example API URL.
- 1065. By Gavin Panella on 2012-09-18
-
Update the help for credentials.
- 1066. By Gavin Panella on 2012-09-18
-
Switched to basestring.
- 1067. By Gavin Panella on 2012-09-18
-
Parameterise command prefix.
- 1068. By Gavin Panella on 2012-09-18
-
Change maybe_sign to sign, a static method.
- 1069. By Gavin Panella on 2012-09-18
-
Add paragraph about MAAS from the command-line.
| John A Meinel (jameinel) wrote : | # |
529 + uri = property(lambda self: self.handler[
530 + method = property(lambda self: self.action[
531 + restful = property(lambda self: self.action[
532 + credentials = property(lambda self: self.profile[
533 + op = property(lambda self: self.action["op"])
These look like they might be hard to debug. Would it be better to just put this unwrapping in register_handlers? I wouldn't require it, but it seems if one of them was missing the errors would be hard to understand.
Also, I'd still rather have the tool called 'maas' rather than 'maascli', but I'm willing to be overruled on it. (Having the library and python imports be maascli makes a lot of sense, less-so the actual tool name that gets typed on the commandline.)
Is it possible to minimize/remove the dependency on twisted? It seems like the CLI doesn't actually use it, and it feels like a pretty heavyweight dependency that we aren't using.
I feel like we should split out the code that creates an Action class and get some unittests for it. That can be added later, but it would help to clarify what the structure is supposed to be and what it is supposed to contain.
| Gavin Panella (allenap) wrote : | # |
On 19 September 2012 07:45, John A Meinel <email address hidden> wrote:
> 529 + uri = property(lambda self: self.handler[
> 530 + method = property(lambda self: self.action[
> 531 + restful = property(lambda self: self.action[
> 532 + credentials = property(lambda self: self.profile[
> 533 + op = property(lambda self: self.action["op"])
>
> These look like they might be hard to debug. Would it be better to
> just put this unwrapping in register_handlers? I wouldn't require
> it, but it seems if one of them was missing the errors would be hard
> to understand.
I like resolving these values late so that there's no repetition. I
suppose these could be resolved into action_ns in register_actions,
instead of profile, handler, and action, but I think that would
prematurely flatten those structures. I'd rather give the Action
subclass its full context and let it decide what's relevant as late as
possible.
> Also, I'd still rather have the tool called 'maas' rather than
> 'maascli', but I'm willing to be overruled on it. (Having the
> library and python imports be maascli makes a lot of sense, less-so
> the actual tool name that gets typed on the commandline.)
Agreed, I just need to take care of the existing maas script.
> Is it possible to minimize/remove the dependency on twisted? It
> seems like the CLI doesn't actually use it, and it feels like a
> pretty heavyweight dependency that we aren't using.
The dependency is only for testing; the .deb will not depend on
Twisted. We already have Twisted around for other things so pulling it
in for the maascli test suite is almost free.
> I feel like we should split out the code that creates an Action
> class and get some unittests for it. That can be added later, but it
> would help to clarify what the structure is supposed to be and what
> it is supposed to contain.
I agree here too. I wanted to get some tests written for it, but it
became more important to land it for others to see than it was to get
tests in place.
Thanks for reading through it!


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: multipart_ data so that it won't blow up when encountering a non-ASCII string.
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_
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-00225f89f2 11
200 OK
Content- Location: http:// 192.168. 0.3:5240/ api/1.0/ nodes/node- 33b55e28- 4671-11e1- 93b8-00225f89f2 11/
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 Encoding: chunked
Vary: Authorization, Cookie
Transfer-
[...]
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 sc...