Merge pull request #2349 from dnephin/validate_all_files

Validate all files before merging them

compose/cli/main.py

@@ -13,12 +13,12 @@ from requests.exceptions import ReadTimeout
 
 from .. import __version__
 from .. import legacy
+from ..config import ConfigurationError
 from ..config import parse_environment
 from ..const import DEFAULT_TIMEOUT
 from ..const import HTTP_TIMEOUT
 from ..const import IS_WINDOWS_PLATFORM
 from ..progress_stream import StreamOutputError
-from ..project import ConfigurationError
 from ..project import NoSuchService
 from ..service import BuildError
 from ..service import ConvergenceStrategy

compose/config/__init__.py

@@ -1,5 +1,4 @@
 # flake8: noqa
-from .config import ConfigDetails
 from .config import ConfigurationError
 from .config import DOCKER_CONFIG_KEYS
 from .config import find

compose/config/config.py

@@ -13,7 +13,6 @@ from .errors import ConfigurationError
 from .interpolation import interpolate_environment_variables
 from .validation import validate_against_fields_schema
 from .validation import validate_against_service_schema
-from .validation import validate_extended_service_exists
 from .validation import validate_extends_file_path
 from .validation import validate_top_level_object
 
@@ -99,6 +98,10 @@ class ConfigFile(namedtuple('_ConfigFile', 'filename config')):
     :type  config: :class:`dict`
     """
 
+    @classmethod
+    def from_filename(cls, filename):
+        return cls(filename, load_yaml(filename))
+
 
 def find(base_dir, filenames):
     if filenames == ['-']:
@@ -114,7 +117,7 @@ def find(base_dir, filenames):
     log.debug("Using configuration files: {}".format(",".join(filenames)))
     return ConfigDetails(
         os.path.dirname(filenames[0]),
-        [ConfigFile(f, load_yaml(f)) for f in filenames])
+        [ConfigFile.from_filename(f) for f in filenames])
 
 
 def get_default_config_files(base_dir):
@@ -174,21 +177,19 @@ def load(config_details):
     """
 
     def build_service(filename, service_name, service_dict):
-        loader = ServiceLoader(
+        resolver = ServiceExtendsResolver(
             config_details.working_dir,
             filename,
             service_name,
             service_dict)
-        service_dict = loader.make_service_dict()
+        service_dict = process_service(config_details.working_dir, resolver.run())
         validate_paths(service_dict)
         return service_dict
 
-    def load_file(filename, config):
-        processed_config = interpolate_environment_variables(config)
-        validate_against_fields_schema(processed_config)
+    def build_services(filename, config):
         return [
             build_service(filename, name, service_config)
-            for name, service_config in processed_config.items()
+            for name, service_config in config.items()
         ]
 
     def merge_services(base, override):
@@ -200,19 +201,30 @@ def load(config_details):
             for name in all_service_names
         }
 
-    config_file = config_details.config_files[0]
-    validate_top_level_object(config_file.config)
+    config_file = process_config_file(config_details.config_files[0])
     for next_file in config_details.config_files[1:]:
-        validate_top_level_object(next_file.config)
+        next_file = process_config_file(next_file)
 
-        config_file = ConfigFile(
-            config_file.filename,
-            merge_services(config_file.config, next_file.config))
+        config = merge_services(config_file.config, next_file.config)
+        config_file = config_file._replace(config=config)
 
-    return load_file(config_file.filename, config_file.config)
+    return build_services(config_file.filename, config_file.config)
 
 
-class ServiceLoader(object):
+def process_config_file(config_file, service_name=None):
+    validate_top_level_object(config_file.config)
+    processed_config = interpolate_environment_variables(config_file.config)
+    validate_against_fields_schema(processed_config)
+
+    if service_name and service_name not in processed_config:
+        raise ConfigurationError(
+            "Cannot extend service '{}' in {}: Service not found".format(
+                service_name, config_file.filename))
+
+    return config_file._replace(config=processed_config)
+
+
+class ServiceExtendsResolver(object):
     def __init__(
         self,
         working_dir,
@@ -222,7 +234,7 @@ class ServiceLoader(object):
         already_seen=None
     ):
         if working_dir is None:
-            raise ValueError("No working_dir passed to ServiceLoader()")
+            raise ValueError("No working_dir passed to ServiceExtendsResolver()")
 
         self.working_dir = os.path.abspath(working_dir)
 
@@ -235,11 +247,17 @@ class ServiceLoader(object):
         self.service_name = service_name
         self.service_dict['name'] = service_name
 
-    def detect_cycle(self, name):
-        if self.signature(name) in self.already_seen:
-            raise CircularReference(self.already_seen + [self.signature(name)])
+    @property
+    def signature(self):
+        return self.filename, self.service_name
+
+    def detect_cycle(self):
+        if self.signature in self.already_seen:
+            raise CircularReference(self.already_seen + [self.signature])
+
+    def run(self):
+        self.detect_cycle()
 
-    def make_service_dict(self):
         service_dict = dict(self.service_dict)
         env = resolve_environment(self.working_dir, self.service_dict)
         if env:
@@ -252,45 +270,32 @@ class ServiceLoader(object):
         if not self.already_seen:
             validate_against_service_schema(service_dict, self.service_name)
 
-        return process_container_options(self.working_dir, service_dict)
+        return service_dict
 
     def validate_and_construct_extends(self):
         extends = self.service_dict['extends']
         if not isinstance(extends, dict):
             extends = {'service': extends}
 
-        validate_extends_file_path(self.service_name, extends, self.filename)
         config_path = self.get_extended_config_path(extends)
         service_name = extends['service']
 
-        config = load_yaml(config_path)
-        validate_top_level_object(config)
-        full_extended_config = interpolate_environment_variables(config)
-
-        validate_extended_service_exists(
-            service_name,
-            full_extended_config,
-            config_path
-        )
-        validate_against_fields_schema(full_extended_config)
-
-        service_config = full_extended_config[service_name]
+        extended_file = process_config_file(
+            ConfigFile.from_filename(config_path),
+            service_name=service_name)
+        service_config = extended_file.config[service_name]
         return config_path, service_config, service_name
 
     def resolve_extends(self, extended_config_path, service_config, service_name):
-        other_working_dir = os.path.dirname(extended_config_path)
-        other_already_seen = self.already_seen + [self.signature(self.service_name)]
-
-        other_loader = ServiceLoader(
-            other_working_dir,
+        resolver = ServiceExtendsResolver(
+            os.path.dirname(extended_config_path),
             extended_config_path,
-            self.service_name,
+            service_name,
             service_config,
-            already_seen=other_already_seen,
+            already_seen=self.already_seen + [self.signature],
         )
 
-        other_loader.detect_cycle(service_name)
-        other_service_dict = other_loader.make_service_dict()
+        other_service_dict = process_service(resolver.working_dir, resolver.run())
         validate_extended_service_dict(
             other_service_dict,
             extended_config_path,
@@ -304,13 +309,11 @@ class ServiceLoader(object):
         need to obtain a full path too or we are extending from a service
         defined in our own file.
         """
+        validate_extends_file_path(self.service_name, extends_options, self.filename)
         if 'file' in extends_options:
             return expand_path(self.working_dir, extends_options['file'])
         return self.filename
 
-    def signature(self, name):
-        return self.filename, name
-
 
 def resolve_environment(working_dir, service_dict):
     """Unpack any environment variables from an env_file, if set.
@@ -354,7 +357,7 @@ def validate_ulimits(ulimit_config):
                     "than 'hard' value".format(ulimit_config))
 
 
-def process_container_options(working_dir, service_dict):
+def process_service(working_dir, service_dict):
     service_dict = dict(service_dict)
 
     if 'volumes' in service_dict and service_dict.get('volume_driver') is None:

compose/config/validation.py

@@ -96,14 +96,6 @@ def validate_extends_file_path(service_name, extends_options, filename):
         )
 
 
-def validate_extended_service_exists(extended_service_name, full_extended_config, extended_config_path):
-    if extended_service_name not in full_extended_config:
-        msg = (
-            "Cannot extend service '%s' in %s: Service not found"
-        ) % (extended_service_name, extended_config_path)
-        raise ConfigurationError(msg)
-
-
 def get_unsupported_config_msg(service_name, error_key):
     msg = "Unsupported config option for '{}' service: '{}'".format(service_name, error_key)
     if error_key in DOCKER_CONFIG_HINTS:
@@ -264,7 +256,7 @@ def process_errors(errors, service_name=None):
                             msg))
                 else:
                     root_msgs.append(
-                        "Service '{}' doesn\'t have any configuration options. "
+                        "Service \"{}\" doesn't have any configuration options. "
                         "All top level keys in your docker-compose.yml must map "
                         "to a dictionary of configuration options.'".format(service_name))
             elif error.validator == 'required':

tests/fixtures/extends/circle-1.yml

@@ -5,7 +5,7 @@ bar:
 web:
   extends:
     file: circle-2.yml
-    service: web
+    service: other
 baz:
   image: busybox
 quux:

tests/fixtures/extends/circle-2.yml

@@ -2,7 +2,7 @@ foo:
   image: busybox
 bar:
   image: busybox
-web:
+other:
   extends:
     file: circle-1.yml
     service: web

tests/integration/service_test.py

@@ -842,7 +842,13 @@ class ServiceTest(DockerClientTestCase):
             environment=['ONE=1', 'TWO=2', 'THREE=3'],
             env_file=['tests/fixtures/env/one.env', 'tests/fixtures/env/two.env'])
         env = create_and_start_container(service).environment
-        for k, v in {'ONE': '1', 'TWO': '2', 'THREE': '3', 'FOO': 'baz', 'DOO': 'dah'}.items():
+        for k, v in {
+            'ONE': '1',
+            'TWO': '2',
+            'THREE': '3',
+            'FOO': 'baz',
+            'DOO': 'dah'
+        }.items():
             self.assertEqual(env[k], v)
 
     @mock.patch.dict(os.environ)
@@ -850,9 +856,22 @@ class ServiceTest(DockerClientTestCase):
         os.environ['FILE_DEF'] = 'E1'
         os.environ['FILE_DEF_EMPTY'] = 'E2'
         os.environ['ENV_DEF'] = 'E3'
-        service = self.create_service('web', environment={'FILE_DEF': 'F1', 'FILE_DEF_EMPTY': '', 'ENV_DEF': None, 'NO_DEF': None})
+        service = self.create_service(
+            'web',
+            environment={
+                'FILE_DEF': 'F1',
+                'FILE_DEF_EMPTY': '',
+                'ENV_DEF': None,
+                'NO_DEF': None
+            }
+        )
         env = create_and_start_container(service).environment
-        for k, v in {'FILE_DEF': 'F1', 'FILE_DEF_EMPTY': '', 'ENV_DEF': 'E3', 'NO_DEF': ''}.items():
+        for k, v in {
+            'FILE_DEF': 'F1',
+            'FILE_DEF_EMPTY': '',
+            'ENV_DEF': 'E3',
+            'NO_DEF': ''
+        }.items():
             self.assertEqual(env[k], v)
 
     def test_with_high_enough_api_version_we_get_default_network_mode(self):

tests/integration/testcases.py

@@ -7,7 +7,8 @@ from pytest import skip
 
 from .. import unittest
 from compose.cli.docker_client import docker_client
-from compose.config.config import ServiceLoader
+from compose.config.config import process_service
+from compose.config.config import resolve_environment
 from compose.const import LABEL_PROJECT
 from compose.progress_stream import stream_output
 from compose.service import Service
@@ -42,23 +43,15 @@ class DockerClientTestCase(unittest.TestCase):
         if 'command' not in kwargs:
             kwargs['command'] = ["top"]
 
-        workaround_options = {}
-        for option in ['links', 'volumes_from', 'net']:
-            if option in kwargs:
-                workaround_options[option] = kwargs.pop(option, None)
-
-        options = ServiceLoader(
-            working_dir='.',
-            filename=None,
-            service_name=name,
-            service_dict=kwargs
-        ).make_service_dict()
-        options.update(workaround_options)
+        # TODO: remove this once #2299 is fixed
+        kwargs['name'] = name
 
+        options = process_service('.', kwargs)
+        options['environment'] = resolve_environment('.', kwargs)
         labels = options.setdefault('labels', {})
         labels['com.docker.compose.test-name'] = self.id()
 
-        return Service(project='composetest', client=self.client, **options)
+        return Service(client=self.client, project='composetest', **options)
 
     def check_build(self, *args, **kwargs):
         kwargs.setdefault('rm', True)

tests/unit/config/config_test.py

@@ -18,13 +18,14 @@ from tests import unittest
 
 def make_service_dict(name, service_dict, working_dir, filename=None):
     """
-    Test helper function to construct a ServiceLoader
+    Test helper function to construct a ServiceExtendsResolver
     """
-    return config.ServiceLoader(
+    resolver = config.ServiceExtendsResolver(
         working_dir=working_dir,
         filename=filename,
         service_name=name,
-        service_dict=service_dict).make_service_dict()
+        service_dict=service_dict)
+    return config.process_service(working_dir, resolver.run())
 
 
 def service_sort(services):
@@ -195,6 +196,19 @@ class ConfigTest(unittest.TestCase):
         ]
         self.assertEqual(service_sort(service_dicts), service_sort(expected))
 
+    def test_load_with_multiple_files_and_invalid_override(self):
+        base_file = config.ConfigFile(
+            'base.yaml',
+            {'web': {'image': 'example/web'}})
+        override_file = config.ConfigFile(
+            'override.yaml',
+            {'bogus': 'thing'})
+        details = config.ConfigDetails('.', [base_file, override_file])
+
+        with pytest.raises(ConfigurationError) as exc:
+            config.load(details)
+        assert 'Service "bogus" doesn\'t have any configuration' in exc.exconly()
+
     def test_config_valid_service_names(self):
         for valid_name in ['_', '-', '.__.', '_what-up.', 'what_.up----', 'whatup']:
             config.load(
@@ -1066,18 +1080,19 @@ class ExtendsTest(unittest.TestCase):
         ]))
 
     def test_circular(self):
-        try:
+        with pytest.raises(config.CircularReference) as exc:
             load_from_filename('tests/fixtures/extends/circle-1.yml')
-            raise Exception("Expected config.CircularReference to be raised")
-        except config.CircularReference as e:
-            self.assertEqual(
-                [(os.path.basename(filename), service_name) for (filename, service_name) in e.trail],
-                [
-                    ('circle-1.yml', 'web'),
-                    ('circle-2.yml', 'web'),
-                    ('circle-1.yml', 'web'),
-                ],
-            )
+
+        path = [
+            (os.path.basename(filename), service_name)
+            for (filename, service_name) in exc.value.trail
+        ]
+        expected = [
+            ('circle-1.yml', 'web'),
+            ('circle-2.yml', 'other'),
+            ('circle-1.yml', 'web'),
+        ]
+        self.assertEqual(path, expected)
 
     def test_extends_validation_empty_dictionary(self):
         with self.assertRaisesRegexp(ConfigurationError, 'service'):