Merge pull request #2335 from dnephin/volumes_refactoring

Some small refactoring around volumes

compose/config/config.py

@@ -252,7 +252,7 @@ class ServiceLoader(object):
         if not self.already_seen:
             validate_against_service_schema(service_dict, self.service_name)
 
-        return process_container_options(service_dict, working_dir=self.working_dir)
+        return process_container_options(self.working_dir, service_dict)
 
     def validate_and_construct_extends(self):
         extends = self.service_dict['extends']
@@ -321,7 +321,7 @@ def resolve_environment(working_dir, service_dict):
 
     env = {}
     if 'env_file' in service_dict:
-        for env_file in get_env_files(service_dict, working_dir=working_dir):
+        for env_file in get_env_files(working_dir, service_dict):
             env.update(env_vars_from_file(env_file))
 
     env.update(parse_environment(service_dict.get('environment')))
@@ -345,14 +345,14 @@ def validate_extended_service_dict(service_dict, filename, service):
                 "%s services with 'net: container' cannot be extended" % error_prefix)
 
 
-def process_container_options(service_dict, working_dir=None):
-    service_dict = service_dict.copy()
+def process_container_options(working_dir, service_dict):
+    service_dict = dict(service_dict)
 
     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)
+        service_dict['volumes'] = resolve_volume_paths(working_dir, service_dict)
 
     if 'build' in service_dict:
-        service_dict['build'] = resolve_build_path(service_dict['build'], working_dir=working_dir)
+        service_dict['build'] = expand_path(working_dir, service_dict['build'])
 
     if 'labels' in service_dict:
         service_dict['labels'] = parse_labels(service_dict['labels'])
@@ -428,7 +428,7 @@ def merge_environment(base, override):
     return env
 
 
-def get_env_files(options, working_dir=None):
+def get_env_files(working_dir, options):
     if 'env_file' not in options:
         return {}
 
@@ -488,17 +488,14 @@ 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()")
-
+def resolve_volume_paths(working_dir, service_dict):
     return [
-        resolve_volume_path(v, working_dir, service_dict['name'])
-        for v in service_dict['volumes']
+        resolve_volume_path(working_dir, volume, service_dict['name'])
+        for volume in service_dict['volumes']
     ]
 
 
-def resolve_volume_path(volume, working_dir, service_name):
+def resolve_volume_path(working_dir, volume, service_name):
     container_path, host_path = split_path_mapping(volume)
 
     if host_path is not None:
@@ -510,12 +507,6 @@ def resolve_volume_path(volume, working_dir, service_name):
         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")
-    return expand_path(working_dir, build_path)
-
-
 def validate_paths(service_dict):
     if 'build' in service_dict:
         build_path = service_dict['build']
@@ -582,7 +573,7 @@ def parse_labels(labels):
         return dict(split_label(e) for e in labels)
 
     if isinstance(labels, dict):
-        return labels
+        return dict(labels)
 
 
 def split_label(label):

compose/service.py

@@ -913,14 +913,15 @@ def merge_volume_bindings(volumes_option, previous_container):
     """Return a list of volume bindings for a container. Container data volumes
     are replaced by those from the previous container.
     """
+    volumes = [parse_volume_spec(volume) for volume in volumes_option or []]
     volume_bindings = dict(
-        build_volume_binding(parse_volume_spec(volume))
-        for volume in volumes_option or []
-        if ':' in volume)
+        build_volume_binding(volume)
+        for volume in volumes
+        if volume.external)
 
     if previous_container:
         volume_bindings.update(
-            get_container_data_volumes(previous_container, volumes_option))
+            get_container_data_volumes(previous_container, volumes))
 
     return list(volume_bindings.values())
 
@@ -931,12 +932,14 @@ def get_container_data_volumes(container, volumes_option):
     """
     volumes = []
 
-    volumes_option = volumes_option or []
     container_volumes = container.get('Volumes') or {}
-    image_volumes = container.image_config['ContainerConfig'].get('Volumes') or {}
+    image_volumes = [
+        parse_volume_spec(volume)
+        for volume in
+        container.image_config['ContainerConfig'].get('Volumes') or {}
+    ]
 
-    for volume in set(volumes_option + list(image_volumes)):
-        volume = parse_volume_spec(volume)
+    for volume in set(volumes_option + image_volumes):
         # No need to preserve host volumes
         if volume.external:
             continue

tests/unit/config/config_test.py

@@ -577,7 +577,7 @@ class VolumeConfigTest(unittest.TestCase):
 
     def test_volume_path_with_non_ascii_directory(self):
         volume = u'/Füü/data:/data'
-        container_path = config.resolve_volume_path(volume, ".", "test")
+        container_path = config.resolve_volume_path(".", volume, "test")
         self.assertEqual(container_path, volume)
 
 

tests/unit/service_test.py

@@ -593,11 +593,11 @@ class ServiceVolumesTest(unittest.TestCase):
         self.assertEqual(binding, ('/inside', '/outside:/inside:rw'))
 
     def test_get_container_data_volumes(self):
-        options = [
+        options = [parse_volume_spec(v) for v in [
             '/host/volume:/host/volume:ro',
             '/new/volume',
             '/existing/volume',
-        ]
+        ]]
 
         self.mock_client.inspect_image.return_value = {
             'ContainerConfig': {