Merge lp:~jtv/maas/maascli-cli-subcommand into lp:maas/trunk

[Jeroen T. Vermeulen]

Move maascli's CLI management commands (login, list etc.) into a dedicated sub-command "cli."

This was pre-imped ages ago, I think several times actually. It seems like a complication to say "maascli cli list" instead of just "maascli list," and so on, but after this I'll be able to make maascli pick a default profile, and move the current profile's API operations directly into the main namespace. So in return for this extra work, you'll be able to type API verbs directly after the "maascli" command.

Jeroen

[lp:~jtv/maas/maascli-cli-subcommand]

1226. By Jeroen T. Vermeulen on 2012-10-08

Fix some arbitrary typos to trigger a re-diff on the MP.

[Gavin Panella (allenap)]

> This was pre-imped ages ago, I think several times actually. It
> seems like a complication to say "maascli cli list" instead of just
> "maascli list," and so on, but after this I'll be able to make
> maascli pick a default profile, and move the current profile's API
> operations directly into the main namespace. So in return for this
> extra work, you'll be able to type API verbs directly after the
> "maascli" command.

I've made my thoughts clear ad nauseam, that I think having a default
profile is a retrograde step, but the code looks just fine :)

I'm actually having second thoughts about this change though. A new
user is going to be faced with:

  $ bin/maascli --help
  usage: bin/maascli [-h] COMMAND ...

  optional arguments:
    -h, --help  show this help message and exit

  drill down:
    COMMAND
      cli       CLI management commands

  http://maas.ubuntu.com/

It's not very obvious what to do now - just like it wasn't when we had
the "api" command - and that's at odds with the desire to make things
easier for the newbie, for which the default profile is intended (and
exceedingly lazy typists).

I'm not going to reject this, but I do think this is a step too far,
and needs a second opinion.

review: Needs Information

[Raphaël Badin (rvb)]

I agree with Gavin that putting login/logout/etc. under the 'cli' sub command will be a bit weird from a user's perspective.

I'm trying to think, step by step, what a user would do when he first interacts with the CLI and make sure that, at each step, what to do next is clear. As Gavin pointed out, the first step won't be obvious if the commands are "hidden" inside the 'cpi' subcommand. How about renaming login/logout/list/etc into maas_login/maas_logout/maas_list/maas_etc? This way, we avoid clashing with API commands and we still show the important commands when 'bin/maascli --help' is run from the first time.

What do you say?

[Jeroen T. Vermeulen (jtv)]

I would think that the right thing to do is point out the need for a login in the help text. I've already got some of that further up in my branch pipeline.

Once you're logged in, if there's a default profile, its API commands appear directly in the --help output.

[Jeroen T. Vermeulen (jtv)]

Scuttling this. We may or may not come back to it later, but for now we found the CLI tolerable with the improvements already made. Also, the “default profile” approach that this branch works towards is still controversial.

Unmerged revisions

1226. By Jeroen T. Vermeulen on 2012-10-08

Fix some arbitrary typos to trigger a re-diff on the MP.

1225. By Jeroen T. Vermeulen on 2012-10-08

Move cli commands out of maascli's main namespace.

=== modified file 'src/maascli/cli.py'
--- src/maascli/cli.py	2012-10-08 13:39:12 +0000
+++ src/maascli/cli.py	2012-10-08 16:37:21 +0000
@@ -27,7 +27,7 @@
 
 
 class cmd_login(Command):
-    """Log-in to a remote API, storing its description and credentials.
+    """Log in to a remote API, storing its description and credentials.
 
     If credentials are not provided on the command-line, they will be prompted
     for interactively.
@@ -88,7 +88,7 @@
 
 
 class cmd_logout(Command):
-    """Log-out of a remote API, purging any stored credentials."""
+    """Log out of a remote API, purging any stored credentials."""
 
     def __init__(self, parser):
         super(cmd_logout, self).__init__(parser)
@@ -104,7 +104,7 @@
 
 
 class cmd_list(Command):
-    """List remote APIs that have been logged-in to."""
+    """List remote APIs that have been logged in to."""
 
     def __call__(self, options):
         with ProfileConfig.open() as config:
@@ -129,8 +129,10 @@
 
 def register_cli_commands(parser):
     """Register the CLI's meta-subcommands on `parser`."""
+    cli_parser = parser.subparsers.add_parser(
+        'cli', help="CLI management commands")
     for name, command in commands.items():
         help_title, help_body = parse_docstring(command)
-        command_parser = parser.subparsers.add_parser(
+        command_parser = cli_parser.subparsers.add_parser(
             safe_name(name), help=help_title, description=help_body)
         command_parser.set_defaults(execute=command(command_parser))

=== modified file 'src/maascli/tests/test_cli.py'
--- src/maascli/tests/test_cli.py	2012-10-08 14:13:24 +0000
+++ src/maascli/tests/test_cli.py	2012-10-08 16:37:21 +0000
@@ -29,6 +29,7 @@
     def test_subparsers_have_appropriate_execute_defaults(self):
         parser = ArgumentParser()
         cli.register_cli_commands(parser)
+        cli_parser = parser.subparsers.choices['cli']
         self.assertIsInstance(
-            parser.subparsers.choices['login'].get_default('execute'),
+            cli_parser.subparsers.choices['login'].get_default('execute'),
             cli.cmd_login)

=== modified file 'src/maascli/tests/test_integration.py'
--- src/maascli/tests/test_integration.py	2012-10-04 08:36:34 +0000
+++ src/maascli/tests/test_integration.py	2012-10-08 16:37:21 +0000
@@ -48,6 +48,6 @@
         pass
 
     def test_list_command_succeeds(self):
-        self.run_command('list')
+        self.run_command('cli', 'list')
         # The test is that we get here without error.
         pass