Am 13.04.2010 13:57, schrieb Jelmer Vernooij:
> This branch should make process-accepted a bit more robust. It changes it to file an OOPS when it encounters an exception while processing an item in the queue, rather than re-raising that exception, skipping the processing of the rest of the queue and rolling back the transactions.
> 
> This should make it impossible for a single problematic package to block process-accepted, as happened about a week ago.

Very good, thank you for fixing this!


> === modified file 'lib/lp/soyuz/scripts/processaccepted.py'
> --- lib/lp/soyuz/scripts/processaccepted.py	2010-02-23 10:30:56 +0000
> +++ lib/lp/soyuz/scripts/processaccepted.py	2010-04-13 11:56:59 +0000
> @@ -11,12 +11,16 @@
>      'ProcessAccepted',
>      ]
>  
> +import sys
> +
>  from zope.component import getUtility
>  
>  from lp.archiveuploader.tagfiles import parse_tagfile_lines
>  from lp.bugs.interfaces.bug import IBugSet
>  from lp.bugs.interfaces.bugtask import BugTaskStatus
>  from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
> +from canonical.launchpad.webapp.errorlog import (
> +    ErrorReportingUtility, ScriptRequest)
>  from canonical.launchpad.webapp.interfaces import NotFoundError
>  from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
>  from lp.registry.interfaces.distribution import IDistributionSet
> @@ -244,11 +248,15 @@
>                      for queue_item in queue_items:
>                          try:
>                              queue_item.realiseUpload(self.logger)

Um, I thought we use en_US as the standard language ... ;-)
Not for you to fix here, though. (realizeUpload)

> -                        except:
> -                            self.logger.error(
> -                                "Failure processing queue_item %d" % (
> -                                    queue_item.id), exc_info=True)
> -                            raise
> +                        except Exception:
> +                            message = "Failure processing queue_item %d" % (
> +                                queue_item.id)
> +                            properties = [('error-explanation', message)]
> +                            request = ScriptRequest(properties)
> +                            error_utility = ErrorReportingUtility()
> +                            error_utility.raising(sys.exc_info(), request)
> +                            self.logger.error('%s (%s)' % (message,
> +                                request.oopsid))

Please put the newline right after the opening "(", as I think that conforms more to our coding style (similar to a function call in looks).

                            self.logger.error('%s (%s)' % (
                                message, request.oopsid))


I have never had to file an OOPS before so I trust that this is the right way to do it.


>                          else:
>                              processed_queue_ids.append(queue_item.id)
>  
> 
> === modified file 'lib/lp/soyuz/tests/test_processaccepted.py'
> --- lib/lp/soyuz/tests/test_processaccepted.py	2010-02-23 10:30:56 +0000
> +++ lib/lp/soyuz/tests/test_processaccepted.py	2010-04-13 11:56:59 +0000
> @@ -3,13 +3,17 @@
>  
>  """Test process-accepted.py"""
>  
> +from cStringIO import StringIO
> +
>  from zope.component import getUtility
>  
>  from canonical.config import config
>  from canonical.launchpad.scripts import QuietFakeLogger
> +from canonical.launchpad.webapp.errorlog import ErrorReportingUtility
>  from canonical.testing import LaunchpadZopelessLayer
>  
>  from lp.registry.interfaces.distribution import IDistributionSet
> +from lp.registry.interfaces.series import SeriesStatus
>  from lp.soyuz.interfaces.archive import ArchivePurpose
>  from lp.soyuz.interfaces.publishing import PackagePublishingStatus
>  from lp.soyuz.scripts.processaccepted import ProcessAccepted
> @@ -39,13 +43,45 @@
>          script.txn = self.layer.txn
>          return script
>  
> -    def createWaitingAcceptancePackage(self, archive=None):
> +    def createWaitingAcceptancePackage(self, archive=None, distroseries=None,
> +            sourcename=None):
>          """Create some pending publications."""
>          if archive is None:
>              archive = getUtility(
> -                IDistributionSet).getByName('ubuntu').main_archive
> +                IDistributionSet).getByName('ubuntutest').main_archive

OK, we talked about this on IRC. I suggest that you put the "getUtility().getByName" call into the setUp method and use something like "self.distribution" here. If possible, use a factory-made distribution, if not, add a comment in setUp as to why you are using "ubuntutest".


> +        if sourcename is None:
> +            sourcename = self.test_package_name
>          return self.stp.getPubSource(
> -            archive=archive, sourcename=self.test_package_name, spr_only=True)
> +            archive=archive, sourcename=sourcename, distroseries=distroseries,
> +            spr_only=True)
> +
> +    def testRobustness(self):

Actually, TestsStyleGuide proposes to use underscores, like in function names. Also, I'd be more specific about what is going on. How about "test_oops_reporting"?

> +        """Test that a broken package doesn't block the publication of other 
> +        packages."""
> +        # Attempt to upload one source to a frozen series
> +        distroseries = self.factory.makeDistroSeries(
> +            distribution=getUtility(IDistributionSet).getByName('ubuntutest'))

What makes the series frozen, I wonder?

> +        broken_source = self.createWaitingAcceptancePackage(
> +            distroseries=distroseries, sourcename="notaccepted")
> +        distroseries.status = SeriesStatus.SUPPORTED

Oh, it was in a different state before (by default)? I am not very familiar with SeriesStatus, I admit ... May you could make this clearer with a comment or two?

> +        # Also upload some other things
> +        other_source = self.createWaitingAcceptancePackage()
> +        script = self.getScript([])
> +        script.main()
> +
> +        # The other source should be published now
> +        published_main = self.stp.ubuntutest.main_archive.getPublishedSources(
> +            name=self.test_package_name)
> +        self.assertEqual(published_main.count(), 1)
> +
> +        # And an oops should be filed for the first
> +        error_utility = ErrorReportingUtility()
> +        error_report = error_utility.getLastOopsReport()
> +        fp = StringIO()
> +        error_report.write(fp)
> +        error_text = fp.getvalue()
> +        self.failUnless("error-explanation=Failure processing queue_item" 
> +            in error_text)

I started out using "failUnless", too, but was pointed by a reviewer to the fact that "assert" is used far more in our code. You even see it in this file.

Also, it would be nice to be able to use "assertEqual" here to compare the error text completely. Can you not extract the id of the queue item to put into the string? Also, the line wrapping needs to be fixed.

        expected_error = (
            "error-explanation=Failure processing queue_item % d" % (
                queue_item_id))
        self.assertEqual(expected_error, error_text)

>  
>      def testAcceptCopyArchives(self):

Ah, I see you have had bad examples ... ;-) I think this is really "test_accept_copy_archives" ?


>          """Test that publications in a copy archive are accepted properly."""


Nothing really bad that needs serious fixing but please do consider applying my suggestions before landing this.

Cheers,
Henning