From 9d2b955e78951673773892687a1481316a06eb15 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 12 Oct 2015 11:39:49 +0100 Subject: [PATCH 1/7] Move ConfigError out of service.py It's a class of error that we need shared, as it'll be used by config.py Signed-off-by: Mazz Mosley --- compose/config/errors.py | 4 ++++ compose/service.py | 5 +---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/compose/config/errors.py b/compose/config/errors.py index 037b7ec84d7..2ef45b2980a 100644 --- a/compose/config/errors.py +++ b/compose/config/errors.py @@ -1,3 +1,7 @@ +class ConfigError(ValueError): + pass + + class ConfigurationError(Exception): def __init__(self, msg): self.msg = msg diff --git a/compose/service.py b/compose/service.py index 044b34ad5e7..1f6836c63e5 100644 --- a/compose/service.py +++ b/compose/service.py @@ -18,6 +18,7 @@ from . import __version__ from .config import DOCKER_CONFIG_KEYS from .config import merge_environment +from .config.errors import ConfigError from .config.validation import VALID_NAME_CHARS from .const import DEFAULT_TIMEOUT from .const import IS_WINDOWS_PLATFORM @@ -67,10 +68,6 @@ def __init__(self, service, reason): self.reason = reason -class ConfigError(ValueError): - pass - - class NeedsBuildError(Exception): def __init__(self, service): self.service = service From 094423c7792352a79217417996937c3c18a1414a Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 12 Oct 2015 11:41:47 +0100 Subject: [PATCH 2/7] Expand volume paths early After a config file has been fully interpolated, we expand all of the volume paths. This helps ensure that splitdrive works correctly, as it won't be dealing with any relative paths. Signed-off-by: Mazz Mosley --- compose/config/config.py | 57 +++++++++++++++++++++++++++++++----- compose/config/validation.py | 8 ++--- 2 files changed, 53 insertions(+), 12 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 9e9cb857fbf..d32d0869957 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -8,6 +8,7 @@ from .errors import CircularReference from .errors import ComposeFileNotFound +from .errors import ConfigError from .errors import ConfigurationError from .interpolation import interpolate_environment_variables from .validation import validate_against_fields_schema @@ -166,12 +167,52 @@ def find_candidates_in_parent_dirs(filenames, path): @validate_top_level_object @validate_service_names -def pre_process_config(config): +def pre_process_config(config, working_dir): """ Pre validation checks and processing of the config file to interpolate env - vars returning a config dict ready to be tested against the schema. + vars and expand volume paths, returning a config dict ready to be tested + against the schema. """ - return interpolate_environment_variables(config) + interpolated_config = interpolate_environment_variables(config) + expanded_paths_config = expand_volume_paths(interpolated_config, working_dir) + return expanded_paths_config + + +def expand_volume_paths(config, working_dir): + """ + For every volume in the volumes list in a service, expand any relative paths. + """ + for (service_name, service_dict) in config.items(): + if 'volumes' in service_dict: + try: + expanded_volumes = [ + expand_volume_path(volume, working_dir) + for volume in service_dict['volumes'] + ] + service_dict['volumes'] = expanded_volumes + except ConfigError as e: + msg = "Service {} contains a config error.".format(service_name) + raise ConfigError(msg + e) + return config + + +def expand_volume_path(volume_path, working_dir): + """ + A volume path can be relative, eg('./stuff', '../stuff') or it can be + absolute, eg('/stuff', 'c:/stuff'). Relative paths need to be expanded. + """ + if volume_path.startswith('.') or volume_path.startswith('~'): + # we're relative + path_parts = volume_path.split(':') + if len(path_parts) == 1: + raise ConfigError( + "Volume %s has incorrect format, external path can" + "not be a relative path." % volume_path + ) + + path_parts[0] = expand_path(working_dir, path_parts[0]) + return ":".join(path_parts) + return volume_path def load(config_details): @@ -193,7 +234,7 @@ def build_service(filename, service_name, service_dict): return service_dict def load_file(filename, config): - processed_config = pre_process_config(config) + processed_config = pre_process_config(config, config_details.working_dir) validate_against_fields_schema(processed_config) return [ build_service(filename, name, service_config) @@ -212,7 +253,6 @@ def merge_services(base, override): config_file = ConfigFile( config_file.filename, merge_services(config_file.config, next_file.config)) - return load_file(config_file.filename, config_file.config) @@ -279,7 +319,8 @@ def validate_and_construct_extends(self): self.extended_service_name = self.service_dict['extends']['service'] full_extended_config = pre_process_config( - load_yaml(self.extended_config_path) + load_yaml(self.extended_config_path), + self.working_dir ) validate_extended_service_exists( @@ -478,12 +519,12 @@ def resolve_volume_paths(service_dict, working_dir=None): raise Exception("No working_dir passed to resolve_volume_paths()") return [ - resolve_volume_path(v, working_dir, service_dict['name']) + resolve_volume_path(v, working_dir) for v in service_dict['volumes'] ] -def resolve_volume_path(volume, working_dir, service_name): +def resolve_volume_path(volume, working_dir): container_path, host_path = split_path_mapping(volume) container_path = os.path.expanduser(container_path) diff --git a/compose/config/validation.py b/compose/config/validation.py index 0fef304a2a1..819f0c8848b 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -66,24 +66,24 @@ def format_boolean_in_environment(instance): def validate_service_names(func): @wraps(func) - def func_wrapper(config): + def func_wrapper(config, working_dir): for service_name in config.keys(): if type(service_name) is int: raise ConfigurationError( "Service name: {} needs to be a string, eg '{}'".format(service_name, service_name) ) - return func(config) + return func(config, working_dir) return func_wrapper def validate_top_level_object(func): @wraps(func) - def func_wrapper(config): + def func_wrapper(config, working_dir): if not isinstance(config, dict): raise ConfigurationError( "Top level object needs to be a dictionary. Check your .yml file that you have defined a service at the top level." ) - return func(config) + return func(config, working_dir) return func_wrapper From 807013c01695b96b1a96c22e92d97a429f1be5f2 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 12 Oct 2015 12:56:34 +0100 Subject: [PATCH 3/7] Container path should never need expanding Signed-off-by: Mazz Mosley --- compose/config/config.py | 1 - 1 file changed, 1 deletion(-) diff --git a/compose/config/config.py b/compose/config/config.py index d32d0869957..9970ef5c4b2 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -526,7 +526,6 @@ def resolve_volume_paths(service_dict, working_dir=None): def resolve_volume_path(volume, working_dir): container_path, host_path = split_path_mapping(volume) - container_path = os.path.expanduser(container_path) if host_path is not None: if host_path.startswith('.'): From 96369c73c25ae657ec47bf32cb5120dad6f9d6f8 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 12 Oct 2015 12:59:19 +0100 Subject: [PATCH 4/7] Use working_dir for the extends file location Previously it was using self.working_dir from the service that had extends not the location of where we are extending from. Hard to write that sentence to make sense. pre_processing expands volume paths and should do so relative to the file they are defined in. Signed-off-by: Mazz Mosley --- compose/config/config.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compose/config/config.py b/compose/config/config.py index 9970ef5c4b2..b889609a20a 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -317,10 +317,11 @@ def validate_and_construct_extends(self): self.service_dict['extends'] ) self.extended_service_name = self.service_dict['extends']['service'] + other_working_dir = os.path.dirname(self.extended_config_path) full_extended_config = pre_process_config( load_yaml(self.extended_config_path), - self.working_dir + other_working_dir ) validate_extended_service_exists( From a96b9c41dd82a17cfc36667fb900d8284364c76c Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 12 Oct 2015 15:46:06 +0100 Subject: [PATCH 5/7] No path expansion if volume_driver exists Doing the path expansion much earlier on means requires that we don't lose the intended behaviour of no path expansion if a volume driver is specified, as per: https://github.com/docker/compose/commit/a68ee0d9 Signed-off-by: Mazz Mosley --- compose/config/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compose/config/config.py b/compose/config/config.py index b889609a20a..3fe67afb246 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -183,7 +183,7 @@ def expand_volume_paths(config, working_dir): For every volume in the volumes list in a service, expand any relative paths. """ for (service_name, service_dict) in config.items(): - if 'volumes' in service_dict: + if 'volumes' in service_dict and service_dict.get('volume_driver') is None: try: expanded_volumes = [ expand_volume_path(volume, working_dir) From 17bb78684229ff6eadf793d23ac8dc26881999c6 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 12 Oct 2015 15:52:12 +0100 Subject: [PATCH 6/7] Remove redundant resolve_volume_paths The expansion logic has already been done during pre_process_config so it's not needed here anymore. Signed-off-by: Mazz Mosley --- compose/config/config.py | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 3fe67afb246..0a5889aa57a 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -388,9 +388,6 @@ def validate_extended_service_dict(service_dict, filename, service): def process_container_options(service_dict, working_dir=None): service_dict = service_dict.copy() - if 'volumes' in service_dict and service_dict.get('volume_driver') is None: - service_dict['volumes'] = resolve_volume_paths(service_dict, working_dir=working_dir) - if 'build' in service_dict: service_dict['build'] = resolve_build_path(service_dict['build'], working_dir=working_dir) @@ -515,28 +512,6 @@ def env_vars_from_file(filename): return env -def resolve_volume_paths(service_dict, working_dir=None): - if working_dir is None: - raise Exception("No working_dir passed to resolve_volume_paths()") - - return [ - resolve_volume_path(v, working_dir) - for v in service_dict['volumes'] - ] - - -def resolve_volume_path(volume, working_dir): - container_path, host_path = split_path_mapping(volume) - - if host_path is not None: - if host_path.startswith('.'): - host_path = expand_path(working_dir, host_path) - host_path = os.path.expanduser(host_path) - return "{}:{}".format(host_path, container_path) - else: - return container_path - - def resolve_build_path(build_path, working_dir=None): if working_dir is None: raise Exception("No working_dir passed to resolve_build_path") From a339437bdca5a2bd827e466ac40cca0d452001e5 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 12 Oct 2015 17:10:54 +0100 Subject: [PATCH 7/7] Update Volume tests to use config.load Now that path expansion logic is happening in the pre_process_config to ensure the logic we're testing is happening, don't short cut to use make_service_dict and instead go through the appropriate config.load. Signed-off-by: Mazz Mosley --- tests/unit/config/config_test.py | 131 +++++++++++++++++++++++-------- 1 file changed, 100 insertions(+), 31 deletions(-) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index d3fb4d5f17b..fe6c491eafe 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -417,72 +417,141 @@ def test_invalid_interpolation(self): class VolumeConfigTest(unittest.TestCase): def test_no_binding(self): - d = make_service_dict('foo', {'build': '.', 'volumes': ['/data']}, working_dir='.') - self.assertEqual(d['volumes'], ['/data']) + service_dict = config.load( + build_config_details( + {'foo': {'build': '.', 'volumes': ['/data']}}, + '.', + None + ) + )[0] + self.assertEqual(service_dict['volumes'], ['/data']) @mock.patch.dict(os.environ) def test_volume_binding_with_environment_variable(self): os.environ['VOLUME_PATH'] = '/host/path' - d = config.load( + service_dict = config.load( build_config_details( {'foo': {'build': '.', 'volumes': ['${VOLUME_PATH}:/container/path']}}, '.', None, ) )[0] - self.assertEqual(d['volumes'], ['/host/path:/container/path']) + self.assertEqual(service_dict['volumes'], ['/host/path:/container/path']) @pytest.mark.skipif(IS_WINDOWS_PLATFORM, reason='posix paths') @mock.patch.dict(os.environ) def test_volume_binding_with_home(self): os.environ['HOME'] = '/home/user' - d = make_service_dict('foo', {'build': '.', 'volumes': ['~:/container/path']}, working_dir='.') - self.assertEqual(d['volumes'], ['/home/user:/container/path']) + service_dict = config.load( + build_config_details( + {'foo': {'build': '.', 'volumes': ['~:/container/path']}}, + '.', + None + ) + )[0] + self.assertEqual(service_dict['volumes'], ['/home/user:/container/path']) def test_name_does_not_expand(self): - d = make_service_dict('foo', {'build': '.', 'volumes': ['mydatavolume:/data']}, working_dir='.') - self.assertEqual(d['volumes'], ['mydatavolume:/data']) + service_dict = config.load( + build_config_details( + {'foo': {'build': '.', 'volumes': ['mydatavolume:/data']}}, + '.', + None + ) + )[0] + self.assertEqual(service_dict['volumes'], ['mydatavolume:/data']) def test_absolute_posix_path_does_not_expand(self): - d = make_service_dict('foo', {'build': '.', 'volumes': ['/var/lib/data:/data']}, working_dir='.') - self.assertEqual(d['volumes'], ['/var/lib/data:/data']) + service_dict = config.load( + build_config_details( + {'foo': {'build': '.', 'volumes': ['/var/lib/data:/data']}}, + '.', + None + ) + )[0] + self.assertEqual(service_dict['volumes'], ['/var/lib/data:/data']) def test_absolute_windows_path_does_not_expand(self): - d = make_service_dict('foo', {'build': '.', 'volumes': ['C:\\data:/data']}, working_dir='.') - self.assertEqual(d['volumes'], ['C:\\data:/data']) + service_dict = config.load( + build_config_details( + {'foo': {'build': '.', 'volumes': ['C:\\data:/data']}}, + '.', + None + ) + )[0] + self.assertEqual(service_dict['volumes'], ['C:\\data:/data']) @pytest.mark.skipif(IS_WINDOWS_PLATFORM, reason='posix paths') def test_relative_path_does_expand_posix(self): - d = make_service_dict('foo', {'build': '.', 'volumes': ['./data:/data']}, working_dir='/home/me/myproject') - self.assertEqual(d['volumes'], ['/home/me/myproject/data:/data']) + with mock.patch('compose.config.config.validate_paths'): + service_dict = config.load( + build_config_details( + {'foo': {'build': '.', 'volumes': ['./data:/data']}}, + '/home/me/myproject', + None + ) + )[0] + self.assertEqual(service_dict['volumes'], ['/home/me/myproject/data:/data']) - d = make_service_dict('foo', {'build': '.', 'volumes': ['.:/data']}, working_dir='/home/me/myproject') - self.assertEqual(d['volumes'], ['/home/me/myproject:/data']) + service_dict = config.load( + build_config_details( + {'foo': {'build': '.', 'volumes': ['.:/data']}}, + '/home/me/myproject', + None + ) + )[0] + self.assertEqual(service_dict['volumes'], ['/home/me/myproject:/data']) - d = make_service_dict('foo', {'build': '.', 'volumes': ['../otherproject:/data']}, working_dir='/home/me/myproject') - self.assertEqual(d['volumes'], ['/home/me/otherproject:/data']) + service_dict = config.load( + build_config_details( + {'foo': {'build': '.', 'volumes': ['../otherproject:/data']}}, + '/home/me/myproject', + None + ) + )[0] + self.assertEqual(service_dict['volumes'], ['/home/me/otherproject:/data']) @pytest.mark.skipif(not IS_WINDOWS_PLATFORM, reason='windows paths') - @pytest.mark.skipif(IS_WINDOWS_PLATFORM, reason='waiting for this to be resolved: https://github.com/docker/compose/issues/2128') def test_relative_path_does_expand_windows(self): - d = make_service_dict('foo', {'build': '.', 'volumes': ['./data:/data']}, working_dir='C:\\Users\\me\\myproject') - self.assertEqual(d['volumes'], ['C:\\Users\\me\\myproject\\data:/data']) + with mock.patch('compose.config.config.validate_paths'): + service_dict = config.load( + build_config_details( + {'foo': {'build': '.', 'volumes': ['./data:/data']}}, + 'C:\\Users\\me\\myproject', + None + ) + )[0] + self.assertEqual(service_dict['volumes'], ['C:\\Users\\me\\myproject\\data:/data']) - d = make_service_dict('foo', {'build': '.', 'volumes': ['.:/data']}, working_dir='C:\\Users\\me\\myproject') - self.assertEqual(d['volumes'], ['C:\\Users\\me\\myproject:/data']) + service_dict = config.load( + build_config_details( + {'foo': {'build': '.', 'volumes': ['.:/data']}}, + 'C:\\Users\\me\\myproject', + None + ) + )[0] + self.assertEqual(service_dict['volumes'], ['C:\\Users\\me\\myproject:/data']) - d = make_service_dict('foo', {'build': '.', 'volumes': ['../otherproject:/data']}, working_dir='C:\\Users\\me\\myproject') - self.assertEqual(d['volumes'], ['C:\\Users\\me\\otherproject:/data']) + service_dict = config.load( + build_config_details( + {'foo': {'build': '.', 'volumes': ['../otherproject:/data']}}, + 'C:\\Users\\me\\myproject', + None + ) + )[0] + self.assertEqual(service_dict['volumes'], ['C:\\Users\\me\\otherproject:/data']) @mock.patch.dict(os.environ) def test_home_directory_with_driver_does_not_expand(self): os.environ['NAME'] = 'surprise!' - d = make_service_dict('foo', { - 'build': '.', - 'volumes': ['~:/data'], - 'volume_driver': 'foodriver', - }, working_dir='.') - self.assertEqual(d['volumes'], ['~:/data']) + service_dict = config.load( + build_config_details( + {'foo': {'build': '.', 'volumes': ['~:/data'], 'volume_driver': 'foodriver'}}, + '.', + None + ) + )[0] + self.assertEqual(service_dict['volumes'], ['~:/data']) class MergePathMappingTest(object):