Merge pull request #2450 from dnephin/fix_env_file_ordering

Fix env_file overrides with extends

compose/config/config.py

@@ -324,16 +324,13 @@ class ServiceExtendsResolver(object):
         return filename
 
 
-def resolve_environment(service_config):
+def resolve_environment(service_dict):
     """Unpack any environment variables from an env_file, if set.
     Interpolate environment values if set.
     """
-    service_dict = service_config.config
-
     env = {}
-    if 'env_file' in service_dict:
-        for env_file in get_env_files(service_config.working_dir, service_dict):
-            env.update(env_vars_from_file(env_file))
+    for env_file in service_dict.get('env_file', []):
+        env.update(env_vars_from_file(env_file))
 
     env.update(parse_environment(service_dict.get('environment')))
     return dict(resolve_env_var(k, v) for k, v in six.iteritems(env))
@@ -370,9 +367,11 @@ def process_service(service_config):
     working_dir = service_config.working_dir
     service_dict = dict(service_config.config)
 
-    if 'environment' in service_dict or 'env_file' in service_dict:
-        service_dict['environment'] = resolve_environment(service_config)
-        service_dict.pop('env_file', None)
+    if 'env_file' in service_dict:
+        service_dict['env_file'] = [
+            expand_path(working_dir, path)
+            for path in to_list(service_dict['env_file'])
+        ]
 
     if 'volumes' in service_dict and service_dict.get('volume_driver') is None:
         service_dict['volumes'] = resolve_volume_paths(working_dir, service_dict)
@@ -396,6 +395,10 @@ def process_service(service_config):
 def finalize_service(service_config):
     service_dict = dict(service_config.config)
 
+    if 'environment' in service_dict or 'env_file' in service_dict:
+        service_dict['environment'] = resolve_environment(service_dict)
+        service_dict.pop('env_file', None)
+
     if 'volumes_from' in service_dict:
         service_dict['volumes_from'] = [
             VolumeFromSpec.parse(vf) for vf in service_dict['volumes_from']]
@@ -440,7 +443,7 @@ def merge_service_dicts(base, override):
     for field in ['ports', 'expose', 'external_links']:
         merge_field(field, operator.add, default=[])
 
-    for field in ['dns', 'dns_search']:
+    for field in ['dns', 'dns_search', 'env_file']:
         merge_field(field, merge_list_or_string)
 
     already_merged_keys = set(d) | {'image', 'build'}
@@ -468,17 +471,6 @@ def merge_environment(base, override):
     return env
 
 
-def get_env_files(working_dir, options):
-    if 'env_file' not in options:
-        return {}
-
-    env_files = options.get('env_file', [])
-    if not isinstance(env_files, list):
-        env_files = [env_files]
-
-    return [expand_path(working_dir, path) for path in env_files]
-
-
 def parse_environment(environment):
     if not environment:
         return {}

tests/integration/testcases.py

@@ -7,7 +7,6 @@ from pytest import skip
 from .. import unittest
 from compose.cli.docker_client import docker_client
 from compose.config.config import resolve_environment
-from compose.config.config import ServiceConfig
 from compose.const import LABEL_PROJECT
 from compose.progress_stream import stream_output
 from compose.service import Service
@@ -39,9 +38,7 @@ class DockerClientTestCase(unittest.TestCase):
         if 'command' not in kwargs:
             kwargs['command'] = ["top"]
 
-        service_config = ServiceConfig('.', None, name, kwargs)
-        kwargs['environment'] = resolve_environment(service_config)
-
+        kwargs['environment'] = resolve_environment(kwargs)
         labels = dict(kwargs.setdefault('labels', {}))
         labels['com.docker.compose.test-name'] = self.id()
 

tests/unit/config/config_test.py

@@ -10,6 +10,7 @@ import py
 import pytest
 
 from compose.config import config
+from compose.config.config import resolve_environment
 from compose.config.errors import ConfigurationError
 from compose.config.types import VolumeSpec
 from compose.const import IS_WINDOWS_PLATFORM
@@ -973,65 +974,54 @@ class EnvTest(unittest.TestCase):
         os.environ['FILE_DEF_EMPTY'] = 'E2'
         os.environ['ENV_DEF'] = 'E3'
 
-        service_dict = make_service_dict(
-            'foo', {
-                'build': '.',
-                'environment': {
-                    'FILE_DEF': 'F1',
-                    'FILE_DEF_EMPTY': '',
-                    'ENV_DEF': None,
-                    'NO_DEF': None
-                },
+        service_dict = {
+            'build': '.',
+            'environment': {
+                'FILE_DEF': 'F1',
+                'FILE_DEF_EMPTY': '',
+                'ENV_DEF': None,
+                'NO_DEF': None
             },
-            'tests/'
-        )
-
+        }
         self.assertEqual(
-            service_dict['environment'],
+            resolve_environment(service_dict),
             {'FILE_DEF': 'F1', 'FILE_DEF_EMPTY': '', 'ENV_DEF': 'E3', 'NO_DEF': ''},
         )
 
-    def test_env_from_file(self):
-        service_dict = make_service_dict(
-            'foo',
-            {'build': '.', 'env_file': 'one.env'},
-            'tests/fixtures/env',
-        )
+    def test_resolve_environment_from_env_file(self):
         self.assertEqual(
-            service_dict['environment'],
+            resolve_environment({'env_file': ['tests/fixtures/env/one.env']}),
             {'ONE': '2', 'TWO': '1', 'THREE': '3', 'FOO': 'bar'},
         )
 
-    def test_env_from_multiple_files(self):
-        service_dict = make_service_dict(
-            'foo',
-            {'build': '.', 'env_file': ['one.env', 'two.env']},
-            'tests/fixtures/env',
-        )
+    def test_resolve_environment_with_multiple_env_files(self):
+        service_dict = {
+            'env_file': [
+                'tests/fixtures/env/one.env',
+                'tests/fixtures/env/two.env'
+            ]
+        }
         self.assertEqual(
-            service_dict['environment'],
+            resolve_environment(service_dict),
             {'ONE': '2', 'TWO': '1', 'THREE': '3', 'FOO': 'baz', 'DOO': 'dah'},
         )
 
-    def test_env_nonexistent_file(self):
-        options = {'env_file': 'nonexistent.env'}
-        self.assertRaises(
-            ConfigurationError,
-            lambda: make_service_dict('foo', options, 'tests/fixtures/env'),
-        )
+    def test_resolve_environment_nonexistent_file(self):
+        with pytest.raises(ConfigurationError) as exc:
+            config.load(build_config_details(
+                {'foo': {'image': 'example', 'env_file': 'nonexistent.env'}},
+                working_dir='tests/fixtures/env'))
+
+            assert 'Couldn\'t find env file' in exc.exconly()
+            assert 'nonexistent.env' in exc.exconly()
 
     @mock.patch.dict(os.environ)
-    def test_resolve_environment_from_file(self):
+    def test_resolve_environment_from_env_file_with_empty_values(self):
         os.environ['FILE_DEF'] = 'E1'
         os.environ['FILE_DEF_EMPTY'] = 'E2'
         os.environ['ENV_DEF'] = 'E3'
-        service_dict = make_service_dict(
-            'foo',
-            {'build': '.', 'env_file': 'resolve.env'},
-            'tests/fixtures/env',
-        )
         self.assertEqual(
-            service_dict['environment'],
+            resolve_environment({'env_file': ['tests/fixtures/env/resolve.env']}),
             {
                 'FILE_DEF': u'bär',
                 'FILE_DEF_EMPTY': '',
@@ -1378,6 +1368,8 @@ class ExtendsTest(unittest.TestCase):
                     - 'envs'
                 environment:
                     - SECRET
+                    - TEST_ONE=common
+                    - TEST_TWO=common
         """)
         tmpdir.join('docker-compose.yml').write("""
             ext:
@@ -1388,12 +1380,20 @@ class ExtendsTest(unittest.TestCase):
                     - 'envs'
                 environment:
                     - THING
+                    - TEST_ONE=top
         """)
         commondir.join('envs').write("""
-            COMMON_ENV_FILE=1
+            COMMON_ENV_FILE
+            TEST_ONE=common-env-file
+            TEST_TWO=common-env-file
+            TEST_THREE=common-env-file
+            TEST_FOUR=common-env-file
         """)
         tmpdir.join('envs').write("""
-            FROM_ENV_FILE=1
+            TOP_ENV_FILE
+            TEST_ONE=top-env-file
+            TEST_TWO=top-env-file
+            TEST_THREE=top-env-file
         """)
 
         expected = [
@@ -1402,15 +1402,21 @@ class ExtendsTest(unittest.TestCase):
                 'image': 'example/app',
                 'environment': {
                     'SECRET': 'secret',
-                    'FROM_ENV_FILE': '1',
-                    'COMMON_ENV_FILE': '1',
+                    'TOP_ENV_FILE': 'secret',
+                    'COMMON_ENV_FILE': 'secret',
                     'THING': 'thing',
+                    'TEST_ONE': 'top',
+                    'TEST_TWO': 'common',
+                    'TEST_THREE': 'top-env-file',
+                    'TEST_FOUR': 'common-env-file',
                 },
             },
         ]
         with mock.patch.dict(os.environ):
             os.environ['SECRET'] = 'secret'
             os.environ['THING'] = 'thing'
+            os.environ['COMMON_ENV_FILE'] = 'secret'
+            os.environ['TOP_ENV_FILE'] = 'secret'
             config = load_from_filename(str(tmpdir.join('docker-compose.yml')))
 
         assert config == expected