Merge pull request #2785 from dnephin/depends_on_with_extends

Fix list options when used with extends and multiple files

compose/config/config.py

@@ -26,6 +26,7 @@ from .sort_services import get_service_name_from_network_mode
 from .sort_services import sort_service_dicts
 from .types import parse_extra_hosts
 from .types import parse_restart_spec
+from .types import ServiceLink
 from .types import VolumeFromSpec
 from .types import VolumeSpec
 from .validation import match_named_volumes
@@ -85,8 +86,6 @@ ALLOWED_KEYS = DOCKER_CONFIG_KEYS + [
     'build',
     'container_name',
     'dockerfile',
-    'expose',
-    'external_links',
     'logging',
 ]
 
@@ -643,44 +642,79 @@ def merge_service_dicts_from_files(base, override, version):
     return new_service
 
 
+class MergeDict(dict):
+    """A dict-like object responsible for merging two dicts into one."""
+
+    def __init__(self, base, override):
+        self.base = base
+        self.override = override
+
+    def needs_merge(self, field):
+        return field in self.base or field in self.override
+
+    def merge_field(self, field, merge_func, default=None):
+        if not self.needs_merge(field):
+            return
+
+        self[field] = merge_func(
+            self.base.get(field, default),
+            self.override.get(field, default))
+
+    def merge_mapping(self, field, parse_func):
+        if not self.needs_merge(field):
+            return
+
+        self[field] = parse_func(self.base.get(field))
+        self[field].update(parse_func(self.override.get(field)))
+
+    def merge_sequence(self, field, parse_func):
+        def parse_sequence_func(seq):
+            return to_mapping((parse_func(item) for item in seq), 'merge_field')
+
+        if not self.needs_merge(field):
+            return
+
+        merged = parse_sequence_func(self.base.get(field, []))
+        merged.update(parse_sequence_func(self.override.get(field, [])))
+        self[field] = [item.repr() for item in merged.values()]
+
+    def merge_scalar(self, field):
+        if self.needs_merge(field):
+            self[field] = self.override.get(field, self.base.get(field))
+
+
 def merge_service_dicts(base, override, version):
-    d = {}
+    md = MergeDict(base, override)
 
-    def merge_field(field, merge_func, default=None):
+    md.merge_mapping('environment', parse_environment)
-        if field in base or field in override:
+    md.merge_mapping('labels', parse_labels)
-            d[field] = merge_func(
+    md.merge_mapping('ulimits', parse_ulimits)
-                base.get(field, default),
+    md.merge_sequence('links', ServiceLink.parse)
-                override.get(field, default))
-
-    def merge_mapping(mapping, parse_func):
-        if mapping in base or mapping in override:
-            merged = parse_func(base.get(mapping, None))
-            merged.update(parse_func(override.get(mapping, None)))
-            d[mapping] = merged
-
-    merge_mapping('environment', parse_environment)
-    merge_mapping('labels', parse_labels)
-    merge_mapping('ulimits', parse_ulimits)
 
     for field in ['volumes', 'devices']:
-        merge_field(field, merge_path_mappings)
+        md.merge_field(field, merge_path_mappings)
 
-    for field in ['ports', 'expose', 'external_links']:
+    for field in [
-        merge_field(field, operator.add, default=[])
+        'depends_on',
+        'expose',
+        'external_links',
+        'ports',
+        'volumes_from',
+    ]:
+        md.merge_field(field, operator.add, default=[])
 
     for field in ['dns', 'dns_search', 'env_file']:
-        merge_field(field, merge_list_or_string)
+        md.merge_field(field, merge_list_or_string)
 
-    for field in set(ALLOWED_KEYS) - set(d):
+    for field in set(ALLOWED_KEYS) - set(md):
-        if field in base or field in override:
+        md.merge_scalar(field)
-            d[field] = override.get(field, base.get(field))
 
     if version == V1:
-        legacy_v1_merge_image_or_build(d, base, override)
+        legacy_v1_merge_image_or_build(md, base, override)
     else:
-        merge_build(d, base, override)
+        merge_build(md, base, override)
 
-    return d
+    return dict(md)
 
 
 def merge_build(output, base, override):
@@ -914,6 +948,10 @@ def to_list(value):
         return value
 
 
+def to_mapping(sequence, key_field):
+    return {getattr(item, key_field): item for item in sequence}
+
+
 def has_uppercase(name):
     return any(char in string.ascii_uppercase for char in name)
 

compose/config/types.py

@@ -168,3 +168,22 @@ class VolumeSpec(namedtuple('_VolumeSpec', 'external internal mode')):
     @property
     def is_named_volume(self):
         return self.external and not self.external.startswith(('.', '/', '~'))
+
+
+class ServiceLink(namedtuple('_ServiceLink', 'target alias')):
+
+    @classmethod
+    def parse(cls, link_spec):
+        target, _, alias = link_spec.partition(':')
+        if not alias:
+            alias = target
+        return cls(target, alias)
+
+    def repr(self):
+        if self.target == self.alias:
+            return self.target
+        return '{s.target}:{s.alias}'.format(s=self)
+
+    @property
+    def merge_field(self):
+        return self.alias

docs/extends.md

@@ -32,12 +32,9 @@ contains your base configuration. The override file, as its name implies, can
 contain configuration overrides for existing services or entirely new
 services.
 
-If a service is defined in both files, Compose merges the configurations using
+If a service is defined in both files Compose merges the configurations using
-the same rules as the `extends` field (see [Adding and overriding
+the rules described in [Adding and overriding
-configuration](#adding-and-overriding-configuration)), with one exception.  If a
+configuration](#adding-and-overriding-configuration).
-service contains `links` or `volumes_from` those fields are copied over and
-replace any values in the original service, in the same way single-valued fields
-are copied.
 
 To use multiple override files, or an override file with a different name, you
 can use the `-f` option to specify the list of files. Compose merges files in
@@ -176,10 +173,12 @@ is useful if you have several services that reuse a common set of configuration
 options. Using `extends` you can define a common set of service options in one
 place and refer to it from anywhere.
 
-> **Note:** `links` and `volumes_from` are never shared between services using
+> **Note:** `links`, `volumes_from`, and `depends_on` are never shared between
-> `extends`. See
+> services using >`extends`. These exceptions exist to avoid
-> [Adding and overriding configuration](#adding-and-overriding-configuration)
+> implicit dependencies—you always define `links` and `volumes_from`
- > for more information.
+> locally. This ensures dependencies between services are clearly visible when
+> reading the current file. Defining these locally also ensures changes to the
+> referenced file don't result in breakage.
 
 ### Understand the extends configuration
 
@@ -275,13 +274,7 @@ common configuration:
 
 ## Adding and overriding configuration
 
-Compose copies configurations from the original service over to the local one,
+Compose copies configurations from the original service over to the local one.
-**except** for `links` and `volumes_from`. These exceptions exist to avoid
-implicit dependencies—you always define `links` and `volumes_from`
-locally. This ensures dependencies between services are clearly visible when
-reading the current file. Defining these locally also ensures changes to the
-referenced file don't result in breakage.
-
 If a configuration option is defined in both the original service the local
 service, the local value *replaces* or *extends* the original value.
 

tests/unit/config/config_test.py

@@ -602,6 +602,7 @@ class ConfigTest(unittest.TestCase):
                 'services': {
                     'web': {
                         'image': 'example/web',
+                        'depends_on': ['db'],
                     },
                     'db': {
                         'image': 'example/db',
@@ -616,7 +617,11 @@ class ConfigTest(unittest.TestCase):
                     'web': {
                         'build': '/',
                         'volumes': ['/home/user/project:/code'],
+                        'depends_on': ['other'],
                     },
+                    'other': {
+                        'image': 'example/other',
+                    }
                 }
             })
         details = config.ConfigDetails('.', [base_file, override_file])
@@ -628,11 +633,16 @@ class ConfigTest(unittest.TestCase):
                 'build': {'context': os.path.abspath('/')},
                 'image': 'example/web',
                 'volumes': [VolumeSpec.parse('/home/user/project:/code')],
+                'depends_on': ['db', 'other'],
             },
             {
                 'name': 'db',
                 'image': 'example/db',
             },
+            {
+                'name': 'other',
+                'image': 'example/other',
+            },
         ]
         assert service_sort(service_dicts) == service_sort(expected)
 
@@ -2299,6 +2309,24 @@ class ExtendsTest(unittest.TestCase):
         service = load_from_filename(str(tmpdir.join('docker-compose.yml')))
         self.assertEquals(service[0]['command'], "top")
 
+    def test_extends_with_depends_on(self):
+        tmpdir = py.test.ensuretemp('test_extends_with_defined_version')
+        self.addCleanup(tmpdir.remove)
+        tmpdir.join('docker-compose.yml').write("""
+            version: "2"
+            services:
+              base:
+                image: example
+              web:
+                extends: base
+                image: busybox
+                depends_on: ['other']
+              other:
+                image: example
+        """)
+        services = load_from_filename(str(tmpdir.join('docker-compose.yml')))
+        assert service_sort(services)[2]['depends_on'] == ['other']
+
 
 @pytest.mark.xfail(IS_WINDOWS_PLATFORM, reason='paths use slash')
 class ExpandPathTest(unittest.TestCase):