launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/no-oops-request-daily-build into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/no-oops-request-daily-build/+merge/119654

Currently, if you request a daily build of a recipe that contains an obsolete series (such as Maverick), you get an OOPS since the code raises a BuildNotAllowedForDistro exception, which is not caught.

I have changed ISourcePackageRecipe.performDailyBuild() to filter out distroseries which can't be built, and have added a helper method which the view calls to add a warning to the notification. 
-- 
https://code.launchpad.net/~stevenk/launchpad/no-oops-request-daily-build/+merge/119654
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/no-oops-request-daily-build into lp:launchpad.

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2012-08-10 04:48:36 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2012-08-15 01:19:20 +0000
@@ -11,6 +11,7 @@
     'SourcePackageRecipeEditView',
     'SourcePackageRecipeNavigationMenu',
     'SourcePackageRecipeRequestBuildsView',
+    'SourcePackageRecipeRequestDailyBuildView',
     'SourcePackageRecipeView',
     ]
 
@@ -366,7 +367,8 @@
         return builds
 
 
-def new_builds_notification_text(builds, already_pending=None):
+def new_builds_notification_text(builds, already_pending=None,
+                                 contains_unbuildable=False):
     nr_builds = len(builds)
     if not nr_builds:
         builds_text = "All requested recipe builds are already queued."
@@ -376,6 +378,9 @@
         builds_text = "%d new recipe builds have been queued." % nr_builds
     if nr_builds > 0 and already_pending:
         builds_text = "<p>%s</p>%s" % (builds_text, already_pending)
+    if contains_unbuildable:
+        builds_text = ("%s<p>The recipe contains an obsolete distroseries, "
+            "which has been skipped.</p>" % builds_text)
     return structured(builds_text)
 
 
@@ -554,9 +559,12 @@
                     "../templates/sourcepackagerecipe-builds.pt")
             return template(self)
         else:
+            contains_unbuildable = recipe.containsUnbuildableSeries(
+                recipe.daily_build_archive)
             self.next_url = canonical_url(recipe)
             self.request.response.addNotification(
-                    new_builds_notification_text(builds))
+                new_builds_notification_text(
+                    builds, contains_unbuildable=contains_unbuildable))
 
     @property
     def builds(self):

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2012-08-13 19:34:10 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2012-08-15 01:19:20 +0000
@@ -1,6 +1,5 @@
 # Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
-# pylint: disable-msg=F0401,E1002
 
 """Tests for the source package recipe view classes and templates."""
 
@@ -1417,6 +1416,24 @@
             "Secret PPA is disabled.",
             harness.view.request.notifications[0].message)
 
+    def test_request_daily_builds_obsolete_series(self):
+        # Requesting a daily build with an obsolete series gives a warning.
+        recipe = self.factory.makeSourcePackageRecipe(
+            owner=self.chef, daily_build_archive=self.ppa,
+            name=u'julia', is_stale=True, build_daily=True)
+        warty = self.factory.makeSourcePackageRecipeDistroseries()
+        hoary = self.factory.makeSourcePackageRecipeDistroseries(name='hoary')
+        with person_logged_in(self.chef):
+            recipe.updateSeries((warty, hoary))
+        removeSecurityProxy(warty).status = SeriesStatus.OBSOLETE
+        harness = LaunchpadFormHarness(
+            recipe, SourcePackageRecipeRequestDailyBuildView)
+        harness.submit('build', {})
+        self.assertEqual(
+            '1 new recipe build has been queued.<p>The recipe contains an '
+            'obsolete distroseries, which has been skipped.</p>',
+            harness.view.request.notifications[0].message)
+
     def test_request_builds_page(self):
         """Ensure the +request-builds page is sane."""
         recipe = self.makeRecipe()

=== modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py'
--- lib/lp/code/interfaces/sourcepackagerecipe.py	2011-12-24 16:54:44 +0000
+++ lib/lp/code/interfaces/sourcepackagerecipe.py	2012-08-15 01:19:20 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0211,E0213,F0401
@@ -161,6 +161,10 @@
             able to upload to the archive.
         """
 
+    def containsUnbuildableSeries(archive):
+        """Does the recipe contain series that can not be built into.
+        """
+
     @export_write_operation()
     def performDailyBuild():
         """Perform a build into the daily build archive."""

=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2011-12-30 06:14:56 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2012-08-15 01:19:20 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=F0401,W1001
@@ -281,6 +281,10 @@
         return SourcePackageRecipeBuild.getRecentBuilds(
             requester, self, distroseries).count() >= 5
 
+    def containsUnbuildableSeries(self, archive):
+        buildable_distros = set(get_buildable_distroseries_set(archive.owner))
+        return len(set(self.distroseries).difference(buildable_distros)) >= 1
+
     def requestBuild(self, archive, requester, distroseries,
                      pocket=PackagePublishingPocket.RELEASE,
                      manual=False):
@@ -321,7 +325,10 @@
         """See `ISourcePackageRecipe`."""
         builds = []
         self.is_stale = False
-        for distroseries in self.distroseries:
+        buildable_distros = set(get_buildable_distroseries_set(
+            self.daily_build_archive.owner))
+        build_for = set(self.distroseries).intersection(buildable_distros)
+        for distroseries in build_for:
             try:
                 build = self.requestBuild(
                     self.daily_build_archive, self.owner,

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2012-05-31 03:54:13 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2012-08-15 01:19:20 +0000
@@ -52,6 +52,7 @@
 from lp.code.tests.helpers import recipe_parser_newest_version
 from lp.registry.enums import InformationType
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.registry.interfaces.series import SeriesStatus
 from lp.services.database.bulk import load_referencing
 from lp.services.database.constants import UTC_NOW
 from lp.services.job.interfaces.job import (
@@ -69,7 +70,6 @@
     )
 from lp.testing import (
     ANONYMOUS,
-    feature_flags,
     launchpadlib_for,
     login,
     login_person,
@@ -89,10 +89,6 @@
 
     layer = DatabaseFunctionalLayer
 
-    def setUp(self):
-        super(TestSourcePackageRecipe, self).setUp()
-        self.useContext(feature_flags())
-
     def test_implements_interface(self):
         """SourcePackageRecipe implements ISourcePackageRecipe."""
         recipe = self.factory.makeSourcePackageRecipe()
@@ -738,6 +734,28 @@
         self.assertEqual([], list(recipe.completed_builds))
         self.assertEqual([], list(recipe.pending_builds))
 
+    def test_containsUnbuildableSeries(self):
+        recipe = self.factory.makeSourcePackageRecipe()
+        self.assertFalse(recipe.containsUnbuildableSeries(
+            recipe.daily_build_archive))
+
+    def test_containsUnbuildableSeries_with_obsolete_series(self):
+        recipe = self.factory.makeSourcePackageRecipe()
+        warty = self.factory.makeSourcePackageRecipeDistroseries()
+        removeSecurityProxy(warty).status = SeriesStatus.OBSOLETE
+        self.assertTrue(recipe.containsUnbuildableSeries(
+            recipe.daily_build_archive))
+
+    def test_performDailyBuild_filters_obsolete_series(self):
+        recipe = self.factory.makeSourcePackageRecipe()
+        warty = self.factory.makeSourcePackageRecipeDistroseries()
+        hoary = self.factory.makeSourcePackageRecipeDistroseries(name='hoary')
+        with person_logged_in(recipe.owner):
+            recipe.updateSeries((warty, hoary))
+        removeSecurityProxy(warty).status = SeriesStatus.OBSOLETE
+        builds = recipe.performDailyBuild()
+        self.assertEqual([build.recipe for build in builds], [recipe])
+
 
 class TestRecipeBranchRoundTripping(TestCaseWithFactory):