Merge lp:~wgrant/launchpad/refactor-slavestatus into lp:launchpad

[William Grant]

Factor out common IBuildFarmJobBehavior.slaveStatus behaviour into Builder.slaveStatus.

This branch shuffles common things from IBuildFarmJobBehavior.slaveStatus implementations up into Builder.slaveStatus. In particular build_id, build_status and logtail (all set by the base class on the slave) are extracted for all job types if present, in addition to builder_status which has always been extracted there. This vastly simplifies the slaveStatus methods on the IBFJBs.

[Brad Crittenden (bac)]

William thanks for the fix. The logic of what you have done looks good.

However, I don't like the idiom you've employed of mutating the status dictionary inside the slaveStatus methods and then returning it. If you didn't want to mutate the original dictionary then making a copy, changing it, and returning that copy would make sense. Otherwise just modifying the status dictionary in place and not returning it is a reasonable thing to do (and preferred barring no solid reason to do the former). But to mutate and then return would lead one to think the dictionary was not modified in place, which we know it was.

Changing the method to mutate will necessitate a small change to your new test code.

I'm marking this approved rather than needs fixing with the expectation you'll make that change. If you would prefer to discuss it further we can and I'd be eager to hear your thoughts.

As usual, I'm happy to land this for you when it is ready.

review: Approve (code)

[William Grant (wgrant)]

> William thanks for the fix. The logic of what you have done looks good.
>
> However, I don't like the idiom you've employed of mutating the status
> dictionary inside the slaveStatus methods and then returning it. If you
> didn't want to mutate the original dictionary then making a copy, changing it,
> and returning that copy would make sense. Otherwise just modifying the status
> dictionary in place and not returning it is a reasonable thing to do (and
> preferred barring no solid reason to do the former). But to mutate and then
> return would lead one to think the dictionary was not modified in place, which
> we know it was.

Thanks for the review, Brad. I was a little concerned about that too, but wasn't
entirely sure of the cleanest solution. I've adopted your suggestion, and also
renamed the method to updateSlaveStatus to more accurately reflect its purpose.

If you approve, please land this once PQM opens.

=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py	2010-03-12 16:50:45 +0000
+++ lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py	2010-03-31 04:20:47 +0000
@@ -48,13 +48,15 @@
         :param logger: A logger to be used to log diagnostic information.
         """
 
-    def slaveStatus(raw_slave_status):
-        """Return a dict of custom slave status values for this behavior.
+    def updateSlaveStatus(raw_slave_status, status):
+        """Update the slave status dict with custom values for this behavior.
 
         :param raw_slave_status: The value returned by the build slave's
            status() method.
-        :return: a dict of extra key/values to be included in the result
-            of IBuilder.slaveStatus().
+        :param status: A dict of the processed slave status values provided
+           by all types: builder_status, build_id, and optionally build_status
+           or logtail. This should have any behaviour-specific values
+           added to it.
         """
 
     def verifySlaveBuildID(slave_build_id):

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2010-03-26 01:52:07 +0000
+++ lib/lp/buildmaster/model/builder.py	2010-03-31 04:20:47 +0000
@@ -438,8 +438,17 @@
         status_sentence = self.slave.status()
 
         status = {'builder_status': status_sentence[0]}
-        status.update(
-            self.current_build_behavior.slaveStatus(status_sentence))
+
+        # Extract detailed status, ID and log information if present.
+        if status['builder_status'] == 'BuilderStatus.WAITING':
+            status['build_status'] = status_sentence[1]
+            status['build_id'] = status_sentence[2]
+        else:
+            status['build_id'] = status_sentence[1]
+            if status['builder_status'] == 'BuilderStatus.BUILDING':
+                status['logtail'] = status_sentence[2]
+
+        self.current_build_behavior.updateSlaveStatus(status_sentence, status)
         return status
 
     def slaveStatusSentence(self):

=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/model/buildfarmjobbehavior.py	2010-03-12 16:10:12 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py	2010-03-31 04:20:47 +0000
@@ -58,11 +58,11 @@
         """The default behavior is a no-op."""
         pass
 
-    def slaveStatus(self, raw_slave_status):
+    def updateSlaveStatus(self, raw_slave_status, status):
         """See `IBuildFarmJobBehavior`.
 
         The default behavior is that we don't add any extra values."""
-        return {}
+        pass
 
     def _helpVerifyBuildIDComponent(self, raw_id, item_type, finder):
         """Helper for verifying parts of a `BuildFarmJob` name.

=== modified file 'lib/lp/code/model/recipebuilder.py'
--- lib/lp/code/model/recipebuilder.py	2010-03-13 10:40:45 +0000
+++ lib/lp/code/model/recipebuilder.py	2010-03-31 04:20:47 +0000
@@ -140,35 +140,23 @@
                     (build.title, build.id, build.pocket.name,
                      build.distroseries.name))
 
-    def slaveStatus(self, raw_slave_status):
-        """Parse and return the binary build specific status info.
+    def updateSlaveStatus(self, raw_slave_status, status):
+        """Parse the recipe build specific status info into the status dict.
 
         This includes:
-        * build_id => string
-        * build_status => string or None
-        * logtail => string or None
         * filemap => dictionary or None
         * dependencies => string or None
         """
-        builder_status = raw_slave_status[0]
-        extra_info = {}
-        if builder_status == 'BuilderStatus.WAITING':
-            extra_info['build_status'] = raw_slave_status[1]
-            extra_info['build_id'] = raw_slave_status[2]
-            build_status_with_files = [
-                'BuildStatus.OK',
-                'BuildStatus.PACKAGEFAIL',
-                'BuildStatus.DEPFAIL',
-                ]
-            if extra_info['build_status'] in build_status_with_files:
-                extra_info['filemap'] = raw_slave_status[3]
-                extra_info['dependencies'] = raw_slave_status[4]
-        else:
-            extra_info['build_id'] = raw_slave_status[1]
-            if builder_status == 'BuilderStatus.BUILDING':
-                extra_info['logtail'] = raw_slave_status[2]
+        build_status_with_files = (
+            'BuildStatus.OK',
+            'BuildStatus.PACKAGEFAIL',
+            'BuildStatus.DEPFAIL',
+            )
+        if (status['builder_status'] == 'BuilderStatus.WAITING' and
+            status['build_status'] in build_status_with_files):
+            status['filemap'] = raw_slave_status[3]
+            status['dependencies'] = raw_slave_status[4]
 
-        return extra_info
 
     def getVerifiedBuild(self, raw_id):
         """See `IBuildFarmJobBehavior`."""

=== modified file 'lib/lp/soyuz/model/binarypackagebuildbehavior.py'
--- lib/lp/soyuz/model/binarypackagebuildbehavior.py	2010-01-22 04:01:17 +0000
+++ lib/lp/soyuz/model/binarypackagebuildbehavior.py	2010-03-31 04:20:47 +0000
@@ -129,35 +129,22 @@
                     (build.title, build.id, build.pocket.name,
                      build.distroseries.name))
 
-    def slaveStatus(self, raw_slave_status):
-        """Parse and return the binary build specific status info.
+    def updateSlaveStatus(self, raw_slave_status, status):
+        """Parse the binary build specific status info into the status dict.
 
         This includes:
-        * build_id => string
-        * build_status => string or None
-        * logtail => string or None
         * filemap => dictionary or None
         * dependencies => string or None
         """
-        builder_status = raw_slave_status[0]
-        extra_info = {}
-        if builder_status == 'BuilderStatus.WAITING':
-            extra_info['build_status'] = raw_slave_status[1]
-            extra_info['build_id'] = raw_slave_status[2]
-            build_status_with_files = [
-                'BuildStatus.OK',
-                'BuildStatus.PACKAGEFAIL',
-                'BuildStatus.DEPFAIL',
-                ]
-            if extra_info['build_status'] in build_status_with_files:
-                extra_info['filemap'] = raw_slave_status[3]
-                extra_info['dependencies'] = raw_slave_status[4]
-        else:
-            extra_info['build_id'] = raw_slave_status[1]
-            if builder_status == 'BuilderStatus.BUILDING':
-                extra_info['logtail'] = raw_slave_status[2]
-
-        return extra_info
+        build_status_with_files = (
+            'BuildStatus.OK',
+            'BuildStatus.PACKAGEFAIL',
+            'BuildStatus.DEPFAIL',
+            )
+        if (status['builder_status'] == 'BuilderStatus.WAITING' and
+            status['build_status'] in build_status_with_files):
+            status['filemap'] = raw_slave_status[3]
+            status['dependencies'] = raw_slave_status[4]
 
     def _cachePrivateSourceOnSlave(self, logger):
         """Ask the slave to download source files for a private build.

=== modified file 'lib/lp/translations/model/translationtemplatesbuildbehavior.py'
--- lib/lp/translations/model/translationtemplatesbuildbehavior.py	2010-03-24 12:26:27 +0000
+++ lib/lp/translations/model/translationtemplatesbuildbehavior.py	2010-03-31 04:20:47 +0000
@@ -93,21 +93,11 @@
             queue.addOrUpdateEntriesFromTarball(
                 tarball, False, branch.owner, productseries=series)
 
-    def slaveStatus(self, raw_slave_status):
+    def updateSlaveStatus(self, raw_slave_status, status):
         """See `IBuildFarmJobBehavior`."""
-        builder_status = raw_slave_status[0]
-
-        if builder_status == 'BuilderStatus.WAITING':
-            extra_info = {
-                'build_status': raw_slave_status[1],
-                'build_id': raw_slave_status[2],
-                }
-            if len(raw_slave_status) >= 3:
-                extra_info['filemap'] = raw_slave_status[3]
-            return extra_info
-        else:
-            # Nothing special to do for other states.
-            return {}
+        if status['builder_status'] == 'BuilderStatus.WAITING':
+            if len(raw_slave_status) >= 4:
+                status['filemap'] = raw_slave_status[3]
 
     def updateBuild_WAITING(self, queue_item, slave_status, logtail, logger):
         """Deal with a finished ("WAITING" state, perversely) build job.

=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py'
--- lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py	2010-03-25 23:19:10 +0000
+++ lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py	2010-03-31 04:20:47 +0000
@@ -185,7 +185,12 @@
         self.assertEqual(0, builder.cleanSlave.call_count)
         self.assertEqual(0, behavior._uploadTarball.call_count)
 
-        slave_status = behavior.slaveStatus(builder.slave.status())
+        slave_status = {
+            'builder_status': builder.slave.status()[0],
+            'build_status': builder.slave.status()[1],
+            'build_id': builder.slave.status()[2]
+            }
+        behavior.updateSlaveStatus(builder.slave.status(), slave_status)
         behavior.updateBuild_WAITING(queue_item, slave_status, None, logging)
 
         self.assertEqual(1, queue_item.destroySelf.call_count)