launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-revision-id into lp:launchpad.

Commit message:
Populate SnapBuild.revision_id from information returned by the builder.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1679157 in Launchpad itself: "Collect revision information for snap builds"
  https://bugs.launchpad.net/launchpad/+bug/1679157

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-revision-id/+merge/321690
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-revision-id into lp:launchpad.

=== modified file 'lib/lp/buildmaster/interactor.py'
--- lib/lp/buildmaster/interactor.py	2016-06-06 12:55:36 +0000
+++ lib/lp/buildmaster/interactor.py	2017-04-03 12:38:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -536,15 +536,9 @@
         # matches the DB, and this method isn't called unless the DB
         # says there's a job.
         builder_status = slave_status['builder_status']
-        if builder_status == 'BuilderStatus.BUILDING':
-            # Build still building, collect the logtail.
-            vitals.build_queue.logtail = str(
-                slave_status.get('logtail')).decode('UTF-8', errors='replace')
-            transaction.commit()
-        elif builder_status == 'BuilderStatus.ABORTING':
-            # Build is being aborted.
-            vitals.build_queue.logtail = (
-                "Waiting for slave process to be terminated")
+        if builder_status in (
+                'BuilderStatus.BUILDING', 'BuilderStatus.ABORTING'):
+            vitals.build_queue.collectStatus(slave_status)
             transaction.commit()
         elif builder_status == 'BuilderStatus.WAITING':
             # Build has finished. Delegate handling to the build itself.

=== modified file 'lib/lp/buildmaster/interfaces/buildqueue.py'
--- lib/lp/buildmaster/interfaces/buildqueue.py	2015-04-20 09:48:57 +0000
+++ lib/lp/buildmaster/interfaces/buildqueue.py	2017-04-03 12:38:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Build interfaces."""
@@ -86,6 +86,9 @@
     def markAsBuilding(builder):
         """Set this queue item to a 'building' state."""
 
+    def collectStatus(slave_status):
+        """Collect status information from the builder."""
+
     def suspend():
         """Suspend this waiting job, removing it from the active queue."""
 

=== modified file 'lib/lp/buildmaster/model/buildqueue.py'
--- lib/lp/buildmaster/model/buildqueue.py	2016-05-14 00:25:07 +0000
+++ lib/lp/buildmaster/model/buildqueue.py	2017-04-03 12:38:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -180,6 +180,17 @@
         if builder is not None:
             del get_property_cache(builder).currentjob
 
+    def collectStatus(self, slave_status):
+        """See `IBuildQueue`."""
+        builder_status = slave_status["builder_status"]
+        if builder_status == "BuilderStatus.ABORTING":
+            self.logtail = "Waiting for slave process to be terminated"
+        elif slave_status.get("logtail") is not None:
+            self.logtail = str(
+                slave_status.get("logtail")).decode("UTF-8", errors="replace")
+        self.specific_build.updateStatus(
+            self.specific_build.status, slave_status=slave_status)
+
     def suspend(self):
         """See `IBuildQueue`."""
         if self.status != BuildQueueStatus.WAITING:

=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2016-06-06 12:50:55 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2017-04-03 12:38:53 +0000
@@ -2,7 +2,7 @@
 # NOTE: The first line above must stay first; do not move the copyright
 # notice to the top.  See http://www.python.org/dev/peps/pep-0263/.
 #
-# Copyright 2009-2014 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the renovated slave scanner aka BuilddManager."""
@@ -113,11 +113,11 @@
         """
         super(TestSlaveScannerScan, self).setUp()
         # Creating the required chroots needed for dispatching.
-        test_publisher = make_publisher()
+        self.test_publisher = make_publisher()
         ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
         hoary = ubuntu.getSeries('hoary')
-        test_publisher.setUpDefaultDistroSeries(hoary)
-        test_publisher.addFakeChroots(db_only=True)
+        self.test_publisher.setUpDefaultDistroSeries(hoary)
+        self.test_publisher.addFakeChroots(db_only=True)
 
     def _resetBuilder(self, builder):
         """Reset the given builder and its job."""
@@ -139,8 +139,7 @@
         self.assertEqual(job.builder, builder)
         self.assertTrue(job.date_started is not None)
         self.assertEqual(job.status, BuildQueueStatus.RUNNING)
-        build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(job)
-        self.assertEqual(build.status, BuildStatus.BUILDING)
+        self.assertEqual(job.specific_build.status, BuildStatus.BUILDING)
         self.assertEqual(job.logtail, logtail)
 
     def _getScanner(self, builder_name=None, clock=None, builder_factory=None):
@@ -397,6 +396,38 @@
         self.assertEqual(2, fake_scan.call_count)
 
     @defer.inlineCallbacks
+    def test_scan_of_snap_build(self):
+        # Snap builds return additional status information, which the scan
+        # collects.
+        class SnapBuildingSlave(BuildingSlave):
+            revision_id = None
+
+            @defer.inlineCallbacks
+            def status(self):
+                status = yield super(SnapBuildingSlave, self).status()
+                status["revision_id"] = self.revision_id
+                defer.returnValue(status)
+
+        build = self.factory.makeSnapBuild(
+            distroarchseries=self.test_publisher.distroseries.architectures[0])
+        job = build.queueBuild()
+        builder = self.factory.makeBuilder(
+            processors=[job.processor], vm_host="fake_vm_host")
+        job.markAsBuilding(builder)
+        slave = SnapBuildingSlave(build_id="SNAPBUILD-%d" % build.id)
+        self.patch(BuilderSlave, "makeBuilderSlave", FakeMethod(slave))
+        transaction.commit()
+        scanner = self._getScanner(builder_name=builder.name)
+        yield scanner.scan()
+        self.assertBuildingJob(job, builder, logtail="This is a build log: 0")
+        self.assertIsNone(build.revision_id)
+        slave.revision_id = "dummy"
+        scanner = self._getScanner(builder_name=builder.name)
+        yield scanner.scan()
+        self.assertBuildingJob(job, builder, logtail="This is a build log: 1")
+        self.assertEqual("dummy", build.revision_id)
+
+    @defer.inlineCallbacks
     def _assertFailureCounting(self, builder_count, job_count,
                                expected_builder_count, expected_job_count):
         # If scan() fails with an exception, failure_counts should be

=== modified file 'lib/lp/snappy/browser/tests/test_snapbuild.py'
--- lib/lp/snappy/browser/tests/test_snapbuild.py	2017-01-27 12:44:41 +0000
+++ lib/lp/snappy/browser/tests/test_snapbuild.py	2017-04-03 12:38:53 +0000
@@ -77,6 +77,16 @@
         build_view = create_initialized_view(build, "+index")
         self.assertEqual([], build_view.files)
 
+    def test_revision_id(self):
+        build = self.factory.makeSnapBuild()
+        build.updateStatus(
+            BuildStatus.FULLYBUILT, slave_status={"revision_id": "dummy"})
+        build_view = create_initialized_view(build, "+index")
+        self.assertThat(build_view(), soupmatchers.HTMLContains(
+            soupmatchers.Tag(
+                "revision ID", "li", attrs={"id": "revision-id"},
+                text=re.compile(r"^\s*Revision: dummy\s*$"))))
+
     def test_store_upload_status_in_progress(self):
         build = self.factory.makeSnapBuild(status=BuildStatus.FULLYBUILT)
         getUtility(ISnapStoreUploadJobSource).create(build)

=== modified file 'lib/lp/snappy/interfaces/snapbuild.py'
--- lib/lp/snappy/interfaces/snapbuild.py	2017-02-27 18:46:38 +0000
+++ lib/lp/snappy/interfaces/snapbuild.py	2017-04-03 12:38:53 +0000
@@ -175,6 +175,12 @@
         title=_("The date when the build completed or is estimated to "
             "complete."), readonly=True)
 
+    revision_id = exported(TextLine(
+        title=_("Revision ID"), required=False, readonly=True,
+        description=_(
+            "The revision ID of the branch used for this build, if "
+            "available.")))
+
     store_upload_jobs = CollectionField(
         title=_("Store upload jobs for this build."),
         # Really ISnapStoreUploadJob.

=== modified file 'lib/lp/snappy/model/snapbuild.py'
--- lib/lp/snappy/model/snapbuild.py	2017-02-27 18:46:38 +0000
+++ lib/lp/snappy/model/snapbuild.py	2017-04-03 12:38:53 +0000
@@ -158,6 +158,8 @@
 
     status = DBEnum(name='status', enum=BuildStatus, allow_none=False)
 
+    revision_id = Unicode(name='revision_id')
+
     log_id = Int(name='log')
     log = Reference(log_id, 'LibraryFileAlias.id')
 
@@ -337,6 +339,10 @@
             status, builder=builder, slave_status=slave_status,
             date_started=date_started, date_finished=date_finished,
             force_invalid_transition=force_invalid_transition)
+        if slave_status is not None:
+            revision_id = slave_status.get("revision_id")
+            if revision_id is not None:
+                self.revision_id = unicode(revision_id)
         notify(SnapBuildStatusChangedEvent(self))
 
     def notify(self, extra_info=None):

=== modified file 'lib/lp/snappy/templates/snapbuild-index.pt'
--- lib/lp/snappy/templates/snapbuild-index.pt	2017-02-27 18:46:38 +0000
+++ lib/lp/snappy/templates/snapbuild-index.pt	2017-04-03 12:38:53 +0000
@@ -112,6 +112,9 @@
     </p>
 
     <ul>
+      <li id="revision-id" tal:condition="context/revision_id">
+        Revision: <span tal:replace="context/revision_id" />
+      </li>
       <li tal:condition="context/dependencies">
         Missing build dependencies: <em tal:content="context/dependencies"/>
      </li>

=== modified file 'lib/lp/snappy/tests/test_snapbuild.py'
--- lib/lp/snappy/tests/test_snapbuild.py	2017-03-20 00:03:52 +0000
+++ lib/lp/snappy/tests/test_snapbuild.py	2017-04-03 12:38:53 +0000
@@ -218,6 +218,15 @@
         self.factory.makeSnapFile(snapbuild=self.build)
         self.assertTrue(self.build.verifySuccessfulUpload())
 
+    def test_updateStatus_stores_revision_id(self):
+        # If the builder reports a revision_id, updateStatus saves it.
+        self.assertIsNone(self.build.revision_id)
+        self.build.updateStatus(BuildStatus.BUILDING, slave_status={})
+        self.assertIsNone(self.build.revision_id)
+        self.build.updateStatus(
+            BuildStatus.BUILDING, slave_status={"revision_id": "dummy"})
+        self.assertEqual("dummy", self.build.revision_id)
+
     def test_updateStatus_triggers_webhooks(self):
         # Updating the status of a SnapBuild triggers webhooks on the
         # corresponding Snap.