Merge pull request #1632 from mnowster/extends_file_default_behaviour

Extends file default behaviour and fixes circular reference bug

compose/config.py

@@ -134,20 +134,20 @@ def load(config_details):
     return service_dicts
 
 
-def make_service_dict(name, service_dict, working_dir=None):
-    return ServiceLoader(working_dir=working_dir).make_service_dict(name, service_dict)
-
-
 class ServiceLoader(object):
     def __init__(self, working_dir, filename=None, already_seen=None):
-        self.working_dir = working_dir
-        self.filename = filename
+        self.working_dir = os.path.abspath(working_dir)
+        if filename:
+            self.filename = os.path.abspath(filename)
+        else:
+            self.filename = filename
         self.already_seen = already_seen or []
 
-    def make_service_dict(self, name, service_dict):
+    def detect_cycle(self, name):
         if self.signature(name) in self.already_seen:
-            raise CircularReference(self.already_seen)
+            raise CircularReference(self.already_seen + [self.signature(name)])
 
+    def make_service_dict(self, name, service_dict):
         service_dict = service_dict.copy()
         service_dict['name'] = name
         service_dict = resolve_environment(service_dict, working_dir=self.working_dir)
@@ -158,12 +158,17 @@ class ServiceLoader(object):
         if 'extends' not in service_dict:
             return service_dict
 
-        extends_options = process_extends_options(service_dict['name'], service_dict['extends'])
+        extends_options = self.validate_extends_options(service_dict['name'], service_dict['extends'])
 
         if self.working_dir is None:
             raise Exception("No working_dir passed to ServiceLoader()")
 
-        other_config_path = expand_path(self.working_dir, extends_options['file'])
+        if 'file' in extends_options:
+            extends_from_filename = extends_options['file']
+            other_config_path = expand_path(self.working_dir, extends_from_filename)
+        else:
+            other_config_path = self.filename
+
         other_working_dir = os.path.dirname(other_config_path)
         other_already_seen = self.already_seen + [self.signature(service_dict['name'])]
         other_loader = ServiceLoader(
@@ -174,6 +179,7 @@ class ServiceLoader(object):
 
         other_config = load_yaml(other_config_path)
         other_service_dict = other_config[extends_options['service']]
+        other_loader.detect_cycle(extends_options['service'])
         other_service_dict = other_loader.make_service_dict(
             service_dict['name'],
             other_service_dict,
@@ -189,25 +195,29 @@ class ServiceLoader(object):
     def signature(self, name):
         return (self.filename, name)
 
+    def validate_extends_options(self, service_name, extends_options):
+        error_prefix = "Invalid 'extends' configuration for %s:" % service_name
 
-def process_extends_options(service_name, extends_options):
-    error_prefix = "Invalid 'extends' configuration for %s:" % service_name
+        if not isinstance(extends_options, dict):
+            raise ConfigurationError("%s must be a dictionary" % error_prefix)
 
-    if not isinstance(extends_options, dict):
-        raise ConfigurationError("%s must be a dictionary" % error_prefix)
-
-    if 'service' not in extends_options:
-        raise ConfigurationError(
-            "%s you need to specify a service, e.g. 'service: web'" % error_prefix
-        )
+        if 'service' not in extends_options:
+            raise ConfigurationError(
+                "%s you need to specify a service, e.g. 'service: web'" % error_prefix
+            )
 
-    for k, _ in extends_options.items():
-        if k not in ['file', 'service']:
+        if 'file' not in extends_options and self.filename is None:
             raise ConfigurationError(
-                "%s unsupported configuration option '%s'" % (error_prefix, k)
+                "%s you need to specify a 'file', e.g. 'file: something.yml'" % error_prefix
             )
 
-    return extends_options
+        for k, _ in extends_options.items():
+            if k not in ['file', 'service']:
+                raise ConfigurationError(
+                    "%s unsupported configuration option '%s'" % (error_prefix, k)
+                )
+
+        return extends_options
 
 
 def validate_extended_service_dict(service_dict, filename, service):

tests/fixtures/extends/no-file-specified.yml

@@ -0,0 +1,9 @@
+myweb:
+  extends:
+    service: web
+  environment:
+    - "BAR=1"
+web:
+  image: busybox
+  environment:
+    - "BAZ=3"

tests/fixtures/extends/specify-file-as-self.yml

@@ -0,0 +1,16 @@
+myweb:
+  extends:
+    file: specify-file-as-self.yml
+    service: web
+  environment:
+    - "BAR=1"
+web:
+  extends:
+    file: specify-file-as-self.yml
+    service: otherweb
+  image: busybox
+  environment:
+    - "BAZ=3"
+otherweb:
+  environment:
+    - "YEP=1"

tests/integration/testcases.py

@@ -1,7 +1,7 @@
 from __future__ import unicode_literals
 from __future__ import absolute_import
 from compose.service import Service
-from compose.config import make_service_dict
+from compose.config import ServiceLoader
 from compose.const import LABEL_PROJECT
 from compose.cli.docker_client import docker_client
 from compose.progress_stream import stream_output
@@ -30,10 +30,12 @@ class DockerClientTestCase(unittest.TestCase):
         if 'command' not in kwargs:
             kwargs['command'] = ["top"]
 
+        options = ServiceLoader(working_dir='.').make_service_dict(name, kwargs)
+
         return Service(
             project='composetest',
             client=self.client,
-            **make_service_dict(name, kwargs, working_dir='.')
+            **options
         )
 
     def check_build(self, *args, **kwargs):

tests/unit/config_test.py

@@ -7,6 +7,13 @@ from .. import unittest
 from compose import config
 
 
+def make_service_dict(name, service_dict, working_dir):
+    """
+    Test helper function to contruct a ServiceLoader
+    """
+    return config.ServiceLoader(working_dir=working_dir).make_service_dict(name, service_dict)
+
+
 class ConfigTest(unittest.TestCase):
     def test_load(self):
         service_dicts = config.load(
@@ -47,22 +54,22 @@ class ConfigTest(unittest.TestCase):
     def test_config_validation(self):
         self.assertRaises(
             config.ConfigurationError,
-            lambda: config.make_service_dict('foo', {'port': ['8000']})
+            lambda: make_service_dict('foo', {'port': ['8000']}, 'tests/')
         )
-        config.make_service_dict('foo', {'ports': ['8000']})
+        make_service_dict('foo', {'ports': ['8000']}, 'tests/')
 
 
 class VolumePathTest(unittest.TestCase):
     @mock.patch.dict(os.environ)
     def test_volume_binding_with_environ(self):
         os.environ['VOLUME_PATH'] = '/host/path'
-        d = config.make_service_dict('foo', {'volumes': ['${VOLUME_PATH}:/container/path']}, working_dir='.')
+        d = make_service_dict('foo', {'volumes': ['${VOLUME_PATH}:/container/path']}, working_dir='.')
         self.assertEqual(d['volumes'], ['/host/path:/container/path'])
 
     @mock.patch.dict(os.environ)
     def test_volume_binding_with_home(self):
         os.environ['HOME'] = '/home/user'
-        d = config.make_service_dict('foo', {'volumes': ['~:/container/path']}, working_dir='.')
+        d = make_service_dict('foo', {'volumes': ['~:/container/path']}, working_dir='.')
         self.assertEqual(d['volumes'], ['/home/user:/container/path'])
 
 
@@ -219,36 +226,36 @@ class MergeLabelsTest(unittest.TestCase):
 
     def test_no_override(self):
         service_dict = config.merge_service_dicts(
-            config.make_service_dict('foo', {'labels': ['foo=1', 'bar']}),
-            config.make_service_dict('foo', {}),
+            make_service_dict('foo', {'labels': ['foo=1', 'bar']}, 'tests/'),
+            make_service_dict('foo', {}, 'tests/'),
         )
         self.assertEqual(service_dict['labels'], {'foo': '1', 'bar': ''})
 
     def test_no_base(self):
         service_dict = config.merge_service_dicts(
-            config.make_service_dict('foo', {}),
-            config.make_service_dict('foo', {'labels': ['foo=2']}),
+            make_service_dict('foo', {}, 'tests/'),
+            make_service_dict('foo', {'labels': ['foo=2']}, 'tests/'),
         )
         self.assertEqual(service_dict['labels'], {'foo': '2'})
 
     def test_override_explicit_value(self):
         service_dict = config.merge_service_dicts(
-            config.make_service_dict('foo', {'labels': ['foo=1', 'bar']}),
-            config.make_service_dict('foo', {'labels': ['foo=2']}),
+            make_service_dict('foo', {'labels': ['foo=1', 'bar']}, 'tests/'),
+            make_service_dict('foo', {'labels': ['foo=2']}, 'tests/'),
         )
         self.assertEqual(service_dict['labels'], {'foo': '2', 'bar': ''})
 
     def test_add_explicit_value(self):
         service_dict = config.merge_service_dicts(
-            config.make_service_dict('foo', {'labels': ['foo=1', 'bar']}),
-            config.make_service_dict('foo', {'labels': ['bar=2']}),
+            make_service_dict('foo', {'labels': ['foo=1', 'bar']}, 'tests/'),
+            make_service_dict('foo', {'labels': ['bar=2']}, 'tests/'),
         )
         self.assertEqual(service_dict['labels'], {'foo': '1', 'bar': '2'})
 
     def test_remove_explicit_value(self):
         service_dict = config.merge_service_dicts(
-            config.make_service_dict('foo', {'labels': ['foo=1', 'bar=2']}),
-            config.make_service_dict('foo', {'labels': ['bar']}),
+            make_service_dict('foo', {'labels': ['foo=1', 'bar=2']}, 'tests/'),
+            make_service_dict('foo', {'labels': ['bar']}, 'tests/'),
         )
         self.assertEqual(service_dict['labels'], {'foo': '1', 'bar': ''})
 
@@ -286,7 +293,7 @@ class EnvTest(unittest.TestCase):
         os.environ['FILE_DEF_EMPTY'] = 'E2'
         os.environ['ENV_DEF'] = 'E3'
 
-        service_dict = config.make_service_dict(
+        service_dict = make_service_dict(
             'foo', {
                 'environment': {
                     'FILE_DEF': 'F1',
@@ -295,6 +302,7 @@ class EnvTest(unittest.TestCase):
                     'NO_DEF': None
                 },
             },
+            'tests/'
         )
 
         self.assertEqual(
@@ -303,7 +311,7 @@ class EnvTest(unittest.TestCase):
         )
 
     def test_env_from_file(self):
-        service_dict = config.make_service_dict(
+        service_dict = make_service_dict(
             'foo',
             {'env_file': 'one.env'},
             'tests/fixtures/env',
@@ -314,7 +322,7 @@ class EnvTest(unittest.TestCase):
         )
 
     def test_env_from_multiple_files(self):
-        service_dict = config.make_service_dict(
+        service_dict = make_service_dict(
             'foo',
             {'env_file': ['one.env', 'two.env']},
             'tests/fixtures/env',
@@ -328,7 +336,7 @@ class EnvTest(unittest.TestCase):
         options = {'env_file': 'nonexistent.env'}
         self.assertRaises(
             config.ConfigurationError,
-            lambda: config.make_service_dict('foo', options, 'tests/fixtures/env'),
+            lambda: make_service_dict('foo', options, 'tests/fixtures/env'),
         )
 
     @mock.patch.dict(os.environ)
@@ -336,7 +344,7 @@ class EnvTest(unittest.TestCase):
         os.environ['FILE_DEF'] = 'E1'
         os.environ['FILE_DEF_EMPTY'] = 'E2'
         os.environ['ENV_DEF'] = 'E3'
-        service_dict = config.make_service_dict(
+        service_dict = make_service_dict(
             'foo',
             {'env_file': 'resolve.env'},
             'tests/fixtures/env',
@@ -351,14 +359,14 @@ class EnvTest(unittest.TestCase):
         os.environ['HOSTENV'] = '/tmp'
         os.environ['CONTAINERENV'] = '/host/tmp'
 
-        service_dict = config.make_service_dict(
+        service_dict = make_service_dict(
             'foo',
             {'volumes': ['$HOSTENV:$CONTAINERENV']},
             working_dir="tests/fixtures/env"
         )
         self.assertEqual(set(service_dict['volumes']), set(['/tmp:/host/tmp']))
 
-        service_dict = config.make_service_dict(
+        service_dict = make_service_dict(
             'foo',
             {'volumes': ['/opt${HOSTENV}:/opt${CONTAINERENV}']},
             working_dir="tests/fixtures/env"
@@ -413,6 +421,33 @@ class ExtendsTest(unittest.TestCase):
             },
         ])
 
+    def test_self_referencing_file(self):
+        """
+        We specify a 'file' key that is the filename we're already in.
+        """
+        service_dicts = load_from_filename('tests/fixtures/extends/specify-file-as-self.yml')
+        self.assertEqual(service_dicts, [
+            {
+                'environment':
+                {
+                    'YEP': '1', 'BAR': '1', 'BAZ': '3'
+                },
+                'image': 'busybox',
+                'name': 'myweb'
+            },
+            {
+                'environment':
+                {'YEP': '1'},
+                'name': 'otherweb'
+            },
+            {
+                'environment':
+                {'YEP': '1', 'BAZ': '3'},
+                'image': 'busybox',
+                'name': 'web'
+            }
+        ])
+
     def test_circular(self):
         try:
             load_from_filename('tests/fixtures/extends/circle-1.yml')
@@ -427,29 +462,81 @@ class ExtendsTest(unittest.TestCase):
                 ],
             )
 
-    def test_extends_validation(self):
+    def test_extends_validation_empty_dictionary(self):
         dictionary = {'extends': None}
 
         def load_config():
-            return config.make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends')
+            return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends')
 
         self.assertRaisesRegexp(config.ConfigurationError, 'dictionary', load_config)
 
         dictionary['extends'] = {}
         self.assertRaises(config.ConfigurationError, load_config)
 
-        dictionary['extends']['file'] = 'common.yml'
+    def test_extends_validation_missing_service_key(self):
+        dictionary = {'extends': {'file': 'common.yml'}}
+
+        def load_config():
+            return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends')
+
         self.assertRaisesRegexp(config.ConfigurationError, 'service', load_config)
 
-        dictionary['extends']['service'] = 'web'
-        self.assertIsInstance(load_config(), dict)
+    def test_extends_validation_invalid_key(self):
+        dictionary = {
+            'extends':
+            {
+                'service': 'web', 'file': 'common.yml', 'what': 'is this'
+            }
+        }
+
+        def load_config():
+            return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends')
 
-        dictionary['extends']['what'] = 'is this'
         self.assertRaisesRegexp(config.ConfigurationError, 'what', load_config)
 
+    def test_extends_validation_no_file_key_no_filename_set(self):
+        dictionary = {'extends': {'service': 'web'}}
+
+        def load_config():
+            return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends')
+
+        self.assertRaisesRegexp(config.ConfigurationError, 'file', load_config)
+
+    def test_extends_validation_valid_config(self):
+        dictionary = {'extends': {'service': 'web', 'file': 'common.yml'}}
+
+        def load_config():
+            return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends')
+
+        self.assertIsInstance(load_config(), dict)
+
+    def test_extends_file_defaults_to_self(self):
+        """
+        Test not specifying a file in our extends options that the
+        config is valid and correctly extends from itself.
+        """
+        service_dicts = load_from_filename('tests/fixtures/extends/no-file-specified.yml')
+        self.assertEqual(service_dicts, [
+            {
+                'name': 'myweb',
+                'image': 'busybox',
+                'environment': {
+                    "BAR": "1",
+                    "BAZ": "3",
+                }
+            },
+            {
+                'name': 'web',
+                'image': 'busybox',
+                'environment': {
+                    "BAZ": "3",
+                }
+            }
+        ])
+
     def test_blacklisted_options(self):
         def load_config():
-            return config.make_service_dict('myweb', {
+            return make_service_dict('myweb', {
                 'extends': {
                     'file': 'whatever',
                     'service': 'web',
@@ -523,7 +610,7 @@ class BuildPathTest(unittest.TestCase):
 
     def test_relative_path(self):
         relative_build_path = '../build-ctx/'
-        service_dict = config.make_service_dict(
+        service_dict = make_service_dict(
             'relpath',
             {'build': relative_build_path},
             working_dir='tests/fixtures/build-path'
@@ -531,7 +618,7 @@ class BuildPathTest(unittest.TestCase):
         self.assertEquals(service_dict['build'], self.abs_context_path)
 
     def test_absolute_path(self):
-        service_dict = config.make_service_dict(
+        service_dict = make_service_dict(
             'abspath',
             {'build': self.abs_context_path},
             working_dir='tests/fixtures/build-path'