Right now we catch and report properly only those exceptions that
are raised by the Application.run() method: basically, we work
under the assumption that nothing before that point can possibly
fail.

That's of course unrealistic: even now, creating an Inventory or
Projects object can raise an exception, in which case we simply
display a stack trace instead of a reasonable error message.

This commit introduces a CommandLine class which takes over
command line handling from the Application class, and moves all
exception handling to the main() method so that, short of
something going horribly wrong while parsing the command line,
we will always manage to hide stack traces from the user.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 guests/lcitool | 56 ++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index ab3b95f..ff58829 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -390,15 +390,9 @@ class Projects:
         return self._packages[project]
 
 
-class Application:
+class CommandLine:
 
     def __init__(self):
-        self._config = Config()
-        self._inventory = Inventory()
-        self._projects = Projects()
-
-        self._native_arch = Util.get_native_arch()
-
         self._parser = argparse.ArgumentParser(
             conflict_handler="resolve",
             description="libvirt CI guest management tool",
@@ -444,14 +438,14 @@ class Application:
 
         installparser = subparsers.add_parser(
             "install", help="perform unattended host installation")
-        installparser.set_defaults(func=self._action_install)
+        installparser.set_defaults(action="install")
 
         add_hosts_arg(installparser)
         add_wait_arg(installparser)
 
         updateparser = subparsers.add_parser(
             "update", help="prepare hosts and keep them updated")
-        updateparser.set_defaults(func=self._action_update)
+        updateparser.set_defaults(action="update")
 
         add_hosts_arg(updateparser)
         add_projects_arg(updateparser)
@@ -459,7 +453,7 @@ class Application:
 
         buildparser = subparsers.add_parser(
             "build", help="build projects on hosts")
-        buildparser.set_defaults(func=self._action_build)
+        buildparser.set_defaults(action="build")
 
         add_hosts_arg(buildparser)
         add_projects_arg(buildparser)
@@ -467,20 +461,33 @@ class Application:
 
         hostsparser = subparsers.add_parser(
             "hosts", help="list all known hosts")
-        hostsparser.set_defaults(func=self._action_hosts)
+        hostsparser.set_defaults(action="hosts")
 
         projectsparser = subparsers.add_parser(
             "projects", help="list all known projects")
-        projectsparser.set_defaults(func=self._action_projects)
+        projectsparser.set_defaults(action="projects")
 
         dockerfileparser = subparsers.add_parser(
             "dockerfile", help="generate Dockerfile (doesn't access the host)")
-        dockerfileparser.set_defaults(func=self._action_dockerfile)
+        dockerfileparser.set_defaults(action="dockerfile")
 
         add_hosts_arg(dockerfileparser)
         add_projects_arg(dockerfileparser)
         add_cross_arch_arg(dockerfileparser)
 
+    def parse(self):
+        return self._parser.parse_args()
+
+
+class Application:
+
+    def __init__(self):
+        self._config = Config()
+        self._inventory = Inventory()
+        self._projects = Projects()
+
+        self._native_arch = Util.get_native_arch()
+
     def _execute_playbook(self, playbook, hosts, projects, git_revision):
         base = Util.get_base()
 
@@ -1011,17 +1018,18 @@ class Application:
         varmap = self._dockerfile_build_varmap(facts, mappings, pip_mappings, projects, cross_arch)
         self._dockerfile_format(facts, cross_arch, varmap)
 
-    def run(self):
-        args = self._parser.parse_args()
-        if args.debug:
-            args.func(args)
-        else:
-            try:
-                args.func(args)
-            except Exception as err:
-                sys.stderr.write("{}: {}\n".format(sys.argv[0], err))
-                sys.exit(1)
+    def run(self, args):
+        getattr(self, "_action_" + args.action)(args)
 
 
 if __name__ == "__main__":
-    Application().run()
+    args = CommandLine().parse()
+
+    if args.debug:
+        Application().run(args)
+    else:
+        try:
+            Application().run(args)
+        except Exception as err:
+            sys.stderr.write("{}: {}\n".format(sys.argv[0], err))
+            sys.exit(1)
-- 
2.25.4

On Thu, May 07, 2020 at 05:41:57PM +0200, Andrea Bolognani wrote:
> Right now we catch and report properly only those exceptions that
> are raised by the Application.run() method: basically, we work
> under the assumption that nothing before that point can possibly
> fail.
> 
> That's of course unrealistic: even now, creating an Inventory or
> Projects object can raise an exception, in which case we simply
> display a stack trace instead of a reasonable error message.
> 
> This commit introduces a CommandLine class which takes over
> command line handling from the Application class, and moves all
> exception handling to the main() method so that, short of
> something going horribly wrong while parsing the command line,
> we will always manage to hide stack traces from the user.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  guests/lcitool | 56 ++++++++++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 24 deletions(-)
> 
> diff --git a/guests/lcitool b/guests/lcitool
> index ab3b95f..ff58829 100755
> --- a/guests/lcitool
> +++ b/guests/lcitool
> @@ -390,15 +390,9 @@ class Projects:
>          return self._packages[project]
>  
>  
> -class Application:
> +class CommandLine:
>  
>      def __init__(self):
> -        self._config = Config()
> -        self._inventory = Inventory()
> -        self._projects = Projects()
> -
> -        self._native_arch = Util.get_native_arch()
> -
>          self._parser = argparse.ArgumentParser(
>              conflict_handler="resolve",
>              description="libvirt CI guest management tool",
> @@ -444,14 +438,14 @@ class Application:
>  
>          installparser = subparsers.add_parser(
>              "install", help="perform unattended host installation")
> -        installparser.set_defaults(func=self._action_install)
> +        installparser.set_defaults(action="install")

please, keep the usage of the 'func' attribute:
.set_defaults(func=Application._action_install)

...

>  
> -    def run(self):
> -        args = self._parser.parse_args()
> -        if args.debug:
> -            args.func(args)
> -        else:
> -            try:
> -                args.func(args)
> -            except Exception as err:
> -                sys.stderr.write("{}: {}\n".format(sys.argv[0], err))
> -                sys.exit(1)
> +    def run(self, args):
> +        getattr(self, "_action_" + args.action)(args)

Okay, ^this is obfuscation:
1) the action attribute should not be misused for your own metadata, it defines
   a small set of actions to take when the option was seen on the cmdline [1]
   - for arbitrary actions, instantiate argparse.Action instead (also to be
     seen found at [1]) - you don't want to do this either in this case,
instead, respect the meaning of the 'func' attribute and use it in one of the
following ways:

# this one is still a bit obfuscated, but conforms to the encapsulation
# principles
self.__getattribute__(args.func.__name__)(args)

OR

# this IMO breaks the OOP principles and encapsulation, but at the same time is
# very much readable
args.func(self, args)

2) goes against the mantra you mentioned recently in your review:
"Explicit is better than implicit" (where implicit doesn't even apply here :))

[1] https://docs.python.org/3/library/argparse.html#action

With that addressed:
Reviewed-by: Erik Skultety <eskultet@redhat.com>

-- 
Erik Skultety

On Mon, 2020-05-11 at 14:03 +0200, Erik Skultety wrote:
> On Thu, May 07, 2020 at 05:41:57PM +0200, Andrea Bolognani wrote:
> > -        installparser.set_defaults(func=self._action_install)
> > +        installparser.set_defaults(action="install")
> 
> please, keep the usage of the 'func' attribute:
> .set_defaults(func=Application._action_install)
> 
> > -    def run(self):
> > -        args = self._parser.parse_args()
> > -        if args.debug:
> > -            args.func(args)
> > -        else:
> > -            try:
> > -                args.func(args)
> > -            except Exception as err:
> > -                sys.stderr.write("{}: {}\n".format(sys.argv[0], err))
> > -                sys.exit(1)
> > +    def run(self, args):
> > +        getattr(self, "_action_" + args.action)(args)
> 
> Okay, ^this is obfuscation:
> 1) the action attribute should not be misused for your own metadata, it defines
>    a small set of actions to take when the option was seen on the cmdline [1]
>    - for arbitrary actions, instantiate argparse.Action instead (also to be
>      seen found at [1]) - you don't want to do this either in this case,
> instead, respect the meaning of the 'func' attribute

I did not realize "action" and "func" had special meaning for
argparse. I'll adopt your suggestion.

-- 
Andrea Bolognani / Red Hat / Virtualization