Merge pull request #2649 from dnephin/make_volumes_from_more_explicit

Make volumes from more explicit in V2 config format

compose/config/config.py

@@ -10,6 +10,7 @@ from collections import namedtuple
 
 import six
 import yaml
+from cached_property import cached_property
 
 from ..const import COMPOSEFILE_VERSIONS
 from .errors import CircularReference
@@ -119,8 +120,23 @@ class ConfigFile(namedtuple('_ConfigFile', 'filename config')):
     def from_filename(cls, filename):
         return cls(filename, load_yaml(filename))
 
-    def get_service_dicts(self, version):
-        return self.config if version == 1 else self.config.get('services', {})
+    @cached_property
+    def version(self):
+        if self.config is None:
+            return 1
+        version = self.config.get('version', 1)
+        if isinstance(version, dict):
+            log.warn("Unexpected type for field 'version', in file {} assuming "
+                     "version is the name of a service, and defaulting to "
+                     "Compose file version 1".format(self.filename))
+            return 1
+        return version
+
+    def get_service_dicts(self):
+        return self.config if self.version == 1 else self.config.get('services', {})
+
+    def get_volumes(self):
+        return {} if self.version == 1 else self.config.get('volumes', {})
 
 
 class Config(namedtuple('_Config', 'version services volumes')):
@@ -165,32 +181,24 @@ def find(base_dir, filenames):
         [ConfigFile.from_filename(f) for f in filenames])
 
 
-def get_config_version(config_details):
-    def get_version(config):
-        if config.config is None:
-            return 1
-        version = config.config.get('version', 1)
-        if isinstance(version, dict):
-            # in that case 'version' is probably a service name, so assume
-            # this is a legacy (version=1) file
-            version = 1
-        return version
-
+def validate_config_version(config_details):
     main_file = config_details.config_files[0]
     validate_top_level_object(main_file)
-    version = get_version(main_file)
     for next_file in config_details.config_files[1:]:
         validate_top_level_object(next_file)
-        next_file_version = get_version(next_file)
 
-        if version != next_file_version and next_file_version is not None:
+        if main_file.version != next_file.version:
             raise ConfigurationError(
-                "Version mismatch: main file {0} specifies version {1} but "
+                "Version mismatch: file {0} specifies version {1} but "
                 "extension file {2} uses version {3}".format(
-                    main_file.filename, version, next_file.filename, next_file_version
-                )
-            )
-    return version
+                    main_file.filename,
+                    main_file.version,
+                    next_file.filename,
+                    next_file.version))
+
+    if main_file.version not in COMPOSEFILE_VERSIONS:
+        raise ConfigurationError(
+            'Invalid Compose file version: {0}'.format(main_file.version))
 
 
 def get_default_config_files(base_dir):
@@ -239,45 +247,32 @@ def load(config_details):
 
     Return a fully interpolated, extended and validated configuration.
     """
-    version = get_config_version(config_details)
-    if version not in COMPOSEFILE_VERSIONS:
-        raise ConfigurationError('Invalid config version provided: {0}'.format(version))
+    validate_config_version(config_details)
 
-    processed_files = []
-    for config_file in config_details.config_files:
-        processed_files.append(
-            process_config_file(config_file, version=version)
-        )
+    processed_files = [
+        process_config_file(config_file)
+        for config_file in config_details.config_files
+    ]
     config_details = config_details._replace(config_files=processed_files)
 
-    if version == 1:
-        service_dicts = load_services(
-            config_details.working_dir, config_details.config_files,
-            version
-        )
-        volumes = {}
-    elif version == 2:
-        config_files = [
-            ConfigFile(f.filename, f.config.get('services', {}))
-            for f in config_details.config_files
-        ]
-        service_dicts = load_services(
-            config_details.working_dir, config_files, version
-        )
-        volumes = load_volumes(config_details.config_files)
-
-    return Config(version, service_dicts, volumes)
+    main_file = config_details.config_files[0]
+    volumes = load_volumes(config_details.config_files)
+    service_dicts = load_services(
+        config_details.working_dir,
+        main_file.filename,
+        [file.get_service_dicts() for file in config_details.config_files],
+        main_file.version)
+    return Config(main_file.version, service_dicts, volumes)
 
 
 def load_volumes(config_files):
     volumes = {}
     for config_file in config_files:
-        for name, volume_config in config_file.config.get('volumes', {}).items():
-            if volume_config is None:
-                volumes.update({name: {}})
+        for name, volume_config in config_file.get_volumes().items():
+            volumes[name] = volume_config or {}
+            if not volume_config:
                 continue
 
-            volumes.update({name: volume_config})
             external = volume_config.get('external')
             if external:
                 if len(volume_config.keys()) > 1:
@@ -296,8 +291,8 @@ def load_volumes(config_files):
     return volumes
 
 
-def load_services(working_dir, config_files, version):
-    def build_service(filename, service_name, service_dict):
+def load_services(working_dir, filename, service_configs, version):
+    def build_service(service_name, service_dict, service_names):
         service_config = ServiceConfig.with_abs_paths(
             working_dir,
             filename,
@@ -310,14 +305,18 @@ def load_services(working_dir, config_files, version):
         validate_against_service_schema(service_dict, service_config.name, version)
         validate_paths(service_dict)
 
-        service_dict = finalize_service(service_config._replace(config=service_dict))
+        service_dict = finalize_service(
+            service_config._replace(config=service_dict),
+            service_names,
+            version)
         service_dict['name'] = service_config.name
         return service_dict
 
-    def build_services(config_file):
+    def build_services(service_config):
+        service_names = service_config.keys()
         return sort_service_dicts([
-            build_service(config_file.filename, name, service_dict)
-            for name, service_dict in config_file.config.items()
+            build_service(name, service_dict, service_names)
+            for name, service_dict in service_config.items()
         ])
 
     def merge_services(base, override):
@@ -330,35 +329,35 @@ def load_services(working_dir, config_files, version):
             for name in all_service_names
         }
 
-    config_file = config_files[0]
-    for next_file in config_files[1:]:
-        config = merge_services(config_file.config, next_file.config)
-        config_file = config_file._replace(config=config)
+    service_config = service_configs[0]
+    for next_config in service_configs[1:]:
+        service_config = merge_services(service_config, next_config)
 
-    return build_services(config_file)
+    return build_services(service_config)
 
 
-def process_config_file(config_file, version, service_name=None):
-    service_dicts = config_file.get_service_dicts(version)
-    validate_top_level_service_objects(
-        config_file.filename, service_dicts
-    )
+def process_config_file(config_file, service_name=None):
+    service_dicts = config_file.get_service_dicts()
+    validate_top_level_service_objects(config_file.filename, service_dicts)
+
+    # TODO: interpolate config in volumes/network sections as well
     interpolated_config = interpolate_environment_variables(service_dicts)
-    if version == 2:
+
+    if config_file.version == 2:
         processed_config = dict(config_file.config)
         processed_config.update({'services': interpolated_config})
-    if version == 1:
+    if config_file.version == 1:
         processed_config = interpolated_config
-    validate_against_fields_schema(
-        processed_config, config_file.filename, version
-    )
+
+    config_file = config_file._replace(config=processed_config)
+    validate_against_fields_schema(config_file)
 
     if service_name and service_name not in processed_config:
         raise ConfigurationError(
             "Cannot extend service '{}' in {}: Service not found".format(
                 service_name, config_file.filename))
 
-    return config_file._replace(config=processed_config)
+    return config_file
 
 
 class ServiceExtendsResolver(object):
@@ -395,8 +394,7 @@ class ServiceExtendsResolver(object):
 
         extended_file = process_config_file(
             ConfigFile.from_filename(config_path),
-            version=self.version, service_name=service_name
-        )
+            service_name=service_name)
         service_config = extended_file.config[service_name]
         return config_path, service_config, service_name
 
@@ -510,7 +508,7 @@ def process_service(service_config):
     return service_dict
 
 
-def finalize_service(service_config):
+def finalize_service(service_config, service_names, version):
     service_dict = dict(service_config.config)
 
     if 'environment' in service_dict or 'env_file' in service_dict:
@@ -519,7 +517,9 @@ def finalize_service(service_config):
 
     if 'volumes_from' in service_dict:
         service_dict['volumes_from'] = [
-            VolumeFromSpec.parse(vf) for vf in service_dict['volumes_from']]
+            VolumeFromSpec.parse(vf, service_names, version)
+            for vf in service_dict['volumes_from']
+        ]
 
     if 'volumes' in service_dict:
         service_dict['volumes'] = [

compose/config/types.py

@@ -11,10 +11,16 @@ from compose.config.errors import ConfigurationError
 from compose.const import IS_WINDOWS_PLATFORM
 
 
-class VolumeFromSpec(namedtuple('_VolumeFromSpec', 'source mode')):
+class VolumeFromSpec(namedtuple('_VolumeFromSpec', 'source mode type')):
+
+    # TODO: drop service_names arg when v1 is removed
+    @classmethod
+    def parse(cls, volume_from_config, service_names, version):
+        func = cls.parse_v1 if version == 1 else cls.parse_v2
+        return func(service_names, volume_from_config)
 
     @classmethod
-    def parse(cls, volume_from_config):
+    def parse_v1(cls, service_names, volume_from_config):
         parts = volume_from_config.split(':')
         if len(parts) > 2:
             raise ConfigurationError(
@@ -27,7 +33,39 @@ class VolumeFromSpec(namedtuple('_VolumeFromSpec', 'source mode')):
         else:
             source, mode = parts
 
-        return cls(source, mode)
+        type = 'service' if source in service_names else 'container'
+        return cls(source, mode, type)
+
+    @classmethod
+    def parse_v2(cls, service_names, volume_from_config):
+        parts = volume_from_config.split(':')
+        if len(parts) > 3:
+            raise ConfigurationError(
+                "volume_from {} has incorrect format, should be one of "
+                "'<service name>[:<mode>]' or "
+                "'container:<container name>[:<mode>]'".format(volume_from_config))
+
+        if len(parts) == 1:
+            source = parts[0]
+            return cls(source, 'rw', 'service')
+
+        if len(parts) == 2:
+            if parts[0] == 'container':
+                type, source = parts
+                return cls(source, 'rw', type)
+
+            source, mode = parts
+            return cls(source, mode, 'service')
+
+        if len(parts) == 3:
+            type, source, mode = parts
+            if type not in ('service', 'container'):
+                raise ConfigurationError(
+                    "Unknown volumes_from type '{}' in '{}'".format(
+                        type,
+                        volume_from_config))
+
+        return cls(source, mode, type)
 
 
 def parse_restart_spec(restart_config):

compose/config/validation.py

@@ -105,8 +105,7 @@ def validate_top_level_service_objects(filename, service_dicts):
 def validate_top_level_object(config_file):
     if not isinstance(config_file.config, dict):
         raise ConfigurationError(
-            "Top level object in '{}' needs to be an object not '{}'. Check "
-            "that you have defined a service at the top level.".format(
+            "Top level object in '{}' needs to be an object not '{}'.".format(
                 config_file.filename,
                 type(config_file.config)))
 
@@ -291,13 +290,13 @@ def process_errors(errors, service_name=None):
     return '\n'.join(format_error_message(error, service_name) for error in errors)
 
 
-def validate_against_fields_schema(config, filename, version):
-    schema_filename = "fields_schema_v{0}.json".format(version)
+def validate_against_fields_schema(config_file):
+    schema_filename = "fields_schema_v{0}.json".format(config_file.version)
     _validate_against_schema(
-        config,
+        config_file.config,
         schema_filename,
         format_checker=["ports", "expose", "bool-value-in-mapping"],
-        filename=filename)
+        filename=config_file.filename)
 
 
 def validate_against_service_schema(config, service_name, version):

compose/project.py

@@ -60,7 +60,7 @@ class Project(object):
 
         for service_dict in config_data.services:
             links = project.get_links(service_dict)
-            volumes_from = project.get_volumes_from(service_dict)
+            volumes_from = get_volumes_from(project, service_dict)
             net = project.get_net(service_dict)
 
             project.services.append(
@@ -162,28 +162,6 @@ class Project(object):
             del service_dict['links']
         return links
 
-    def get_volumes_from(self, service_dict):
-        volumes_from = []
-        if 'volumes_from' in service_dict:
-            for volume_from_spec in service_dict.get('volumes_from', []):
-                # Get service
-                try:
-                    service = self.get_service(volume_from_spec.source)
-                    volume_from_spec = volume_from_spec._replace(source=service)
-                except NoSuchService:
-                    try:
-                        container = Container.from_id(self.client, volume_from_spec.source)
-                        volume_from_spec = volume_from_spec._replace(source=container)
-                    except APIError:
-                        raise ConfigurationError(
-                            'Service "%s" mounts volumes from "%s", which is '
-                            'not the name of a service or container.' % (
-                                service_dict['name'],
-                                volume_from_spec.source))
-                volumes_from.append(volume_from_spec)
-            del service_dict['volumes_from']
-        return volumes_from
-
     def get_net(self, service_dict):
         net = service_dict.pop('net', None)
         if not net:
@@ -486,6 +464,34 @@ def remove_links(service_dicts):
         del s['links']
 
 
+def get_volumes_from(project, service_dict):
+    volumes_from = service_dict.pop('volumes_from', None)
+    if not volumes_from:
+        return []
+
+    def build_volume_from(spec):
+        if spec.type == 'service':
+            try:
+                return spec._replace(source=project.get_service(spec.source))
+            except NoSuchService:
+                pass
+
+        if spec.type == 'container':
+            try:
+                container = Container.from_id(project.client, spec.source)
+                return spec._replace(source=container)
+            except APIError:
+                pass
+
+        raise ConfigurationError(
+            "Service \"{}\" mounts volumes from \"{}\", which is not the name "
+            "of a service or container.".format(
+                service_dict['name'],
+                spec.source))
+
+    return [build_volume_from(vf) for vf in volumes_from]
+
+
 class NoSuchService(Exception):
     def __init__(self, name):
         self.name = name

requirements.txt

@@ -1,4 +1,5 @@
 PyYAML==3.11
+cached-property==1.2.0
 dockerpty==0.3.4
 docopt==0.6.1
 enum34==1.0.4

setup.py

@@ -28,6 +28,7 @@ def find_version(*file_paths):
 
 
 install_requires = [
+    'cached-property >= 1.2.0, < 2',
     'docopt >= 0.6.1, < 0.7',
     'PyYAML >= 3.10, < 4',
     'requests >= 2.6.1, < 2.8',

tests/integration/project_test.py

@@ -81,7 +81,7 @@ class ProjectTest(DockerClientTestCase):
         )
         db = project.get_service('db')
         data = project.get_service('data')
-        self.assertEqual(db.volumes_from, [VolumeFromSpec(data, 'rw')])
+        self.assertEqual(db.volumes_from, [VolumeFromSpec(data, 'rw', 'service')])
 
     def test_volumes_from_container(self):
         data_container = Container.create(

tests/integration/service_test.py

@@ -224,8 +224,8 @@ class ServiceTest(DockerClientTestCase):
         host_service = self.create_service(
             'host',
             volumes_from=[
-                VolumeFromSpec(volume_service, 'rw'),
-                VolumeFromSpec(volume_container_2, 'rw')
+                VolumeFromSpec(volume_service, 'rw', 'service'),
+                VolumeFromSpec(volume_container_2, 'rw', 'container')
             ]
         )
         host_container = host_service.create_container()

tests/unit/config/config_test.py

@@ -19,7 +19,7 @@ from compose.const import IS_WINDOWS_PLATFORM
 from tests import mock
 from tests import unittest
 
-DEFAULT_VERSION = 2
+DEFAULT_VERSION = V2 = 2
 V1 = 1
 
 

tests/unit/config/sort_services_test.py

@@ -77,7 +77,7 @@ class SortServiceTest(unittest.TestCase):
             },
             {
                 'name': 'parent',
-                'volumes_from': [VolumeFromSpec('child', 'rw')]
+                'volumes_from': [VolumeFromSpec('child', 'rw', 'service')]
             },
             {
                 'links': ['parent'],
@@ -120,7 +120,7 @@ class SortServiceTest(unittest.TestCase):
             },
             {
                 'name': 'parent',
-                'volumes_from': [VolumeFromSpec('child', 'ro')]
+                'volumes_from': [VolumeFromSpec('child', 'ro', 'service')]
             },
             {
                 'name': 'child'
@@ -145,7 +145,7 @@ class SortServiceTest(unittest.TestCase):
             },
             {
                 'name': 'two',
-                'volumes_from': [VolumeFromSpec('one', 'rw')]
+                'volumes_from': [VolumeFromSpec('one', 'rw', 'service')]
             },
             {
                 'name': 'one'

tests/unit/config/types_test.py

@@ -5,8 +5,11 @@ import pytest
 
 from compose.config.errors import ConfigurationError
 from compose.config.types import parse_extra_hosts
+from compose.config.types import VolumeFromSpec
 from compose.config.types import VolumeSpec
 from compose.const import IS_WINDOWS_PLATFORM
+from tests.unit.config.config_test import V1
+from tests.unit.config.config_test import V2
 
 
 def test_parse_extra_hosts_list():
@@ -67,3 +70,45 @@ class TestVolumeSpec(object):
             "/opt/shiny/config",
             "ro"
         )
+
+
+class TestVolumesFromSpec(object):
+
+    services = ['servicea', 'serviceb']
+
+    def test_parse_v1_from_service(self):
+        volume_from = VolumeFromSpec.parse('servicea', self.services, V1)
+        assert volume_from == VolumeFromSpec('servicea', 'rw', 'service')
+
+    def test_parse_v1_from_container(self):
+        volume_from = VolumeFromSpec.parse('foo:ro', self.services, V1)
+        assert volume_from == VolumeFromSpec('foo', 'ro', 'container')
+
+    def test_parse_v1_invalid(self):
+        with pytest.raises(ConfigurationError):
+            VolumeFromSpec.parse('unknown:format:ro', self.services, V1)
+
+    def test_parse_v2_from_service(self):
+        volume_from = VolumeFromSpec.parse('servicea', self.services, V2)
+        assert volume_from == VolumeFromSpec('servicea', 'rw', 'service')
+
+    def test_parse_v2_from_service_with_mode(self):
+        volume_from = VolumeFromSpec.parse('servicea:ro', self.services, V2)
+        assert volume_from == VolumeFromSpec('servicea', 'ro', 'service')
+
+    def test_parse_v2_from_container(self):
+        volume_from = VolumeFromSpec.parse('container:foo', self.services, V2)
+        assert volume_from == VolumeFromSpec('foo', 'rw', 'container')
+
+    def test_parse_v2_from_container_with_mode(self):
+        volume_from = VolumeFromSpec.parse('container:foo:ro', self.services, V2)
+        assert volume_from == VolumeFromSpec('foo', 'ro', 'container')
+
+    def test_parse_v2_invalid_type(self):
+        with pytest.raises(ConfigurationError) as exc:
+            VolumeFromSpec.parse('bogus:foo:ro', self.services, V2)
+        assert "Unknown volumes_from type 'bogus'" in exc.exconly()
+
+    def test_parse_v2_invalid(self):
+        with pytest.raises(ConfigurationError):
+            VolumeFromSpec.parse('unknown:format:ro', self.services, V2)

tests/unit/project_test.py

@@ -165,10 +165,10 @@ class ProjectTest(unittest.TestCase):
             {
                 'name': 'test',
                 'image': 'busybox:latest',
-                'volumes_from': [VolumeFromSpec('aaa', 'rw')]
+                'volumes_from': [VolumeFromSpec('aaa', 'rw', 'container')]
             }
         ], None), self.mock_client)
-        self.assertEqual(project.get_service('test')._get_volumes_from(), [container_id + ":rw"])
+        assert project.get_service('test')._get_volumes_from() == [container_id + ":rw"]
 
     def test_use_volumes_from_service_no_container(self):
         container_name = 'test_vol_1'
@@ -188,10 +188,10 @@ class ProjectTest(unittest.TestCase):
             {
                 'name': 'test',
                 'image': 'busybox:latest',
-                'volumes_from': [VolumeFromSpec('vol', 'rw')]
+                'volumes_from': [VolumeFromSpec('vol', 'rw', 'service')]
             }
         ], None), self.mock_client)
-        self.assertEqual(project.get_service('test')._get_volumes_from(), [container_name + ":rw"])
+        assert project.get_service('test')._get_volumes_from() == [container_name + ":rw"]
 
     def test_use_volumes_from_service_container(self):
         container_ids = ['aabbccddee', '12345']
@@ -204,16 +204,17 @@ class ProjectTest(unittest.TestCase):
             {
                 'name': 'test',
                 'image': 'busybox:latest',
-                'volumes_from': [VolumeFromSpec('vol', 'rw')]
+                'volumes_from': [VolumeFromSpec('vol', 'rw', 'service')]
             }
         ], None), None)
         with mock.patch.object(Service, 'containers') as mock_return:
             mock_return.return_value = [
                 mock.Mock(id=container_id, spec=Container)
                 for container_id in container_ids]
-            self.assertEqual(
-                project.get_service('test')._get_volumes_from(),
-                [container_ids[0] + ':rw'])
+            assert (
+                project.get_service('test')._get_volumes_from() ==
+                [container_ids[0] + ':rw']
+            )
 
     def test_events(self):
         services = [Service(name='web'), Service(name='db')]

tests/unit/service_test.py

@@ -72,7 +72,11 @@ class ServiceTest(unittest.TestCase):
         service = Service(
             'test',
             image='foo',
-            volumes_from=[VolumeFromSpec(mock.Mock(id=container_id, spec=Container), 'rw')])
+            volumes_from=[
+                VolumeFromSpec(
+                    mock.Mock(id=container_id, spec=Container),
+                    'rw',
+                    'container')])
 
         self.assertEqual(service._get_volumes_from(), [container_id + ':rw'])
 
@@ -81,7 +85,11 @@ class ServiceTest(unittest.TestCase):
         service = Service(
             'test',
             image='foo',
-            volumes_from=[VolumeFromSpec(mock.Mock(id=container_id, spec=Container), 'ro')])
+            volumes_from=[
+                VolumeFromSpec(
+                    mock.Mock(id=container_id, spec=Container),
+                    'ro',
+                    'container')])
 
         self.assertEqual(service._get_volumes_from(), [container_id + ':ro'])
 
@@ -92,7 +100,10 @@ class ServiceTest(unittest.TestCase):
             mock.Mock(id=container_id, spec=Container)
             for container_id in container_ids
         ]
-        service = Service('test', volumes_from=[VolumeFromSpec(from_service, 'rw')], image='foo')
+        service = Service(
+            'test',
+            volumes_from=[VolumeFromSpec(from_service, 'rw', 'service')],
+            image='foo')
 
         self.assertEqual(service._get_volumes_from(), [container_ids[0] + ":rw"])
 
@@ -104,7 +115,10 @@ class ServiceTest(unittest.TestCase):
                 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')
+            service = Service(
+                'test',
+                volumes_from=[VolumeFromSpec(from_service, mode, 'service')],
+                image='foo')
 
             self.assertEqual(service._get_volumes_from(), [container_ids[0]])
 
@@ -115,7 +129,10 @@ class ServiceTest(unittest.TestCase):
         from_service.create_container.return_value = mock.Mock(
             id=container_id,
             spec=Container)
-        service = Service('test', image='foo', volumes_from=[VolumeFromSpec(from_service, 'rw')])
+        service = Service(
+            'test',
+            image='foo',
+            volumes_from=[VolumeFromSpec(from_service, 'rw', 'service')])
 
         self.assertEqual(service._get_volumes_from(), [container_id + ':rw'])
         from_service.create_container.assert_called_once_with()
@@ -391,7 +408,7 @@ class ServiceTest(unittest.TestCase):
             client=self.mock_client,
             net=ServiceNet(Service('other')),
             links=[(Service('one'), 'one')],
-            volumes_from=[VolumeFromSpec(Service('two'), 'rw')])
+            volumes_from=[VolumeFromSpec(Service('two'), 'rw', 'service')])
 
         config_dict = service.config_dict()
         expected = {