launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/daily-build-permissions into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #740567 in Launchpad itself: "CannotUploadToPPA: Signer has no upload rights to this PPA."
  https://bugs.launchpad.net/launchpad/+bug/740567

For more details, see:
https://code.launchpad.net/~abentley/launchpad/daily-build-permissions/+merge/60207

= Summary =
Fix bug #740567: CannotUploadToPPA: Signer has no upload rights to this PPA.

== Proposed fix ==
If CannotUploadToPPA is raised, no SourcePackageRecipeBuild is created.
A message is logged.
Daily builds on that recipe are disabled.
An email is sent to the recipe owner, explaining the situation.

== Pre-implementation notes ==
None

== Implementation details ==
Some driveby lint fixes, like unused variables.

== Tests ==
bin/test -t test_performDailyBuild_with_wrong_archive

== Demo and Q/A ==
Create a team PPA, with a team of which you are a member.  Create a daily build
recipe for that PPA.  Remove yourself from the team.  Run cronscripts/request_daily_builds.py

Daily builds of the recipe should be disabled, and you should receive an email explaining this.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/emailtemplates/wrong-ppa.txt
  lib/lp/code/model/tests/test_sourcepackagerecipe.py
  lib/lp/testing/__init__.py
  lib/lp/soyuz/model/archive.py
  lib/lp/code/model/sourcepackagerecipe.py
  lib/lp/code/interfaces/sourcepackagerecipe.py

./lib/canonical/launchpad/emailtemplates/wrong-ppa.txt
       1: Line exceeds 78 characters.
-- 
https://code.launchpad.net/~abentley/launchpad/daily-build-permissions/+merge/60207
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/daily-build-permissions into lp:launchpad.

=== added file 'lib/canonical/launchpad/emailtemplates/wrong-ppa.txt'
--- lib/canonical/launchpad/emailtemplates/wrong-ppa.txt	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/emailtemplates/wrong-ppa.txt	2011-05-06 15:39:00 +0000
@@ -0,0 +1,1 @@
+Daily builds of your recipe %(recipe_name)s have been disabled, because you do not have permission to upload to the archive %(archive_name)s.

=== modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py'
--- lib/lp/code/interfaces/sourcepackagerecipe.py	2011-04-11 01:30:37 +0000
+++ lib/lp/code/interfaces/sourcepackagerecipe.py	2011-05-06 15:39:00 +0000
@@ -162,7 +162,13 @@
 
     @export_write_operation()
     def performDailyBuild():
-        """Perform a build into the daily build archive."""
+        """Perform a build into the daily build archive.
+
+        If `CannotUploadToPPA` is raised, disables daily builds for that
+        recipe and sends an email to the recipe owner.
+
+        :return: A list of each new `SourcePackageRecipeBuild`.
+        """
 
     @export_read_operation()
     @operation_for_version("devel")

=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2011-03-23 16:28:51 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2011-05-06 15:39:00 +0000
@@ -15,6 +15,7 @@
     datetime,
     timedelta,
     )
+import logging
 
 from bzrlib.plugins.builder.recipe import RecipeParseError
 from lazr.delegates import delegates
@@ -73,7 +74,11 @@
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.model.distroseries import DistroSeries
 from lp.services.database.stormexpr import Greatest
-from lp.soyuz.interfaces.archive import IArchiveSet
+from lp.services.mail.basemailer import BaseMailer, RecipientReason
+from lp.soyuz.interfaces.archive import (
+    CannotUploadToPPA,
+    IArchiveSet,
+    )
 from lp.soyuz.model.archive import Archive
 
 # "Slam" a 400 response code onto RecipeParseError so that it will behave
@@ -121,6 +126,33 @@
     distroseries = Reference(distroseries_id, 'DistroSeries.id')
 
 
+class WrongArchiveMailer(BaseMailer):
+    """Mailer to inform recipe owner that daily build is disabled."""
+
+    def __init__(self, recipe):
+        """Constructor.
+
+        :param recipe: The recipe that is disabled.
+        """
+        header = RecipientReason.makeRationale('Owner', recipe.owner)
+        reason = '%(entity_is)s the owner of the recipe.'
+        r_reason = RecipientReason(recipe.owner, recipe.owner, header, reason)
+        super(WrongArchiveMailer, self).__init__(
+            'Daily builds disabled', 'wrong-ppa.txt',
+            {recipe.owner: r_reason}, 'noreply')
+        self.recipe = recipe
+
+    def _getTemplateParams(self, email, recipient):
+        """See `BaseMailer`"""
+        params = super(WrongArchiveMailer, self)._getTemplateParams(
+            email, recipient)
+        params.update({
+            'recipe_name': str(self.recipe),
+            'archive_name': self.recipe.daily_build_archive.unique_name,
+        })
+        return params
+
+
 class SourcePackageRecipe(Storm):
     """See `ISourcePackageRecipe` and `ISourcePackageRecipeSource`."""
 
@@ -303,15 +335,23 @@
         """See `ISourcePackageRecipe`."""
         builds = []
         self.is_stale = False
-        for distroseries in self.distroseries:
-            try:
-                build = self.requestBuild(
-                    self.daily_build_archive, self.owner,
-                    distroseries, PackagePublishingPocket.RELEASE)
-                builds.append(build)
-            except BuildAlreadyPending:
-                continue
-        return builds
+        try:
+            for distroseries in self.distroseries:
+                try:
+                    build = self.requestBuild(
+                        self.daily_build_archive, self.owner,
+                        distroseries, PackagePublishingPocket.RELEASE)
+                    builds.append(build)
+                except BuildAlreadyPending:
+                    continue
+            return builds
+        except CannotUploadToPPA:
+            self.build_daily = False
+            logging.getLogger().error(
+                'Owner of %s cannot upload to PPA %s.  Daily builds disabled.'
+                % (str(self), self.daily_build_archive.unique_name))
+            WrongArchiveMailer(self).sendAll()
+            return []
 
     @property
     def builds(self):
@@ -363,8 +403,7 @@
         for build in builds:
             result.append(
                 {"distroseries": build.distroseries.displayname,
-                 "archive": '%s/%s' %
-                           (build.archive.owner.name, build.archive.name)})
+                 "archive": build.archive.unique_name})
         return result
 
     @property

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2011-03-23 16:28:51 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2011-05-06 15:39:00 +0000
@@ -76,6 +76,7 @@
     TestCaseWithFactory,
     ws_object,
     )
+from lp.testing.mail_helpers import pop_notifications
 
 
 class TestSourcePackageRecipe(TestCaseWithFactory):
@@ -418,6 +419,24 @@
         recipe.requestBuild(archive, recipe.owner, series,
                 PackagePublishingPocket.RELEASE)
 
+    def test_performDailyBuild_with_wrong_archive(self):
+        recipe = self.factory.makeSourcePackageRecipe(
+            daily_build_archive=self.factory.makeArchive(),
+            build_daily=True)
+        with self.expectedLog(
+            'Owner of .*/.* cannot upload to .*/.*\.  Daily builds disabled'):
+            builds = recipe.performDailyBuild()
+        self.assertEqual([], builds)
+        self.assertFalse(recipe.build_daily)
+        (notification,) = pop_notifications()
+        self.assertEqual('Daily builds disabled', notification['subject'])
+        body = notification.get_payload(decode=True)
+        import re
+        self.assertTrue(re.match(
+            'Daily builds of your recipe .* have been disabled, because you'
+            ' do not have permission to upload to the archive .*\.',
+        body))
+
     def test_sourcepackagerecipe_description(self):
         """Ensure that the SourcePackageRecipe has a proper description."""
         description = u'The whoozits and whatzits.'
@@ -611,7 +630,7 @@
 
         build_info = []
         for archive in archives:
-            build = recipe.requestBuild(archive, person, distroseries)
+            recipe.requestBuild(archive, person, distroseries)
             build_info.insert(0, {
                 "distroseries": distroseries.displayname,
                 "archive": '%s/%s' %
@@ -1064,7 +1083,7 @@
         build_info = []
         for archive in archives:
             ws_archive = ws_object(launchpad, archive)
-            build = recipe.requestBuild(
+            recipe.requestBuild(
                 archive=ws_archive, distroseries=ws_distroseries,
                 pocket=PackagePublishingPocket.RELEASE.title)
             build_info.insert(0, {

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2011-05-05 13:01:50 +0000
+++ lib/lp/soyuz/model/archive.py	2011-05-06 15:39:00 +0000
@@ -330,6 +330,10 @@
         return self.displayname
 
     @property
+    def unique_name(self):
+        return '%s/%s' % (self.owner.name, self.name)
+
+    @property
     def is_ppa(self):
         """See `IArchive`."""
         return self.purpose == ArchivePurpose.PPA

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2011-05-03 04:39:43 +0000
+++ lib/lp/testing/__init__.py	2011-05-06 15:39:00 +0000
@@ -51,6 +51,7 @@
     'ZopeTestInSubProcess',
     ]
 
+from cStringIO import StringIO
 from contextlib import contextmanager
 from datetime import (
     datetime,
@@ -529,6 +530,19 @@
         expected_vector, observed_vector = zip(*args)
         return self.assertEqual(expected_vector, observed_vector)
 
+    @contextmanager
+    def expectedLog(self, regex):
+        """Expect a log to be written that matches the regex."""
+        output = StringIO()
+        handler = logging.StreamHandler(output)
+        logger = logging.getLogger()
+        logger.addHandler(handler)
+        try:
+            yield
+        finally:
+            logger.removeHandler(handler)
+        self.assertTrue(re.compile(regex).search(output.getvalue()))
+
     def pushConfig(self, section, **kwargs):
         """Push some key-value pairs into a section of the config.