Code review comment for lp:~abentley/juju-ci-tools/industrial-test

Revision history for this message
Aaron Bentley (abentley) wrote :

Your summary is correct, but I misled you about the nature of exceptions destroy_environment is likely to raise.

I had forgotten that destroy_environment does not raise an exception when juju exits with failure. (It uses "call" rather "check_call", because juju returns a non-zero exit status if you try to destroy an environment that does not exist.)

So the sorts of exceptions you're likely to suppress are:
- string formatting issues when the 'destroy-environment' command cannot be constructed due to bogus values
- out-of-memory running fork()
- no-such-file running exec()

These are pretty rare and weird errors, and you'll only suppress them if new_client.destroy_environment() *also* raises a rare and weird error.

However, it's not hard to turn the try/finally into a try/except/finally where the except block logs the exception. Actually, it can be a try/except then, because after you've logged the exception, you shouldn't re-raise it.

« Back to merge proposal