Modifications according to review comments

buildbot · Nov 22, 2014 · e39c005 · e39c005
1 parent 4358920
commit e39c005
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 15 deletions.
diff --git a/master/buildbot/buildslave/docker.py b/master/buildbot/buildslave/docker.py
@@ -41,7 +41,8 @@ def handle_stream_line(line):
     Output is a generator yield "Content", and then "content"
     """
     # XXX This necessary processing is probably a bug from docker-py,
-    # hence, might break if the bug is fixed ...
+    # hence, might break if the bug is fixed, i.e. we should get decoded JSON
+    # directly from the API.
     line = json.loads(line)
     if 'error' in line:
         content = "ERROR: " + line['error']
@@ -56,10 +57,8 @@ class DockerLatentBuildSlave(AbstractLatentBuildSlave):
     instance = None
 
     def __init__(self, name, password, docker_host, image=None, command=None,
-                 max_builds=None, notify_on_missing=None,
-                 missing_timeout=(60 * 20), build_wait_timeout=0,
-                 properties={}, locks=None, volumes=None, dockerfile=None,
-                 version=None, tls=None):
+                 volumes=None, dockerfile=None, version=None, tls=None,
+                 **kwargs):
 
         if not client:
             config.error("The python module 'docker-py' is needed to use a"
@@ -79,15 +78,16 @@ def __init__(self, name, password, docker_host, image=None, command=None,
             self.volumes.append(volume)
 
             ro = False
-            if bind.endswith(':ro'):
+            if bind.endswith(':ro') or bind.endswith(':rw'):
+                ro = bind[-2:] == 'ro'
                 bind = bind[:-3]
-                ro = True
             self.binds[volume] = {'bind': bind, 'ro': ro}
 
-        AbstractLatentBuildSlave.__init__(self, name, password, max_builds,
-                                          notify_on_missing or [],
-                                          missing_timeout, build_wait_timeout,
-                                          properties, locks)
+        # Set build_wait_timeout to 0 if not explicitely set: Starting a
+        # container is almost immediate, we can affort doing so for each build.
+        if 'build_wait_timeout' not in kwargs:
+            kwargs['build_wait_timeout'] = 0
+        AbstractLatentBuildSlave.__init__(self, name, password, **kwargs)
 
         self.image = image
         self.command = command or []

diff --git a/master/buildbot/test/unit/test_buildslave_docker.py b/master/buildbot/test/unit/test_buildslave_docker.py
@@ -73,10 +73,15 @@ def test_rw_volume(self):
         self.assertEqual(bs.volumes, ['/src/webapp'])
         self.assertEqual(bs.binds, {'/src/webapp': {'bind': '/opt/webapp', 'ro': False}})
 
-    def test_rw_ro_volume(self):
-        bs = self.ConcreteBuildSlave('bot', 'pass', 'tcp://1234:2375', 'slave', ['bin/bash'], volumes=['~/.bash_history:/.bash_history', '/src/webapp:/opt/webapp:ro'])
-        self.assertEqual(bs.volumes, ['~/.bash_history', '/src/webapp'])
-        self.assertEqual(bs.binds, {'~/.bash_history': {'bind': '/.bash_history', 'ro': False}, '/src/webapp': {'bind': '/opt/webapp', 'ro': True}})
+    def test__ro_rw_volume(self):
+        bs = self.ConcreteBuildSlave('bot', 'pass', 'tcp://1234:2375', 'slave', ['bin/bash'],
+                                     volumes=['~/.bash_history:/.bash_history',
+                                              '/src/webapp:/opt/webapp:ro',
+                                              '~:/backup:rw'])
+        self.assertEqual(bs.volumes, ['~/.bash_history', '/src/webapp', '~'])
+        self.assertEqual(bs.binds, {'~/.bash_history': {'bind': '/.bash_history', 'ro': False},
+                                    '/src/webapp': {'bind': '/opt/webapp', 'ro': True},
+                                    '~': {'bind': '/backup', 'ro': False}})
 
     @defer.inlineCallbacks
     def test_start_instance_image_no_version(self):