Merge pull request #2126 from mnowster/1188-support-ro-options-in-volumes-from

1188 support ro options in volumes from

compose/project.py

@@ -17,8 +17,10 @@ from .legacy import check_for_legacy_containers
 from .service import ContainerNet
 from .service import ConvergenceStrategy
 from .service import Net
+from .service import parse_volume_from_spec
 from .service import Service
 from .service import ServiceNet
+from .service import VolumeFromSpec
 from .utils import parallel_execute
 
 
@@ -34,12 +36,18 @@ def sort_service_dicts(services):
     def get_service_names(links):
         return [link.split(':')[0] for link in links]
 
+    def get_service_names_from_volumes_from(volumes_from):
+        return [
+            parse_volume_from_spec(volume_from).source
+            for volume_from in volumes_from
+        ]
+
     def get_service_dependents(service_dict, services):
         name = service_dict['name']
         return [
             service for service in services
             if (name in get_service_names(service.get('links', [])) or
-                name in service.get('volumes_from', []) or
+                name in get_service_names_from_volumes_from(service.get('volumes_from', [])) or
                 name == get_service_name_from_net(service.get('net')))
         ]
 
@@ -176,20 +184,23 @@ class Project(object):
     def get_volumes_from(self, service_dict):
         volumes_from = []
         if 'volumes_from' in service_dict:
-            for volume_name in service_dict.get('volumes_from', []):
+            for volume_from_config in service_dict.get('volumes_from', []):
+                volume_from_spec = parse_volume_from_spec(volume_from_config)
+                # Get service
                 try:
-                    service = self.get_service(volume_name)
+                    service_name = self.get_service(volume_from_spec.source)
-                    volumes_from.append(service)
+                    volume_from_spec = VolumeFromSpec(service_name, volume_from_spec.mode)
                 except NoSuchService:
                     try:
-                        container = Container.from_id(self.client, volume_name)
+                        container_name = Container.from_id(self.client, volume_from_spec.source)
-                        volumes_from.append(container)
+                        volume_from_spec = VolumeFromSpec(container_name, volume_from_spec.mode)
                     except APIError:
                         raise ConfigurationError(
                             'Service "%s" mounts volumes from "%s", which is '
                             'not the name of a service or container.' % (
                                 service_dict['name'],
-                                volume_name))
+                                volume_from_spec.source))
+                volumes_from.append(volume_from_spec)
             del service_dict['volumes_from']
         return volumes_from
 

compose/service.py

@@ -83,6 +83,9 @@ class NoSuchImageError(Exception):
 VolumeSpec = namedtuple('VolumeSpec', 'external internal mode')
 
 
+VolumeFromSpec = namedtuple('VolumeFromSpec', 'source mode')
+
+
 ServiceName = namedtuple('ServiceName', 'project service number')
 
 
@@ -520,7 +523,7 @@ class Service(object):
         return [(service.name, alias) for service, alias in self.links]
 
     def get_volumes_from_names(self):
-        return [s.name for s in self.volumes_from if isinstance(s, Service)]
+        return [s.source.name for s in self.volumes_from if isinstance(s.source, Service)]
 
     def get_container_name(self, number, one_off=False):
         # TODO: Implement issue #652 here
@@ -560,16 +563,9 @@ class Service(object):
 
     def _get_volumes_from(self):
         volumes_from = []
-        for volume_source in self.volumes_from:
+        for volume_from_spec in self.volumes_from:
-            if isinstance(volume_source, Service):
+            volumes = build_volume_from(volume_from_spec)
-                containers = volume_source.containers(stopped=True)
+            volumes_from.extend(volumes)
-                if not containers:
-                    volumes_from.append(volume_source.create_container().id)
-                else:
-                    volumes_from.extend(map(attrgetter('id'), containers))
-
-            elif isinstance(volume_source, Container):
-                volumes_from.append(volume_source.id)
 
         return volumes_from
 
@@ -989,6 +985,38 @@ def parse_volume_spec(volume_config):
 
     return VolumeSpec(external, internal, mode)
 
+
+def build_volume_from(volume_from_spec):
+    """
+    volume_from can be either a service or a container. We want to return the
+    container.id and format it into a string complete with the mode.
+    """
+    if isinstance(volume_from_spec.source, Service):
+        containers = volume_from_spec.source.containers(stopped=True)
+        if not containers:
+            return ["{}:{}".format(volume_from_spec.source.create_container().id, volume_from_spec.mode)]
+
+        container = containers[0]
+        return ["{}:{}".format(container.id, volume_from_spec.mode)]
+    elif isinstance(volume_from_spec.source, Container):
+        return ["{}:{}".format(volume_from_spec.source.id, volume_from_spec.mode)]
+
+
+def parse_volume_from_spec(volume_from_config):
+    parts = volume_from_config.split(':')
+    if len(parts) > 2:
+        raise ConfigError("Volume %s has incorrect format, should be "
+                          "external:internal[:mode]" % volume_from_config)
+
+    if len(parts) == 1:
+        source = parts[0]
+        mode = 'rw'
+    else:
+        source, mode = parts
+
+    return VolumeFromSpec(source, mode)
+
+
 # Labels
 
 

docs/yml.md

@@ -346,11 +346,13 @@ should always begin with `.` or `..`.
 
 ### volumes_from
 
-Mount all of the volumes from another service or container.
+Mount all of the volumes from another service or container, optionally
+specifying read-only access(``ro``) or read-write(``rw``).
 
     volumes_from:
      - service_name
      - container_name
+     - service_name:rw
 
 ### cpu\_shares, cpuset, domainname, entrypoint, hostname, ipc, mac\_address, mem\_limit, memswap\_limit, privileged, read\_only, restart, stdin\_open, tty, user, volume\_driver, working\_dir
 

tests/integration/project_test.py

@@ -6,6 +6,7 @@ from compose.const import LABEL_PROJECT
 from compose.container import Container
 from compose.project import Project
 from compose.service import ConvergenceStrategy
+from compose.service import VolumeFromSpec
 
 
 def build_service_dicts(service_config):
@@ -72,7 +73,7 @@ class ProjectTest(DockerClientTestCase):
         )
         db = project.get_service('db')
         data = project.get_service('data')
-        self.assertEqual(db.volumes_from, [data])
+        self.assertEqual(db.volumes_from, [VolumeFromSpec(data, 'rw')])
 
     def test_volumes_from_container(self):
         data_container = Container.create(
@@ -93,7 +94,7 @@ class ProjectTest(DockerClientTestCase):
             client=self.client,
         )
         db = project.get_service('db')
-        self.assertEqual(db.volumes_from, [data_container])
+        self.assertEqual(db._get_volumes_from(), [data_container.id + ':rw'])
 
     def test_net_from_service(self):
         project = Project.from_dicts(

tests/integration/service_test.py

@@ -25,6 +25,7 @@ from compose.service import ConfigError
 from compose.service import ConvergencePlan
 from compose.service import Net
 from compose.service import Service
+from compose.service import VolumeFromSpec
 
 
 def create_and_start_container(service, **override_options):
@@ -272,12 +273,18 @@ class ServiceTest(DockerClientTestCase):
             command=["top"],
             labels={LABEL_PROJECT: 'composetest'},
         )
-        host_service = self.create_service('host', volumes_from=[volume_service, volume_container_2])
+        host_service = self.create_service(
+            'host',
+            volumes_from=[
+                VolumeFromSpec(volume_service, 'rw'),
+                VolumeFromSpec(volume_container_2, 'rw')
+            ]
+        )
         host_container = host_service.create_container()
         host_service.start_container(host_container)
-        self.assertIn(volume_container_1.id,
+        self.assertIn(volume_container_1.id + ':rw',
                       host_container.get('HostConfig.VolumesFrom'))
-        self.assertIn(volume_container_2.id,
+        self.assertIn(volume_container_2.id + ':rw',
                       host_container.get('HostConfig.VolumesFrom'))
 
     def test_execute_convergence_plan_recreate(self):

tests/unit/project_test.py

@@ -168,7 +168,7 @@ class ProjectTest(unittest.TestCase):
                 'volumes_from': ['aaa']
             }
         ], self.mock_client)
-        self.assertEqual(project.get_service('test')._get_volumes_from(), [container_id])
+        self.assertEqual(project.get_service('test')._get_volumes_from(), [container_id + ":rw"])
 
     def test_use_volumes_from_service_no_container(self):
         container_name = 'test_vol_1'
@@ -191,7 +191,7 @@ class ProjectTest(unittest.TestCase):
                 'volumes_from': ['vol']
             }
         ], self.mock_client)
-        self.assertEqual(project.get_service('test')._get_volumes_from(), [container_name])
+        self.assertEqual(project.get_service('test')._get_volumes_from(), [container_name + ":rw"])
 
     @mock.patch.object(Service, 'containers')
     def test_use_volumes_from_service_container(self, mock_return):
@@ -211,7 +211,7 @@ class ProjectTest(unittest.TestCase):
                 'volumes_from': ['vol']
             }
         ], None)
-        self.assertEqual(project.get_service('test')._get_volumes_from(), container_ids)
+        self.assertEqual(project.get_service('test')._get_volumes_from(), [container_ids[0] + ':rw'])
 
     def test_net_unset(self):
         project = Project.from_dicts('test', [

tests/unit/service_test.py

@@ -24,6 +24,7 @@ from compose.service import parse_repository_tag
 from compose.service import parse_volume_spec
 from compose.service import Service
 from compose.service import ServiceNet
+from compose.service import VolumeFromSpec
 
 
 class ServiceTest(unittest.TestCase):
@@ -75,9 +76,18 @@ class ServiceTest(unittest.TestCase):
         service = Service(
             'test',
             image='foo',
-            volumes_from=[mock.Mock(id=container_id, spec=Container)])
+            volumes_from=[VolumeFromSpec(mock.Mock(id=container_id, spec=Container), 'rw')])
 
-        self.assertEqual(service._get_volumes_from(), [container_id])
+        self.assertEqual(service._get_volumes_from(), [container_id + ':rw'])
+
+    def test_get_volumes_from_container_read_only(self):
+        container_id = 'aabbccddee'
+        service = Service(
+            'test',
+            image='foo',
+            volumes_from=[VolumeFromSpec(mock.Mock(id=container_id, spec=Container), 'ro')])
+
+        self.assertEqual(service._get_volumes_from(), [container_id + ':ro'])
 
     def test_get_volumes_from_service_container_exists(self):
         container_ids = ['aabbccddee', '12345']
@@ -86,9 +96,21 @@ class ServiceTest(unittest.TestCase):
             mock.Mock(id=container_id, spec=Container)
             for container_id in container_ids
         ]
-        service = Service('test', volumes_from=[from_service], image='foo')
+        service = Service('test', volumes_from=[VolumeFromSpec(from_service, 'rw')], image='foo')
 
-        self.assertEqual(service._get_volumes_from(), container_ids)
+        self.assertEqual(service._get_volumes_from(), [container_ids[0] + ":rw"])
+
+    def test_get_volumes_from_service_container_exists_with_flags(self):
+        for mode in ['ro', 'rw', 'z', 'rw,z', 'z,rw']:
+            container_ids = ['aabbccddee:' + mode, '12345:' + mode]
+            from_service = mock.create_autospec(Service)
+            from_service.containers.return_value = [
+                mock.Mock(id=container_id.split(':')[0], spec=Container)
+                for container_id in container_ids
+            ]
+            service = Service('test', volumes_from=[VolumeFromSpec(from_service, mode)], image='foo')
+
+            self.assertEqual(service._get_volumes_from(), [container_ids[0]])
 
     def test_get_volumes_from_service_no_container(self):
         container_id = 'abababab'
@@ -97,9 +119,9 @@ class ServiceTest(unittest.TestCase):
         from_service.create_container.return_value = mock.Mock(
             id=container_id,
             spec=Container)
-        service = Service('test', image='foo', volumes_from=[from_service])
+        service = Service('test', image='foo', volumes_from=[VolumeFromSpec(from_service, 'rw')])
 
-        self.assertEqual(service._get_volumes_from(), [container_id])
+        self.assertEqual(service._get_volumes_from(), [container_id + ':rw'])
         from_service.create_container.assert_called_once_with()
 
     def test_split_domainname_none(self):
@@ -357,7 +379,7 @@ class ServiceTest(unittest.TestCase):
             client=self.mock_client,
             net=ServiceNet(Service('other')),
             links=[(Service('one'), 'one')],
-            volumes_from=[Service('two')])
+            volumes_from=[VolumeFromSpec(Service('two'), 'rw')])
 
         config_dict = service.config_dict()
         expected = {