From f4cd5b1d45f0eee1a731af1664c75d27e9b4aa18 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Tue, 22 Sep 2015 16:13:42 +0100 Subject: [PATCH 1/5] Handle windows volume paths When a relative path is expanded and we're on a windows platform, it expands to include the drive, eg C:\ , which was causing a ConfigError as we split on ":" in parse_volume_spec and that was giving too many parts. Use os.path.splitdrive instead of manually calculating the drive. This should help us deal with windows drives as part of the volume path better than us doing it manually. Signed-off-by: Mazz Mosley --- compose/config/config.py | 11 ++++++----- compose/const.py | 1 + compose/service.py | 14 ++++++++++++++ tests/unit/config/config_test.py | 15 +++++++++++++++ tests/unit/service_test.py | 15 +++++++++++++++ 5 files changed, 51 insertions(+), 5 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 0444ba3a6bc..9e9cb857fbf 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -526,12 +526,13 @@ def path_mappings_from_dict(d): return [join_path_mapping(v) for v in d.items()] -def split_path_mapping(string): - if ':' in string: - (host, container) = string.split(':', 1) - return (container, host) +def split_path_mapping(volume_path): + drive, volume_config = os.path.splitdrive(volume_path) + if ':' in volume_config: + (host, container) = volume_config.split(':', 1) + return (container, drive + host) else: - return (string, None) + return (volume_path, None) def join_path_mapping(pair): diff --git a/compose/const.py b/compose/const.py index b43e655b19d..b04b7e7e72e 100644 --- a/compose/const.py +++ b/compose/const.py @@ -2,6 +2,7 @@ import sys DEFAULT_TIMEOUT = 10 +IS_WINDOWS_PLATFORM = (sys.platform == "win32") LABEL_CONTAINER_NUMBER = 'com.docker.compose.container-number' LABEL_ONE_OFF = 'com.docker.compose.oneoff' LABEL_PROJECT = 'com.docker.compose.project' diff --git a/compose/service.py b/compose/service.py index 960d3936bf6..4df10fbb101 100644 --- a/compose/service.py +++ b/compose/service.py @@ -20,6 +20,7 @@ from .config import merge_environment from .config.validation import VALID_NAME_CHARS from .const import DEFAULT_TIMEOUT +from .const import IS_WINDOWS_PLATFORM from .const import LABEL_CONFIG_HASH from .const import LABEL_CONTAINER_NUMBER from .const import LABEL_ONE_OFF @@ -937,7 +938,20 @@ def build_volume_binding(volume_spec): def parse_volume_spec(volume_config): + """ + A volume_config string, which is a path, split it into external:internal[:mode] + parts to be returned as a valid VolumeSpec tuple. + """ parts = volume_config.split(':') + + if IS_WINDOWS_PLATFORM: + # relative paths in windows expand to include the drive, eg C:\ + # so we join the first 2 parts back together to count as one + drive, volume_path = os.path.splitdrive(volume_config) + windows_parts = volume_path.split(":") + windows_parts[0] = os.path.join(drive, windows_parts[0]) + parts = windows_parts + if len(parts) > 3: raise ConfigError("Volume %s has incorrect format, should be " "external:internal[:mode]" % volume_config) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 3269cdff87c..cf299738f1a 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -1124,6 +1124,21 @@ def test_expand_path_with_tilde(self): self.assertEqual(result, user_path + 'otherdir/somefile') +class VolumePathTest(unittest.TestCase): + + @pytest.mark.xfail((not IS_WINDOWS_PLATFORM), reason='does not have a drive') + def test_split_path_mapping_with_windows_path(self): + windows_volume_path = "c:\\Users\\msamblanet\\Documents\\anvil\\connect\\config:/opt/connect/config:ro" + expected_mapping = ( + "/opt/connect/config:ro", + "c:\\Users\\msamblanet\\Documents\\anvil\\connect\\config" + ) + + mapping = config.split_path_mapping(windows_volume_path) + + self.assertEqual(mapping, expected_mapping) + + @pytest.mark.xfail(IS_WINDOWS_PLATFORM, reason='paths use slash') class BuildPathTest(unittest.TestCase): def setUp(self): diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index a1c195acf00..b0cee1ee160 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -466,6 +466,21 @@ def test_parse_volume_spec_too_many_parts(self): with self.assertRaises(ConfigError): parse_volume_spec('one:two:three:four') + @pytest.mark.xfail((not IS_WINDOWS_PLATFORM), reason='does not have a drive') + def test_parse_volume_windows_relative_path(self): + windows_relative_path = "c:\\Users\\msamblanet\\Documents\\anvil\\connect\\config:\\opt\\connect\\config:ro" + + spec = parse_volume_spec(windows_relative_path) + + self.assertEqual( + spec, + ( + "c:\\Users\\msamblanet\\Documents\\anvil\\connect\\config", + "\\opt\\connect\\config", + "ro" + ) + ) + def test_build_volume_binding(self): binding = build_volume_binding(parse_volume_spec('/outside:/inside')) self.assertEqual(binding, ('/inside', '/outside:/inside:rw')) From 58270d88592e6a097763ce0052ef6a8d22e9bbcb Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Wed, 23 Sep 2015 17:08:41 +0100 Subject: [PATCH 2/5] Remove duplicate and re-order alphabetically Signed-off-by: Mazz Mosley --- compose/const.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/compose/const.py b/compose/const.py index b04b7e7e72e..1b6894189e2 100644 --- a/compose/const.py +++ b/compose/const.py @@ -2,6 +2,7 @@ import sys DEFAULT_TIMEOUT = 10 +HTTP_TIMEOUT = int(os.environ.get('COMPOSE_HTTP_TIMEOUT', os.environ.get('DOCKER_CLIENT_TIMEOUT', 60))) IS_WINDOWS_PLATFORM = (sys.platform == "win32") LABEL_CONTAINER_NUMBER = 'com.docker.compose.container-number' LABEL_ONE_OFF = 'com.docker.compose.oneoff' @@ -9,5 +10,3 @@ LABEL_SERVICE = 'com.docker.compose.service' LABEL_VERSION = 'com.docker.compose.version' LABEL_CONFIG_HASH = 'com.docker.compose.config-hash' -HTTP_TIMEOUT = int(os.environ.get('COMPOSE_HTTP_TIMEOUT', os.environ.get('DOCKER_CLIENT_TIMEOUT', 60))) -IS_WINDOWS_PLATFORM = (sys.platform == 'win32') From 2276326d7ecc4b3bbc30d1acaaa0213669b7ad59 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Thu, 1 Oct 2015 11:06:15 +0100 Subject: [PATCH 3/5] volume path compatibility with engine An expanded windows path of c:\shiny\thing:\shiny:rw needs to be changed to be linux style path, including the drive, like /c/shiny/thing /shiny to be mounted successfully by the engine. Signed-off-by: Mazz Mosley --- compose/service.py | 47 +++++++++++++++++++++++--------- tests/unit/config/config_test.py | 3 +- tests/unit/service_test.py | 7 ++--- 3 files changed, 38 insertions(+), 19 deletions(-) diff --git a/compose/service.py b/compose/service.py index 4df10fbb101..c9ca00ae414 100644 --- a/compose/service.py +++ b/compose/service.py @@ -937,33 +937,54 @@ def build_volume_binding(volume_spec): return volume_spec.internal, "{}:{}:{}".format(*volume_spec) -def parse_volume_spec(volume_config): +def normalize_paths_for_engine(external_path, internal_path): """ - A volume_config string, which is a path, split it into external:internal[:mode] - parts to be returned as a valid VolumeSpec tuple. + Windows paths, c:\my\path\shiny, need to be changed to be compatible with + the Engine. Volume paths are expected to be linux style /c/my/path/shiny/ """ - parts = volume_config.split(':') + if IS_WINDOWS_PLATFORM: + if external_path: + drive, tail = os.path.splitdrive(external_path) + + if drive: + reformatted_drive = "/{}".format(drive.replace(":", "")) + external_path = reformatted_drive + tail + + external_path = "/".join(external_path.split("\\")) + + return external_path, "/".join(internal_path.split("\\")) + else: + return external_path, internal_path + +def parse_volume_spec(volume_config): + """ + Parse a volume_config path and split it into external:internal[:mode] + parts to be returned as a valid VolumeSpec. + """ if IS_WINDOWS_PLATFORM: # relative paths in windows expand to include the drive, eg C:\ # so we join the first 2 parts back together to count as one - drive, volume_path = os.path.splitdrive(volume_config) - windows_parts = volume_path.split(":") - windows_parts[0] = os.path.join(drive, windows_parts[0]) - parts = windows_parts + drive, tail = os.path.splitdrive(volume_config) + parts = tail.split(":") + + if drive: + parts[0] = drive + parts[0] + else: + parts = volume_config.split(':') if len(parts) > 3: raise ConfigError("Volume %s has incorrect format, should be " "external:internal[:mode]" % volume_config) if len(parts) == 1: - external = None - internal = os.path.normpath(parts[0]) + external, internal = normalize_paths_for_engine(None, os.path.normpath(parts[0])) else: - external = os.path.normpath(parts[0]) - internal = os.path.normpath(parts[1]) + external, internal = normalize_paths_for_engine(os.path.normpath(parts[0]), os.path.normpath(parts[1])) - mode = parts[2] if len(parts) == 3 else 'rw' + mode = 'rw' + if len(parts) == 3: + mode = parts[2] return VolumeSpec(external, internal, mode) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index cf299738f1a..c06a2a32792 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -420,7 +420,6 @@ def test_no_binding(self): d = make_service_dict('foo', {'build': '.', 'volumes': ['/data']}, working_dir='.') self.assertEqual(d['volumes'], ['/data']) - @pytest.mark.xfail(IS_WINDOWS_PLATFORM, reason='paths use slash') @mock.patch.dict(os.environ) def test_volume_binding_with_environment_variable(self): os.environ['VOLUME_PATH'] = '/host/path' @@ -433,7 +432,7 @@ def test_volume_binding_with_environment_variable(self): )[0] self.assertEqual(d['volumes'], ['/host/path:/container/path']) - @pytest.mark.xfail(IS_WINDOWS_PLATFORM, reason='paths use slash') + @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' diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index b0cee1ee160..a3db0d43439 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -441,7 +441,6 @@ def mock_get_image(images): raise NoSuchImageError() -@pytest.mark.xfail(IS_WINDOWS_PLATFORM, reason='paths use slash') class ServiceVolumesTest(unittest.TestCase): def setUp(self): @@ -468,15 +467,15 @@ def test_parse_volume_spec_too_many_parts(self): @pytest.mark.xfail((not IS_WINDOWS_PLATFORM), reason='does not have a drive') def test_parse_volume_windows_relative_path(self): - windows_relative_path = "c:\\Users\\msamblanet\\Documents\\anvil\\connect\\config:\\opt\\connect\\config:ro" + windows_relative_path = "c:\\Users\\me\\Documents\\shiny\\config:\\opt\\shiny\\config:ro" spec = parse_volume_spec(windows_relative_path) self.assertEqual( spec, ( - "c:\\Users\\msamblanet\\Documents\\anvil\\connect\\config", - "\\opt\\connect\\config", + "/c/Users/me/Documents/shiny/config", + "/opt/shiny/config", "ro" ) ) From af8032a5f4a5075d71c220bfafbadcbbebbcb5b7 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Thu, 1 Oct 2015 12:09:32 +0100 Subject: [PATCH 4/5] Fix incorrect test name I'd written relative, when I meant absolute. D'oh. Signed-off-by: Mazz Mosley --- tests/unit/service_test.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index a3db0d43439..c682b823773 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -466,10 +466,10 @@ def test_parse_volume_spec_too_many_parts(self): parse_volume_spec('one:two:three:four') @pytest.mark.xfail((not IS_WINDOWS_PLATFORM), reason='does not have a drive') - def test_parse_volume_windows_relative_path(self): - windows_relative_path = "c:\\Users\\me\\Documents\\shiny\\config:\\opt\\shiny\\config:ro" + def test_parse_volume_windows_absolute_path(self): + windows_absolute_path = "c:\\Users\\me\\Documents\\shiny\\config:\\opt\\shiny\\config:ro" - spec = parse_volume_spec(windows_relative_path) + spec = parse_volume_spec(windows_absolute_path) self.assertEqual( spec, From bef5c2238e7cefa12cb34b814dc536fa46e9773f Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Fri, 2 Oct 2015 15:29:26 +0100 Subject: [PATCH 5/5] Skip a test for now This needs resolving outside of this PR, as it is a much bigger piece of work. https://github.com/docker/compose/issues/2128 Signed-off-by: Mazz Mosley --- tests/unit/config/config_test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index c06a2a32792..b505740f571 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -463,6 +463,7 @@ def test_relative_path_does_expand_posix(self): self.assertEqual(d['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'])