[libvirt] [jenkins-ci PATCH] lcitool: Raise Error instead of Exception

[Daniel P. Berrangé]

On Mon, Mar 11, 2019 at 07:11:49PM +0100, Andrea Bolognani wrote:
> On Mon, 2019-03-11 at 17:55 +0000, Daniel P. Berrangé wrote:
> > On Mon, Mar 11, 2019 at 06:48:11PM +0100, Andrea Bolognani wrote:
> > > This results in
> > > 
> > >   $ ./lcitool dockerfile dockerfile -x foo libvirt-debian-9 libvirt
> > >   FROM debian:9
> > >   ./lcitool: Unsupported architecture ppc64el
> > > 
> > > being printed on error, instead of the much nastier
> > > 
> > >   $ ./lcitool dockerfile dockerfile -x foo libvirt-debian-9 libvirt
> > >   FROM debian:9
> > >   Traceback (most recent call last):
> > >     File "./lcitool", line 704, in <module>
> > >       Application().run()
> > >     File "./lcitool", line 699, in run
> > >       args.func(args)
> > >     File "./lcitool", line 643, in _action_dockerfile
> > >       deb_arch = Util.native_arch_to_deb_arch(args.cross_arch)
> > >     File "./lcitool", line 126, in native_arch_to_deb_arch
> > >       raise Exception("Unsupported architecture {}".format(native_arch))
> > >   Exception: Unsupported architecture foo
> > 
> > I'm curious why the "Error" class exists at all ? It doesn't seem
> > to add anything that the normal "Exception" class can't do, and
> > leads to bugs like the one here.
> 
> I seem to understand you're not supposed to use Exception directly,
> but rather define your own exception types:
> 
>   https://docs.python.org/3/tutorial/errors.html#user-defined-exceptions
> 
> I remember reading more about this, but I can't find the source
> right now.

I think it largely depends on the scope / scale of the application you
are writing. If the code is complex enough that you're going to want
to be catching exceptions in specific scenarios, then creating subclasses
makes sense as you can avoid mistakenly catching too much exceptions.

In the lcitool case though, we're not trying to do anything very clever
with catching specific exceptions as our app is quite small & self
contained. We just want to catch everything to hide the ugly stack traces
by default. IMHO in that scenario it is fine to just use the base exception
class directly. BTW, there's no need for the "message" field, even with
the Error class, as the default Exception class will stringify to report
just the message it received: eg this is equiv:

@@ -703,5 +700,5 @@ if __name__ == "__main__":
     try:
         Application().run()
     except Error as err:
-        sys.stderr.write("{}: {}\n".format(sys.argv[0], err.message))
+        sys.stderr.write("{}: {}\n".format(sys.argv[0], err))
         sys.exit(1)

anyway, I think we can just simplify our life & delete the Error class.

It would probably be worth registering a "--debug" flag to the CLI to
disable the global  exception catch, so that we can see the full stack
trace when needed.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|