Simple code review changes.

I sneakily also added local-test.sh and renamed run-local to
local-run.sh.

buildman/basecomponent.py

@@ -2,7 +2,6 @@ from autobahn.asyncio.wamp import ApplicationSession
 
 class BaseComponent(ApplicationSession):
   """ Base class for all registered component sessions in the server. """
-
   def __init__(self, config, **kwargs):
     ApplicationSession.__init__(self, config)
     self.server = None

buildman/buildcomponent.py

@@ -31,7 +31,6 @@ class ComponentStatus(object):
 
 class BuildComponent(BaseComponent):
   """ An application session component which conducts one (or more) builds. """
-
   def __init__(self, config, realm=None, token=None, **kwargs):
     self.expected_token = token
     self.builder_realm = realm
@@ -113,7 +112,7 @@ class BuildComponent(BaseComponent):
       base_image_information['username'] = build_config['pull_credentials'].get('username', '')
       base_image_information['password'] = build_config['pull_credentials'].get('password', '')
 
-    # Retrieve the repository's full name.
+    # Retrieve the repository's fully qualified name.
     repo = build_job.repo_build().repository
     repository_name = repo.namespace_user.username + '/' + repo.name
 
@@ -160,6 +159,7 @@ class BuildComponent(BaseComponent):
 
   @staticmethod
   def _process_pushpull_status(status_dict, current_phase, docker_data, images):
+    """ Processes the status of a push or pull by updating the provided status_dict and images. """
     if not docker_data:
       return
 
@@ -186,6 +186,7 @@ class BuildComponent(BaseComponent):
           BuildComponent._total_completion(images, max(len(images), num_images))
 
   def _on_log_message(self, phase, json_data):
+    """ Tails log messages and updates the build status. """
     # Parse any of the JSON data logged.
     docker_data = {}
     if json_data:
@@ -237,7 +238,7 @@ class BuildComponent(BaseComponent):
   def _build_failure(self, error_message, exception=None):
     """ Handles and logs a failed build. """
     self._build_status.set_error(error_message, {
-      'internal_error': exception.message if exception else None
+        'internal_error': exception.message if exception else None
     })
 
     build_id = self._current_job.repo_build().uuid
@@ -346,7 +347,7 @@ class BuildComponent(BaseComponent):
     # manager.
     if self._current_job is not None:
       if timed_out:
-        self._build_status.set_error('Build worker timed out. Build has been requeued')
+        self._build_status.set_error('Build worker timed out. Build has been requeued.')
 
       self.parent_manager.job_completed(self._current_job, BuildJobResult.INCOMPLETE, self)
       self._build_status = None

buildman/buildpack.py

@@ -17,13 +17,13 @@ class BuildPackage(object):
 
   def __init__(self, requests_file):
     self._mime_processors = {
-        'application/zip': BuildPackage.__prepare_zip,
+        'application/zip': BuildPackage._prepare_zip,
-        'application/x-zip-compressed': BuildPackage.__prepare_zip,
+        'application/x-zip-compressed': BuildPackage._prepare_zip,
-        'text/plain': BuildPackage.__prepare_dockerfile,
+        'text/plain': BuildPackage._prepare_dockerfile,
-        'application/octet-stream': BuildPackage.__prepare_dockerfile,
+        'application/octet-stream': BuildPackage._prepare_dockerfile,
-        'application/x-tar': BuildPackage.__prepare_tarball,
+        'application/x-tar': BuildPackage._prepare_tarball,
-        'application/gzip': BuildPackage.__prepare_tarball,
+        'application/gzip': BuildPackage._prepare_tarball,
-        'application/x-gzip': BuildPackage.__prepare_tarball,
+        'application/x-gzip': BuildPackage._prepare_tarball,
     }
 
     c_type = requests_file.headers['content-type']
@@ -57,7 +57,7 @@ class BuildPackage(object):
     return BuildPackage(buildpack_resource)
 
   @staticmethod
-  def __prepare_zip(request_file):
+  def _prepare_zip(request_file):
     build_dir = mkdtemp(prefix='docker-build-')
 
     # Save the zip file to temp somewhere
@@ -69,7 +69,7 @@ class BuildPackage(object):
     return build_dir
 
   @staticmethod
-  def __prepare_dockerfile(request_file):
+  def _prepare_dockerfile(request_file):
     build_dir = mkdtemp(prefix='docker-build-')
     dockerfile_path = os.path.join(build_dir, "Dockerfile")
     with open(dockerfile_path, 'w') as dockerfile:
@@ -78,7 +78,7 @@ class BuildPackage(object):
     return build_dir
 
   @staticmethod
-  def __prepare_tarball(request_file):
+  def _prepare_tarball(request_file):
     build_dir = mkdtemp(prefix='docker-build-')
 
     # Save the zip file to temp somewhere

buildman/enterprise_builder.py

@@ -12,7 +12,7 @@ if __name__ == '__main__':
   logging.basicConfig(level=logging.DEBUG)
 
   parser = argparse.ArgumentParser()
-  parser.add_argument("--host", type = str, default = "127.0.0.1", help = 'Host IP.')
+  parser.add_argument('--host', type=str, default='127.0.0.1', help='Host IP.')
   args = parser.parse_args()
 
   server = BuilderServer(app.config['SERVER_HOSTNAME'], dockerfile_build_queue, build_logs,

buildman/manager/basemanager.py

@@ -12,7 +12,7 @@ class BaseManager(object):
     """
     raise NotImplementedError
 
-  def schedule(self, build_job):
+  def schedule(self, build_job, loop):
     """ Schedules a queue item to be built. Returns True if the item was properly scheduled
         and False if all workers are busy.
     """

buildman/server.py

@@ -61,14 +61,14 @@ class BuilderServer(object):
     self._controller_app = controller_app
 
   def run(self, host):
-    logging.debug('Initializing the lifecycle manager')
+    LOGGER.debug('Initializing the lifecycle manager')
     self._lifecycle_manager.initialize()
 
-    logging.debug('Initializing all members of the event loop')
+    LOGGER.debug('Initializing all members of the event loop')
     loop = trollius.get_event_loop()
     trollius.Task(self._initialize(loop, host))
 
-    logging.debug('Starting server on port 8080, with controller on port 8181')
+    LOGGER.debug('Starting server on port 8080, with controller on port 8181')
     try:
       loop.run_forever()
     except KeyboardInterrupt:
@@ -77,17 +77,17 @@ class BuilderServer(object):
       loop.close()
 
   def close(self):
-    logging.debug('Requested server shutdown')
+    LOGGER.debug('Requested server shutdown')
     self._current_status = 'shutting_down'
     self._lifecycle_manager.shutdown()
     self._shutdown_event.wait()
-    logging.debug('Shutting down server')
+    LOGGER.debug('Shutting down server')
 
   def _register_component(self, realm, component_klass, **kwargs):
     """ Registers a component with the server. The component_klass must derive from
         BaseComponent.
     """
-    logging.debug('Registering component with realm %s', realm)
+    LOGGER.debug('Registering component with realm %s', realm)
 
     component = component_klass(types.ComponentConfig(realm=realm), realm=realm, **kwargs)
     component.server = self
@@ -101,7 +101,7 @@ class BuilderServer(object):
     return component
 
   def _unregister_component(self, component):
-    logging.debug('Unregistering component with realm %s and token %s',
+    LOGGER.debug('Unregistering component with realm %s and token %s',
                   component.builder_realm, component.expected_token)
 
     self._current_components.remove(component)
@@ -120,7 +120,7 @@ class BuilderServer(object):
     if self._current_status == 'shutting_down' and not self._job_count:
       self._shutdown_event.set()
 
-    # TODO:(jschorr) check for work here?
+    # TODO(jschorr): check for work here?
 
   @trollius.coroutine
   def _work_checker(self):

local-test.sh

@@ -0,0 +1,2 @@
+#!/bin/sh
+TEST=true python -m unittest discover