From d0763862b14dd9eae00ae8aacede4dd17025ed5a Mon Sep 17 00:00:00 2001 From: Jimmy Zelinskie Date: Thu, 20 Nov 2014 14:36:22 -0500 Subject: [PATCH] Simple code review changes. I sneakily also added local-test.sh and renamed run-local to local-run.sh. --- buildman/basecomponent.py | 1 - buildman/buildcomponent.py | 9 +++++---- buildman/buildpack.py | 20 ++++++++++---------- buildman/enterprise_builder.py | 2 +- buildman/manager/basemanager.py | 2 +- buildman/server.py | 16 ++++++++-------- run-local.sh => local-run.sh | 0 local-test.sh | 2 ++ 8 files changed, 27 insertions(+), 25 deletions(-) rename run-local.sh => local-run.sh (100%) create mode 100755 local-test.sh diff --git a/buildman/basecomponent.py b/buildman/basecomponent.py index 5a1d04536..47781dff5 100644 --- a/buildman/basecomponent.py +++ b/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 diff --git a/buildman/buildcomponent.py b/buildman/buildcomponent.py index 3c57e912f..59c07042c 100644 --- a/buildman/buildcomponent.py +++ b/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 diff --git a/buildman/buildpack.py b/buildman/buildpack.py index cdc4d4b07..9892c65d3 100644 --- a/buildman/buildpack.py +++ b/buildman/buildpack.py @@ -17,13 +17,13 @@ class BuildPackage(object): def __init__(self, requests_file): self._mime_processors = { - 'application/zip': BuildPackage.__prepare_zip, - 'application/x-zip-compressed': BuildPackage.__prepare_zip, - 'text/plain': BuildPackage.__prepare_dockerfile, - 'application/octet-stream': BuildPackage.__prepare_dockerfile, - 'application/x-tar': BuildPackage.__prepare_tarball, - 'application/gzip': BuildPackage.__prepare_tarball, - 'application/x-gzip': BuildPackage.__prepare_tarball, + 'application/zip': BuildPackage._prepare_zip, + 'application/x-zip-compressed': BuildPackage._prepare_zip, + 'text/plain': BuildPackage._prepare_dockerfile, + 'application/octet-stream': BuildPackage._prepare_dockerfile, + 'application/x-tar': BuildPackage._prepare_tarball, + 'application/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 diff --git a/buildman/enterprise_builder.py b/buildman/enterprise_builder.py index cc9ce8432..88ea62b2d 100644 --- a/buildman/enterprise_builder.py +++ b/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, diff --git a/buildman/manager/basemanager.py b/buildman/manager/basemanager.py index c856159b5..1de6e24df 100644 --- a/buildman/manager/basemanager.py +++ b/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. """ diff --git a/buildman/server.py b/buildman/server.py index 96a793e01..0e2d7e050 100644 --- a/buildman/server.py +++ b/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): diff --git a/run-local.sh b/local-run.sh similarity index 100% rename from run-local.sh rename to local-run.sh diff --git a/local-test.sh b/local-test.sh new file mode 100755 index 000000000..c5cb9e283 --- /dev/null +++ b/local-test.sh @@ -0,0 +1,2 @@ +#!/bin/sh +TEST=true python -m unittest discover