From 9e67eae311324fb4f9d307e6b4158caff0b32d3e Mon Sep 17 00:00:00 2001 From: Joffrey F Date: Thu, 21 Jan 2016 18:03:58 -0800 Subject: [PATCH 1/3] Match named volumes in service definitions with declared volumes Raise ConfigurationError for undeclared named volumes Test new behavior Signed-off-by: Joffrey F --- compose/config/types.py | 6 +++++ compose/project.py | 45 ++++++++++++++++++++++--------- tests/integration/project_test.py | 36 +++++++++++++++++++++++++ tests/unit/project_test.py | 43 +++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+), 12 deletions(-) diff --git a/compose/config/types.py b/compose/config/types.py index b872cba91..2bb2519d1 100644 --- a/compose/config/types.py +++ b/compose/config/types.py @@ -163,3 +163,9 @@ class VolumeSpec(namedtuple('_VolumeSpec', 'external internal mode')): def repr(self): external = self.external + ':' if self.external else '' return '{ext}{v.internal}:{v.mode}'.format(ext=external, v=self) + + @property + def is_named_volume(self): + return self.external and not ( + self.external.startswith('.') or self.external.startswith('/') + ) diff --git a/compose/project.py b/compose/project.py index 080c4c49f..7415e8242 100644 --- a/compose/project.py +++ b/compose/project.py @@ -74,6 +74,17 @@ class Project(object): if 'default' not in network_config: all_networks.append(project.default_network) + if config_data.volumes: + for vol_name, data in config_data.volumes.items(): + project.volumes.append( + Volume( + client=client, project=name, name=vol_name, + driver=data.get('driver'), + driver_opts=data.get('driver_opts'), + external_name=data.get('external_name') + ) + ) + for service_dict in config_data.services: if use_networking: networks = get_networks(service_dict, all_networks) @@ -86,6 +97,9 @@ class Project(object): volumes_from = get_volumes_from(project, service_dict) + if config_data.version == 2: + match_named_volumes(service_dict, project.volumes) + project.services.append( Service( client=client, @@ -95,23 +109,13 @@ class Project(object): links=links, net=net, volumes_from=volumes_from, - **service_dict)) + **service_dict) + ) project.networks += custom_networks if 'default' not in network_config and project.uses_default_network(): project.networks.append(project.default_network) - if config_data.volumes: - for vol_name, data in config_data.volumes.items(): - project.volumes.append( - Volume( - client=client, project=name, name=vol_name, - driver=data.get('driver'), - driver_opts=data.get('driver_opts'), - external_name=data.get('external_name') - ) - ) - return project @property @@ -473,6 +477,23 @@ def get_networks(service_dict, network_definitions): return networks +def match_named_volumes(service_dict, project_volumes): + for volume_spec in service_dict.get('volumes', []): + if volume_spec.is_named_volume: + declared_volume = next( + (v for v in project_volumes if v.name == volume_spec.external), + None + ) + if not declared_volume: + raise ConfigurationError( + 'Named volume "{0}" is used in service "{1}" but no' + ' declaration was found in the volumes section.'.format( + volume_spec.repr(), service_dict.get('name') + ) + ) + volume_spec._replace(external=declared_volume.full_name) + + def get_volumes_from(project, service_dict): volumes_from = service_dict.pop('volumes_from', None) if not volumes_from: diff --git a/tests/integration/project_test.py b/tests/integration/project_test.py index d3fbb71eb..aa0dd33f0 100644 --- a/tests/integration/project_test.py +++ b/tests/integration/project_test.py @@ -813,3 +813,39 @@ class ProjectTest(DockerClientTestCase): assert 'Volume {0} declared as external'.format( vol_name ) in str(e.exception) + + @v2_only() + def test_project_up_named_volumes_in_binds(self): + vol_name = '{0:x}'.format(random.getrandbits(32)) + full_vol_name = 'composetest_{0}'.format(vol_name) + + base_file = config.ConfigFile( + 'base.yml', + { + 'version': 2, + 'services': { + 'simple': { + 'image': 'busybox:latest', + 'command': 'top', + 'volumes': ['{0}:/data'.format(vol_name)] + }, + }, + 'volumes': { + vol_name: {'driver': 'local'} + } + + }) + config_details = config.ConfigDetails('.', [base_file]) + config_data = config.load(config_details) + project = Project.from_config( + name='composetest', config_data=config_data, client=self.client + ) + service = project.services[0] + self.assertEqual(service.name, 'simple') + volumes = service.options.get('volumes') + self.assertEqual(len(volumes), 1) + self.assertEqual(volumes[0].external, full_vol_name) + project.up() + engine_volumes = self.client.volumes() + self.assertIsNone(next(v for v in engine_volumes if v['Name'] == vol_name)) + self.assertIsNotNone(next(v for v in engine_volumes if v['Name'] == full_vol_name)) diff --git a/tests/unit/project_test.py b/tests/unit/project_test.py index ffd4455f3..f587d6970 100644 --- a/tests/unit/project_test.py +++ b/tests/unit/project_test.py @@ -7,8 +7,10 @@ import docker from .. import mock from .. import unittest +from compose.config import ConfigurationError from compose.config.config import Config from compose.config.types import VolumeFromSpec +from compose.config.types import VolumeSpec from compose.const import LABEL_SERVICE from compose.container import Container from compose.project import Project @@ -476,3 +478,44 @@ class ProjectTest(unittest.TestCase): ), ) self.assertEqual([c.id for c in project.containers()], ['1']) + + def test_undeclared_volume_v2(self): + config = Config( + version=2, + services=[ + { + 'name': 'web', + 'image': 'busybox:latest', + 'volumes': [VolumeSpec.parse('data0028:/data:ro')], + }, + ], + networks=None, + volumes=None, + ) + with self.assertRaises(ConfigurationError): + Project.from_config('composetest', config, None) + + config = Config( + version=2, + services=[ + { + 'name': 'web', + 'image': 'busybox:latest', + 'volumes': [VolumeSpec.parse('./data0028:/data:ro')], + }, + ], networks=None, volumes=None, + ) + Project.from_config('composetest', config, None) + + def test_undeclared_volume_v1(self): + config = Config( + version=1, + services=[ + { + 'name': 'web', + 'image': 'busybox:latest', + 'volumes': [VolumeSpec.parse('data0028:/data:ro')], + }, + ], networks=None, volumes=None, + ) + Project.from_config('composetest', config, None) From 48377a354f7527a60a8be3efb9ca17ec23816acd Mon Sep 17 00:00:00 2001 From: Joffrey F Date: Fri, 22 Jan 2016 16:05:21 -0800 Subject: [PATCH 2/3] is_named_volume also tests for home paths ~ Fix bug with VolumeSpec not being updated Fix integration test Signed-off-by: Joffrey F --- compose/config/types.py | 4 +--- compose/project.py | 7 +++++-- tests/integration/project_test.py | 7 ++++--- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/compose/config/types.py b/compose/config/types.py index 2bb2519d1..2e648e5a9 100644 --- a/compose/config/types.py +++ b/compose/config/types.py @@ -166,6 +166,4 @@ class VolumeSpec(namedtuple('_VolumeSpec', 'external internal mode')): @property def is_named_volume(self): - return self.external and not ( - self.external.startswith('.') or self.external.startswith('/') - ) + return self.external and not self.external.startswith(('.', '/', '~')) diff --git a/compose/project.py b/compose/project.py index 7415e8242..bed55925b 100644 --- a/compose/project.py +++ b/compose/project.py @@ -478,7 +478,8 @@ def get_networks(service_dict, network_definitions): def match_named_volumes(service_dict, project_volumes): - for volume_spec in service_dict.get('volumes', []): + service_volumes = service_dict.get('volumes', []) + for volume_spec in service_volumes: if volume_spec.is_named_volume: declared_volume = next( (v for v in project_volumes if v.name == volume_spec.external), @@ -491,7 +492,9 @@ def match_named_volumes(service_dict, project_volumes): volume_spec.repr(), service_dict.get('name') ) ) - volume_spec._replace(external=declared_volume.full_name) + service_volumes[service_volumes.index(volume_spec)] = ( + volume_spec._replace(external=declared_volume.full_name) + ) def get_volumes_from(project, service_dict): diff --git a/tests/integration/project_test.py b/tests/integration/project_test.py index aa0dd33f0..586f9444c 100644 --- a/tests/integration/project_test.py +++ b/tests/integration/project_test.py @@ -846,6 +846,7 @@ class ProjectTest(DockerClientTestCase): self.assertEqual(len(volumes), 1) self.assertEqual(volumes[0].external, full_vol_name) project.up() - engine_volumes = self.client.volumes() - self.assertIsNone(next(v for v in engine_volumes if v['Name'] == vol_name)) - self.assertIsNotNone(next(v for v in engine_volumes if v['Name'] == full_vol_name)) + engine_volumes = self.client.volumes()['Volumes'] + container = service.get_container() + assert [mount['Name'] for mount in container.get('Mounts')] == [full_vol_name] + assert next((v for v in engine_volumes if v['Name'] == vol_name), None) is None From 139c7f7830ede9ea66b0b12750ff7555ab4dca91 Mon Sep 17 00:00:00 2001 From: Joffrey F Date: Fri, 22 Jan 2016 17:42:24 -0800 Subject: [PATCH 3/3] Move named volumes matching to config validation phase Signed-off-by: Joffrey F --- compose/config/config.py | 6 ++++ compose/config/validation.py | 12 ++++++++ compose/project.py | 46 ++++++++++------------------- tests/unit/config/config_test.py | 50 ++++++++++++++++++++++++++++++++ tests/unit/project_test.py | 43 --------------------------- 5 files changed, 83 insertions(+), 74 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 72ad50af5..62e9929f0 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -25,6 +25,7 @@ from .types import parse_extra_hosts from .types import parse_restart_spec from .types import VolumeFromSpec from .types import VolumeSpec +from .validation import match_named_volumes from .validation import validate_against_fields_schema from .validation import validate_against_service_schema from .validation import validate_depends_on @@ -271,6 +272,11 @@ def load(config_details): config_details.working_dir, main_file, [file.get_service_dicts() for file in config_details.config_files]) + + if main_file.version >= 2: + for service_dict in service_dicts: + match_named_volumes(service_dict, volumes) + return Config(main_file.version, service_dicts, volumes, networks) diff --git a/compose/config/validation.py b/compose/config/validation.py index ecf8d4f92..5c2d69ec3 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -77,6 +77,18 @@ def format_boolean_in_environment(instance): return True +def match_named_volumes(service_dict, project_volumes): + service_volumes = service_dict.get('volumes', []) + for volume_spec in service_volumes: + if volume_spec.is_named_volume and volume_spec.external not in project_volumes: + raise ConfigurationError( + 'Named volume "{0}" is used in service "{1}" but no' + ' declaration was found in the volumes section.'.format( + volume_spec.repr(), service_dict.get('name') + ) + ) + + def validate_top_level_service_objects(filename, service_dicts): """Perform some high level validation of the service name and value. diff --git a/compose/project.py b/compose/project.py index bed55925b..48947eafe 100644 --- a/compose/project.py +++ b/compose/project.py @@ -42,7 +42,7 @@ class Project(object): self.use_networking = use_networking self.network_driver = network_driver self.networks = networks or [] - self.volumes = volumes or [] + self.volumes = volumes or {} def labels(self, one_off=False): return [ @@ -76,13 +76,11 @@ class Project(object): if config_data.volumes: for vol_name, data in config_data.volumes.items(): - project.volumes.append( - Volume( - client=client, project=name, name=vol_name, - driver=data.get('driver'), - driver_opts=data.get('driver_opts'), - external_name=data.get('external_name') - ) + project.volumes[vol_name] = Volume( + client=client, project=name, name=vol_name, + driver=data.get('driver'), + driver_opts=data.get('driver_opts'), + external_name=data.get('external_name') ) for service_dict in config_data.services: @@ -98,7 +96,13 @@ class Project(object): volumes_from = get_volumes_from(project, service_dict) if config_data.version == 2: - match_named_volumes(service_dict, project.volumes) + service_volumes = service_dict.get('volumes', []) + for volume_spec in service_volumes: + if volume_spec.is_named_volume: + declared_volume = project.volumes[volume_spec.external] + service_volumes[service_volumes.index(volume_spec)] = ( + volume_spec._replace(external=declared_volume.full_name) + ) project.services.append( Service( @@ -244,7 +248,7 @@ class Project(object): def initialize_volumes(self): try: - for volume in self.volumes: + for volume in self.volumes.values(): if volume.external: log.debug( 'Volume {0} declared as external. No new ' @@ -299,7 +303,7 @@ class Project(object): network.remove() def remove_volumes(self): - for volume in self.volumes: + for volume in self.volumes.values(): volume.remove() def initialize_networks(self): @@ -477,26 +481,6 @@ def get_networks(service_dict, network_definitions): return networks -def match_named_volumes(service_dict, project_volumes): - service_volumes = service_dict.get('volumes', []) - for volume_spec in service_volumes: - if volume_spec.is_named_volume: - declared_volume = next( - (v for v in project_volumes if v.name == volume_spec.external), - None - ) - if not declared_volume: - raise ConfigurationError( - 'Named volume "{0}" is used in service "{1}" but no' - ' declaration was found in the volumes section.'.format( - volume_spec.repr(), service_dict.get('name') - ) - ) - service_volumes[service_volumes.index(volume_spec)] = ( - volume_spec._replace(external=declared_volume.full_name) - ) - - def get_volumes_from(project, service_dict): volumes_from = service_dict.pop('volumes_from', None) if not volumes_from: diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 3c3c6326b..10f6aad86 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -523,6 +523,56 @@ class ConfigTest(unittest.TestCase): ] assert service_sort(service_dicts) == service_sort(expected) + def test_undeclared_volume_v2(self): + base_file = config.ConfigFile( + 'base.yaml', + { + 'version': 2, + 'services': { + 'web': { + 'image': 'busybox:latest', + 'volumes': ['data0028:/data:ro'], + }, + }, + } + ) + details = config.ConfigDetails('.', [base_file]) + with self.assertRaises(ConfigurationError): + config.load(details) + + base_file = config.ConfigFile( + 'base.yaml', + { + 'version': 2, + 'services': { + 'web': { + 'image': 'busybox:latest', + 'volumes': ['./data0028:/data:ro'], + }, + }, + } + ) + details = config.ConfigDetails('.', [base_file]) + config_data = config.load(details) + volume = config_data.services[0].get('volumes')[0] + assert not volume.is_named_volume + + def test_undeclared_volume_v1(self): + base_file = config.ConfigFile( + 'base.yaml', + { + 'web': { + 'image': 'busybox:latest', + 'volumes': ['data0028:/data:ro'], + }, + } + ) + details = config.ConfigDetails('.', [base_file]) + config_data = config.load(details) + volume = config_data.services[0].get('volumes')[0] + assert volume.external == 'data0028' + assert volume.is_named_volume + def test_config_valid_service_names(self): for valid_name in ['_', '-', '.__.', '_what-up.', 'what_.up----', 'whatup']: services = config.load( diff --git a/tests/unit/project_test.py b/tests/unit/project_test.py index f587d6970..ffd4455f3 100644 --- a/tests/unit/project_test.py +++ b/tests/unit/project_test.py @@ -7,10 +7,8 @@ import docker from .. import mock from .. import unittest -from compose.config import ConfigurationError from compose.config.config import Config from compose.config.types import VolumeFromSpec -from compose.config.types import VolumeSpec from compose.const import LABEL_SERVICE from compose.container import Container from compose.project import Project @@ -478,44 +476,3 @@ class ProjectTest(unittest.TestCase): ), ) self.assertEqual([c.id for c in project.containers()], ['1']) - - def test_undeclared_volume_v2(self): - config = Config( - version=2, - services=[ - { - 'name': 'web', - 'image': 'busybox:latest', - 'volumes': [VolumeSpec.parse('data0028:/data:ro')], - }, - ], - networks=None, - volumes=None, - ) - with self.assertRaises(ConfigurationError): - Project.from_config('composetest', config, None) - - config = Config( - version=2, - services=[ - { - 'name': 'web', - 'image': 'busybox:latest', - 'volumes': [VolumeSpec.parse('./data0028:/data:ro')], - }, - ], networks=None, volumes=None, - ) - Project.from_config('composetest', config, None) - - def test_undeclared_volume_v1(self): - config = Config( - version=1, - services=[ - { - 'name': 'web', - 'image': 'busybox:latest', - 'volumes': [VolumeSpec.parse('data0028:/data:ro')], - }, - ], networks=None, volumes=None, - ) - Project.from_config('composetest', config, None)