Merge lp:~jcsackett/launchpad/bug-expiration-doesnt-oops into lp:launchpad

[j.c.sackett]

Adds some exception catching and Oops logging to a previously patched problem area in expire-bugtasks.py

Summary
=======
The bug expiration script wasn't working due to a permisions error, but because it wasn't logging the issue no Oops was raised.

The major parts of setting up the script and running the expiration job are now enclosed in try/excepts that log any errors to the Oops handler.

Preimplementation
=================
Spoke with Curtis Hovey

Implementation
==============
Added try/excepts around the initialization of the BugJanitor and the janitor running it's expiration job. The excepts are catchalls because we can't predict what might happen--anything expected should be handled within the routines called, this is a last-ditch "at least log it" measure.

Tests
=====
I cannot for the life of me figure out how to test a cronjob itself. The actual expiration code used by the cronjob is already tested, and the error actually occurs when the cronscript itself is run. If there is a utility to test cronscript/crontabs that I am unaware of, I am happy to implement tests for this.

QA
==
qa-untestable. This will only come into being if we encounter a new error. If necessary, we can alter security.cfg to reintroduce the error we originally discovered and see if it gets logged properly.

Lint
====
= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  cronscripts/expire-bugtasks.py

./cronscripts/expire-bugtasks.py
    16: '_pythonpath' imported but unused
    33: E222 multiple spaces after operator

[Robert Collins (lifeless)]

So, I'm ok on the testing aspect, but the code needs to be correct :)

try:
   foo = bar
except Exception:
   ..
try:
   foo.quux()
except Exception:
   ..

is going to fail horridly because foo won't be assigned in the second try:except: block.

You can write this correctly as:
try:
    foo = bar
except Exception:
    ..
else:
    try:
       foo.quux()
    except Exception:
       ..

But I think that really all you need is:
try:
   foo = bar
   foo.quux()
except Exception:
   ..
   raise

e.g. (and I've improved the use of logging here too):
+ try:
+ janitor = BugJanitor(
+ log=self.logger, target=target, limit=self.options.limit)
+ janitor.expireBugTasks(self.txn)
+ except Exception:
+ self.logger.error(
+ 'An error occured trying to expire bugtasks.', exc_info=1)
+ raise

The raise is there because we shouldn't swallow exceptions unless we have corrected the error - and we haven't. It may not be needed (I haven't checked to see if that was the last line in the script) but its safer to assume its needed - or put a comment in on why its not, as catchall excepts really are, well, the exception.

review: Needs Fixing

[Robert Collins (lifeless)]

(by last line, I mean in the context of the whole template-method Script object doohicky, not the code merely in this file :)).

[j.c.sackett (jcsackett)]

> So, I'm ok on the testing aspect, but the code needs to be correct :)
>
> try:
> foo = bar
> except Exception:
> ..
> try:
> foo.quux()
> except Exception:
> ..
>
> is going to fail horridly because foo won't be assigned in the second
> try:except: block.

Damn, good catch.

[j.c.sackett (jcsackett)]

> The raise is there because we shouldn't swallow exceptions unless we have
> corrected the error - and we haven't. It may not be needed (I haven't checked
> to see if that was the last line in the script) but its safer to assume its
> needed - or put a comment in on why its not, as catchall excepts really are,
> well, the exception.

You're right about the raise, and I thought I had put that in; it (and the other changes) have been made and pushed up.

[Robert Collins (lifeless)]

Sweet.

review: Approve

=== modified file 'cronscripts/expire-bugtasks.py'
--- cronscripts/expire-bugtasks.py	2010-10-21 20:05:52 +0000
+++ cronscripts/expire-bugtasks.py	2011-04-20 14:28:55 +0000
@@ -47,13 +47,19 @@
             # Avoid circular import.
             from lp.registry.interfaces.distribution import IDistributionSet
             target = getUtility(IDistributionSet).getByName('ubuntu')
-        janitor = BugJanitor(
-            log=self.logger, target=target, limit=self.options.limit)
-        janitor.expireBugTasks(self.txn)
+        try:
+            janitor = BugJanitor(
+                log=self.logger, target=target, limit=self.options.limit)
+            janitor.expireBugTasks(self.txn)
+        except Exception:
+            # We use a catchall here because we don't know (and don't care)
+            # about the particular error--we'll just log it to as an Oops.
+            self.logger.error(
+                'An error occured trying to expire bugtasks.', exc_info=1)
+            raise
 
 
 if __name__ == '__main__':
     script = ExpireBugTasks(
         'expire-bugtasks', dbuser=config.malone.expiration_dbuser)
     script.lock_and_run()
-