launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/builder-history-webservice into lp:launchpad.

Commit message:
Export Builder.getBuildRecords and DistroArchSeries.getBuildRecords.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1236819 in Launchpad itself: "Export builder history over API"
  https://bugs.launchpad.net/launchpad/+bug/1236819

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/builder-history-webservice/+merge/192820

== Summary ==

The only way to get at a builder's history programmatically is to screen-scrape the Builder:+history page.  This results in some pretty awful client-side code.  Other IHasBuildRecords objects export getBuildRecords methods; it seems like mainly an oversight that Builder doesn't.

== Proposed fix ==

Make IBuilder subclass IHasBuildRecords.  Testing this revealed that it didn't actually implement the IHasBuildRecords.getBuildRecords contract properly (missing pocket keyword argument), so I fixed that too and enforced it with tests.  We only get BinaryPackageBuild-type results because of the way that getBuildRecords is exported, but that's an awful lot better than nothing and for most of my purposes the other entries aren't all that interesting.

Along the way, I noticed that IDistroArchSeries also doesn't subclass IHasBuildRecords so in turn DistroArchSeries.getBuildRecords also isn't exported.  I fixed that too.

== LOC Rationale ==

+37, mostly new test code.  Worth it for the reduction in ridiculous HTML-scraping client code, I feel.

== Tests ==

bin/test -vvct lp.buildmaster.tests.test_builder -t lp.buildmaster.tests.test_webservice -t lp.soyuz.tests.test_binarypackagebuild

== Demo and Q/A ==

Call the relevant methods against qastaging and make sure they return sensible results.
-- 
https://code.launchpad.net/~cjwatson/launchpad/builder-history-webservice/+merge/192820
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/builder-history-webservice into lp:launchpad.

=== modified file 'lib/lp/buildmaster/configure.zcml'
--- lib/lp/buildmaster/configure.zcml	2013-02-04 06:55:17 +0000
+++ lib/lp/buildmaster/configure.zcml	2013-10-27 15:03:18 +0000
@@ -1,4 +1,4 @@
-<!-- Copyright 2010 Canonical Ltd.  This software is licensed under the
+<!-- Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
      GNU Affero General Public License version 3 (see the file LICENSE).
 -->
 
@@ -17,8 +17,6 @@
         class="lp.buildmaster.model.builder.Builder">
         <allow
             interface="lp.buildmaster.interfaces.builder.IBuilder"/>
-        <allow
-            interface="lp.soyuz.interfaces.buildrecords.IHasBuildRecords"/>
         <require
             permission="launchpad.Edit"
             set_schema="lp.buildmaster.interfaces.builder.IBuilder"/>

=== modified file 'lib/lp/buildmaster/interfaces/builder.py'
--- lib/lp/buildmaster/interfaces/builder.py	2013-10-02 05:47:26 +0000
+++ lib/lp/buildmaster/interfaces/builder.py	2013-10-27 15:03:18 +0000
@@ -50,6 +50,7 @@
     PersonChoice,
     Title,
     )
+from lp.soyuz.interfaces.buildrecords import IHasBuildRecords
 from lp.soyuz.interfaces.processor import IProcessor
 
 
@@ -84,7 +85,7 @@
     """The build slave has suffered an error and cannot be used."""
 
 
-class IBuilder(IHasOwner):
+class IBuilder(IHasBuildRecords, IHasOwner):
     """Build-slave information and state.
 
     Builder instance represents a single builder slave machine within the

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2013-10-10 06:31:30 +0000
+++ lib/lp/buildmaster/model/builder.py	2013-10-27 15:03:18 +0000
@@ -130,17 +130,17 @@
         self.builderok = False
         self.failnotes = reason
 
-    def getBuildRecords(self, build_state=None, name=None, arch_tag=None,
-                        user=None, binary_only=True):
+    def getBuildRecords(self, build_state=None, name=None, pocket=None,
+                        arch_tag=None, user=None, binary_only=True):
         """See IHasBuildRecords."""
         if binary_only:
             return getUtility(IBinaryPackageBuildSet).getBuildsForBuilder(
-                self.id, build_state, name, arch_tag, user)
+                self.id, build_state, name, pocket, arch_tag, user)
         else:
-            if arch_tag is not None or name is not None:
+            if arch_tag is not None or name is not None or pocket is not None:
                 raise IncompatibleArguments(
-                    "The 'arch_tag' and 'name' parameters can be used only "
-                    "with binary_only=True.")
+                    "The 'arch_tag', 'name', and 'pocket' parameters can be "
+                    "used only with binary_only=True.")
             return getUtility(IBuildFarmJobSet).getBuildsForBuilder(
                 self, status=build_state, user=user)
 

=== modified file 'lib/lp/buildmaster/tests/test_webservice.py'
--- lib/lp/buildmaster/tests/test_webservice.py	2013-09-11 06:05:44 +0000
+++ lib/lp/buildmaster/tests/test_webservice.py	2013-10-27 15:03:18 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the builders webservice ."""
@@ -65,3 +65,19 @@
         entry = self.webservice.get(
             api_url(builder), api_version='devel').jsonBody()
         self.assertEndsWith(entry['processor_link'], '/+processors/s1')
+
+    def test_getBuildRecords(self):
+        builder = self.factory.makeBuilder()
+        build = self.factory.makeBinaryPackageBuild(builder=builder)
+        build_title = build.title
+
+        logout()
+        results = self.webservice.named_get(
+            api_url(builder), 'getBuildRecords', pocket='Release',
+            api_version='devel').jsonBody()
+        self.assertEqual(
+            [build_title], [entry['title'] for entry in results['entries']])
+        results = self.webservice.named_get(
+            api_url(builder), 'getBuildRecords', pocket='Proposed',
+            api_version='devel').jsonBody()
+        self.assertEqual(0, len(results['entries']))

=== modified file 'lib/lp/soyuz/browser/tests/test_distroarchseries_webservice.py'
--- lib/lp/soyuz/browser/tests/test_distroarchseries_webservice.py	2013-06-24 15:51:00 +0000
+++ lib/lp/soyuz/browser/tests/test_distroarchseries_webservice.py	2013-10-27 15:03:18 +0000
@@ -58,6 +58,16 @@
         #See note above regarding testing of length of .entries
         self.assertEqual(1, len(ws_distroseries.architectures.entries))
 
+    def test_getBuildRecords(self):
+        das = self.factory.makeDistroArchSeries()
+        build = self.factory.makeBinaryPackageBuild(distroarchseries=das)
+        build_title = build.title
+        user = self.factory.makePerson()
+        launchpad = launchpadlib_for("testing", user)
+        ws_das = ws_object(launchpad, das)
+        self.assertEqual(
+            [build_title], [entry.title for entry in ws_das.getBuildRecords()])
+
     def test_setChroot_removeChroot_random_user(self):
         # Random users are not allowed to set or remove chroots.
         das = self.factory.makeDistroArchSeries()

=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml	2013-09-12 05:19:43 +0000
+++ lib/lp/soyuz/configure.zcml	2013-10-27 15:03:18 +0000
@@ -606,8 +606,6 @@
             <allow
                 interface="lp.soyuz.interfaces.distroarchseries.IDistroArchSeriesPublic"/>
             <allow
-                interface="lp.soyuz.interfaces.buildrecords.IHasBuildRecords"/>
-            <allow
                 interface="lp.soyuz.interfaces.publishing.ICanPublishPackages"/>
             <require
                 permission="launchpad.Admin"

=== modified file 'lib/lp/soyuz/interfaces/binarypackagebuild.py'
--- lib/lp/soyuz/interfaces/binarypackagebuild.py	2013-02-06 10:44:24 +0000
+++ lib/lp/soyuz/interfaces/binarypackagebuild.py	2013-10-27 15:03:18 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """BinaryPackageBuild interfaces."""
@@ -311,7 +311,7 @@
         :param builder: An optional `IBuilder`.
         """
 
-    def getBuildsForBuilder(builder_id, status=None, name=None,
+    def getBuildsForBuilder(builder_id, status=None, name=None, pocket=None,
                             arch_tag=None):
         """Return build records touched by a builder.
 
@@ -320,6 +320,8 @@
             will be returned.
         :param name: If name is provided, only builds which correspond to a
             matching sourcepackagename will be returned (SQL LIKE).
+        :param pocket: If pocket is provided, only builds for that pocket
+            will be returned.
         :param arch_tag: If arch_tag is provided, only builds for that
             architecture will be returned.
         :return: a `ResultSet` representing the requested builds.

=== modified file 'lib/lp/soyuz/interfaces/distroarchseries.py'
--- lib/lp/soyuz/interfaces/distroarchseries.py	2013-09-10 06:28:26 +0000
+++ lib/lp/soyuz/interfaces/distroarchseries.py	2013-10-27 15:03:18 +0000
@@ -40,6 +40,7 @@
 from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.role import IHasOwner
+from lp.soyuz.interfaces.buildrecords import IHasBuildRecords
 
 
 @error_status(httplib.BAD_REQUEST)
@@ -47,7 +48,7 @@
     """Raised when the sha1sum of an uploaded chroot does not match."""
 
 
-class IDistroArchSeriesPublic(IHasOwner):
+class IDistroArchSeriesPublic(IHasBuildRecords, IHasOwner):
     """Public attributes for a DistroArchSeries."""
 
     id = Attribute("Identifier")

=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
--- lib/lp/soyuz/model/binarypackagebuild.py	2013-08-28 12:03:57 +0000
+++ lib/lp/soyuz/model/binarypackagebuild.py	2013-10-27 15:03:18 +0000
@@ -946,7 +946,7 @@
                 clauses.append(SourcePackageName.name.is_in(name))
 
     def getBuildsForBuilder(self, builder_id, status=None, name=None,
-                            arch_tag=None, user=None):
+                            pocket=None, arch_tag=None, user=None):
         """See `IBinaryPackageBuildSet`."""
         # Circular. :(
         from lp.soyuz.model.archive import (
@@ -959,7 +959,7 @@
         origin = [Archive]
 
         self.handleOptionalParamsForBuildQueries(
-            clauses, origin, status, name, pocket=None, arch_tag=arch_tag)
+            clauses, origin, status, name, pocket, arch_tag=arch_tag)
 
         return IStore(BinaryPackageBuild).using(*origin).find(
             BinaryPackageBuild, *clauses).order_by(

=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
--- lib/lp/soyuz/tests/test_binarypackagebuild.py	2013-09-10 06:28:26 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuild.py	2013-10-27 15:03:18 +0000
@@ -17,6 +17,7 @@
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
 from lp.buildmaster.interfaces.packagebuild import IPackageBuild
 from lp.buildmaster.model.buildqueue import BuildQueue
+from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.job.model.job import Job
 from lp.services.webapp.interaction import ANONYMOUS
 from lp.services.webapp.interfaces import OAuthPermission
@@ -349,7 +350,8 @@
             self.factory.makeBinaryPackageBuild(
                 archive=self.ds.main_archive, distroarchseries=i386_das),
             self.factory.makeBinaryPackageBuild(
-                archive=self.ds.main_archive, distroarchseries=i386_das),
+                archive=self.ds.main_archive, distroarchseries=i386_das,
+                pocket=PackagePublishingPocket.PROPOSED),
             self.factory.makeBinaryPackageBuild(
                 archive=self.ds.main_archive, distroarchseries=hppa_das),
             ]
@@ -437,6 +439,15 @@
                                                     arch_tag="i386")
         self.assertContentEqual(builds, i386_builds)
 
+    def test_getBuildsForBuilder_by_pocket(self):
+        # Results can be filtered by pocket.
+        builds = self.build_set.getBuildsForBuilder(
+            self.builder.id, pocket=PackagePublishingPocket.RELEASE)
+        self.assertContentEqual([self.builds[0], self.builds[2]], builds)
+        builds = self.build_set.getBuildsForBuilder(
+            self.builder.id, pocket=PackagePublishingPocket.PROPOSED)
+        self.assertContentEqual([self.builds[1]], builds)
+
 
 class TestBinaryPackageBuildWebservice(TestCaseWithFactory):
     """Test cases for BinaryPackageBuild on the webservice.