Currently only a single global parser is used for all commands. This
means that every command accepts every argument which is undesirable as
users don't know what to pass. It also prevents the parser from
generating useful help information.

Python's argparse module supports multi-command binaries in a very easy
way by adding subparsers, each of which has their own distinct
arguments. It can then generate suitable help text on a per command
basis. This also means commands can use positional arguments for data
items that are always passed.

    $ ./guests/lcitool -h
    usage: lcitool [-h] {install,update,build,hosts,projects,dockerfile} ...

    libvirt CI guest management tool

    positional arguments:
      {install,update,build,hosts,projects,dockerfile}
                            commands
        install             perform unattended host installation
        update              prepare hosts and keep them updated
        build               build projects on hosts
        hosts               list all known hosts
        projects            list all known projects
        dockerfile          generate a host docker file

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

    $ ./guests/lcitool install -h
    usage: lcitool install [-h] hosts

    positional arguments:
      hosts       list of hosts to act on (accepts globs)

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

    $ ./guests/lcitool dockerfile  -h
    usage: lcitool dockerfile [-h] hosts projects

    positional arguments:
      hosts       list of hosts to act on (accepts globs)
      projects    list of projects to consider (accepts globs)

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

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 guests/lcitool | 135 ++++++++++++++++++++++++++++---------------------
 1 file changed, 78 insertions(+), 57 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 726e3bb..35a6b68 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -307,43 +307,72 @@ class Application:
             conflict_handler="resolve",
             formatter_class=argparse.RawDescriptionHelpFormatter,
             description="libvirt CI guest management tool",
-            epilog=textwrap.dedent("""
-                common actions:
-                  install  perform unattended host installation
-                  update   prepare hosts and keep them updated
-                  build    build projects on hosts
+        )
 
-                informational actions:
-                  hosts     list all known hosts
-                  projects  list all known projects
+        subparser = self._parser.add_subparsers(help="commands")
+        subparser.required = True
+        subparser.dest = "command"
 
-                uncommon actions:
-                  dockerfile  generate Dockerfile (doesn't access the host)
+        def add_hosts_arg(parser):
+            parser.add_argument(
+                "hosts",
+                help="list of hosts to act on (accepts globs)",
+            )
 
-                glob patterns are supported for HOSTS and PROJECTS
-            """),
-        )
-        self._parser.add_argument(
-            "-a",
-            metavar="ACTION",
-            required=True,
-            help="action to perform (see below)",
-        )
-        self._parser.add_argument(
-            "-h",
-            metavar="HOSTS",
-            help="list of hosts to act on",
-        )
-        self._parser.add_argument(
-            "-p",
-            metavar="PROJECTS",
-            help="list of projects to consider",
-        )
-        self._parser.add_argument(
-            "-g",
-            metavar="GIT_REVISION",
-            help="git revision to build (remote/branch)",
-        )
+        def add_projects_arg(parser):
+            parser.add_argument(
+                "projects",
+                help="list of projects to consider (accepts globs)",
+            )
+
+        def add_gitrev_arg(parser):
+            parser.add_argument(
+                "-g", "--git-revision",
+                help="git revision to build (remote/branch)",
+            )
+
+        installparser = subparser.add_parser(
+            "install", help="perform unattended host installation",
+            formatter_class=argparse.RawDescriptionHelpFormatter)
+        installparser.set_defaults(func=self._action_install)
+
+        add_hosts_arg(installparser)
+
+        updateparser = subparser.add_parser(
+            "update", help="prepare hosts and keep them updated",
+            formatter_class=argparse.RawDescriptionHelpFormatter)
+        updateparser.set_defaults(func=self._action_update)
+
+        add_hosts_arg(updateparser)
+        add_projects_arg(updateparser)
+        add_gitrev_arg(updateparser)
+
+        buildparser = subparser.add_parser(
+            "build", help="build projects on hosts",
+            formatter_class=argparse.RawDescriptionHelpFormatter)
+        buildparser.set_defaults(func=self._action_build)
+
+        add_hosts_arg(buildparser)
+        add_projects_arg(buildparser)
+        add_gitrev_arg(buildparser)
+
+        hostsparser = subparser.add_parser(
+            "hosts", help="list all known hosts",
+            formatter_class=argparse.RawDescriptionHelpFormatter)
+        hostsparser.set_defaults(func=self._action_hosts)
+
+        projectsparser = subparser.add_parser(
+            "projects", help="list all known projects",
+            formatter_class=argparse.RawDescriptionHelpFormatter)
+        projectsparser.set_defaults(func=self._action_projects)
+
+        dockerfileparser = subparser.add_parser(
+            "dockerfile", help="generate a host docker file",
+            formatter_class=argparse.RawDescriptionHelpFormatter)
+        dockerfileparser.set_defaults(func=self._action_dockerfile)
+
+        add_hosts_arg(dockerfileparser)
+        add_projects_arg(dockerfileparser)
 
     def _execute_playbook(self, playbook, hosts, projects, git_revision):
         base = Util.get_base()
@@ -404,20 +433,20 @@ class Application:
             raise Error(
                 "Failed to run {} on '{}': {}".format(playbook, hosts, ex))
 
-    def _action_hosts(self, _hosts, _projects, _revision):
+    def _action_hosts(self, args):
         for host in self._inventory.expand_pattern("all"):
             print(host)
 
-    def _action_projects(self, _hosts, _projects, _revision):
+    def _action_projects(self, args):
         for project in self._projects.expand_pattern("all"):
             print(project)
 
-    def _action_install(self, hosts, _projects, _revision):
+    def _action_install(self, args):
         base = Util.get_base()
 
         flavor = self._config.get_flavor()
 
-        for host in self._inventory.expand_pattern(hosts):
+        for host in self._inventory.expand_pattern(args.hosts):
             facts = self._inventory.get_facts(host)
 
             # Both memory size and disk size are stored as GiB in the
@@ -484,16 +513,18 @@ class Application:
             except Exception as ex:
                 raise Error("Failed to install '{}': {}".format(host, ex))
 
-    def _action_update(self, hosts, projects, git_revision):
-        self._execute_playbook("update", hosts, projects, git_revision)
+    def _action_update(self, args):
+        self._execute_playbook("update", args.hosts, args.projects,
+                               args.git_revision)
 
-    def _action_build(self, hosts, projects, git_revision):
-        self._execute_playbook("build", hosts, projects, git_revision)
+    def _action_build(self, args):
+        self._execute_playbook("build", args.hosts, args.projects,
+                               args.git_revision)
 
-    def _action_dockerfile(self, hosts, projects, _revision):
+    def _action_dockerfile(self, args):
         mappings = self._projects.get_mappings()
 
-        hosts = self._inventory.expand_pattern(hosts)
+        hosts = self._inventory.expand_pattern(args.hosts)
         if len(hosts) > 1:
             raise Error("Can't generate Dockerfile for multiple hosts")
         host = hosts[0]
@@ -507,7 +538,7 @@ class Application:
         if package_format not in ["deb", "rpm"]:
             raise Error("Host {} doesn't support Dockerfiles".format(host))
 
-        projects = self._projects.expand_pattern(projects)
+        projects = self._projects.expand_pattern(args.projects)
         for project in projects:
             if project not in facts["projects"]:
                 raise Error(
@@ -573,18 +604,8 @@ class Application:
                 """).format(**varmap))
 
     def run(self):
-        cmdline = self._parser.parse_args()
-        action = cmdline.a
-        hosts = cmdline.h
-        projects = cmdline.p
-        git_revision = cmdline.g
-
-        method = "_action_{}".format(action.replace("-", "_"))
-
-        if hasattr(self, method):
-            getattr(self, method).__call__(hosts, projects, git_revision)
-        else:
-            raise Error("Invalid action '{}'".format(action))
+        args = self._parser.parse_args()
+        args.func(args)
 
 
 if __name__ == "__main__":
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, 2019-02-21 at 16:33 +0000, Daniel P. Berrangé wrote:
> Currently only a single global parser is used for all commands. This
> means that every command accepts every argument which is undesirable as
> users don't know what to pass. It also prevents the parser from
> generating useful help information.
> 
> Python's argparse module supports multi-command binaries in a very easy
> way by adding subparsers, each of which has their own distinct
> arguments. It can then generate suitable help text on a per command
> basis. This also means commands can use positional arguments for data
> items that are always passed.
> 
>     $ ./guests/lcitool -h
>     usage: lcitool [-h] {install,update,build,hosts,projects,dockerfile} ...
> 
>     libvirt CI guest management tool
> 
>     positional arguments:
>       {install,update,build,hosts,projects,dockerfile}
>                             commands
>         install             perform unattended host installation
>         update              prepare hosts and keep them updated
>         build               build projects on hosts
>         hosts               list all known hosts
>         projects            list all known projects
>         dockerfile          generate a host docker file
> 
>     optional arguments:
>       -h, --help            show this help message and exit
> 
>     $ ./guests/lcitool install -h
>     usage: lcitool install [-h] hosts
> 
>     positional arguments:
>       hosts       list of hosts to act on (accepts globs)
> 
>     optional arguments:
>       -h, --help  show this help message and exit
> 
>     $ ./guests/lcitool dockerfile  -h
>     usage: lcitool dockerfile [-h] hosts projects
> 
>     positional arguments:
>       hosts       list of hosts to act on (accepts globs)
>       projects    list of projects to consider (accepts globs)
> 
>     optional arguments:
>       -h, --help  show this help message and exit

I feel like the usage examples are not really adding much to the
commit message, so I'd just leave them out.

On the other hand, all the examples in README.markdown are now
invalid, so you should make sure that file is updated along with
the script.

[...]
> +++ b/guests/lcitool
> @@ -307,43 +307,72 @@ class Application:
>              conflict_handler="resolve",
>              formatter_class=argparse.RawDescriptionHelpFormatter,

Since we're no longer providing our own, pre-formatted help text
and we're leaving its generation up to argparse instead, setting
formatter_class is no longer needed.

The same applies to all callers you introduce later.

[...]
> +        subparser = self._parser.add_subparsers(help="commands")

Please rename the variable to 'subparsers'. At the same time, drop
use of the 'help' parameter and set 'metavar' to 'ACTION' instead:
that will change the main help output from

  usage: lcitool [-h] {install,update,build,hosts,projects,dockerfile} ...

  libvirt CI guest management tool

  positional arguments:
    {install,update,build,hosts,projects,dockerfile}
                          commands
      install             perform unattended host installation
      ...

to a less unwieldy

  usage: lcitool [-h] ACTION ...

  libvirt CI guest management tool

  positional arguments:
    ACTION
      install   perform unattended host installation
      ...

As an aside, I don't much like how argparse takes over the '-h'
option as a shorthand for '--help', especially since up until now
it has been used to specify hosts, but I haven't found a way to
disable that behavior. I guess I can live with it :)

> +        subparser.required = True
> +        subparser.dest = "command"

Setting the 'dest' parameter is not necessary - you're not using
it anywhere.

[...]
> +        installparser.set_defaults(func=self._action_install)

Well, isn't this clever :)

[...]
> +        dockerfileparser = subparser.add_parser(
> +            "dockerfile", help="generate a host docker file",

I'd prefer if you kept the original help text here.


Everything else looks good.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list