Resolves #927 - fix merging command line environment with a list in the config

Signed-off-by: Daniel Nephin <dnephin@gmail.com>

compose/cli/main.py

@@ -1,26 +1,25 @@
 from __future__ import print_function
 from __future__ import unicode_literals
+from inspect import getdoc
+from operator import attrgetter
 import logging
-import sys
 import re
 import signal
-from operator import attrgetter
+import sys
 
-from inspect import getdoc
+from docker.errors import APIError
 import dockerpty
 
 from .. import __version__
 from ..project import NoSuchService, ConfigurationError
-from ..service import BuildError, CannotBeScaledError
+from ..service import BuildError, CannotBeScaledError, parse_environment
 from .command import Command
+from .docopt_command import NoSuchCommand
+from .errors import UserError
 from .formatter import Formatter
 from .log_printer import LogPrinter
 from .utils import yesno
 
-from docker.errors import APIError
-from .errors import UserError
-from .docopt_command import NoSuchCommand
-
 log = logging.getLogger(__name__)
 
 
@@ -316,11 +315,10 @@ class TopLevelCommand(Command):
         }
 
         if options['-e']:
-            for option in options['-e']:
-                if 'environment' not in service.options:
-                    service.options['environment'] = {}
-                k, v = option.split('=', 1)
-                service.options['environment'][k] = v
+            # Merge environment from config with -e command line
+            container_options['environment'] = dict(
+                parse_environment(service.options.get('environment')),
+                **parse_environment(options['-e']))
 
         if options['--entrypoint']:
             container_options['entrypoint'] = options.get('--entrypoint')

compose/service.py

@@ -8,6 +8,7 @@ from operator import attrgetter
 import sys
 
 from docker.errors import APIError
+import six
 
 from .container import Container, get_container_name
 from .progress_stream import stream_output, StreamOutputError
@@ -450,7 +451,7 @@ class Service(object):
                 (parse_volume_spec(v).internal, {})
                 for v in container_options['volumes'])
 
-        container_options['environment'] = merge_environment(container_options)
+        container_options['environment'] = build_environment(container_options)
 
         if self.can_be_built():
             container_options['image'] = self.full_name
@@ -629,19 +630,28 @@ def get_env_files(options):
     return env_files
 
 
-def merge_environment(options):
+def build_environment(options):
     env = {}
 
     for f in get_env_files(options):
         env.update(env_vars_from_file(f))
 
-    if 'environment' in options:
-        if isinstance(options['environment'], list):
-            env.update(dict(split_env(e) for e in options['environment']))
-        else:
-            env.update(options['environment'])
+    env.update(parse_environment(options.get('environment')))
+    return dict(resolve_env(k, v) for k, v in six.iteritems(env))
 
-    return dict(resolve_env(k, v) for k, v in env.items())
+
+def parse_environment(environment):
+    if not environment:
+        return {}
+
+    if isinstance(environment, list):
+        return dict(split_env(e) for e in environment)
+
+    if isinstance(environment, dict):
+        return environment
+
+    raise ConfigError("environment \"%s\" must be a list or mapping," %
+                      environment)
 
 
 def split_env(env):

tests/unit/cli_test.py

@@ -6,12 +6,14 @@ import tempfile
 import shutil
 from .. import unittest
 
+import docker
 import mock
+from six import StringIO
 
 from compose.cli import main
 from compose.cli.main import TopLevelCommand
 from compose.cli.errors import ComposeFileNotFound
-from six import StringIO
+from compose.service import Service
 
 
 class CLITestCase(unittest.TestCase):
@@ -103,6 +105,35 @@ class CLITestCase(unittest.TestCase):
         self.assertEqual(logging.getLogger().level, logging.DEBUG)
         self.assertEqual(logging.getLogger('requests').propagate, False)
 
+    @mock.patch('compose.cli.main.dockerpty', autospec=True)
+    def test_run_with_environment_merged_with_options_list(self, mock_dockerpty):
+        command = TopLevelCommand()
+        mock_client = mock.create_autospec(docker.Client)
+        mock_project = mock.Mock()
+        mock_project.get_service.return_value = Service(
+            'service',
+            client=mock_client,
+            environment=['FOO=ONE', 'BAR=TWO'],
+            image='someimage')
+
+        command.run(mock_project, {
+            'SERVICE': 'service',
+            'COMMAND': None,
+            '-e': ['BAR=NEW', 'OTHER=THREE'],
+            '--no-deps': None,
+            '--allow-insecure-ssl': None,
+            '-d': True,
+            '-T': None,
+            '--entrypoint': None,
+            '--service-ports': None,
+            '--rm': None,
+        })
+
+        _, _, call_kwargs = mock_client.create_container.mock_calls[0]
+        self.assertEqual(
+            call_kwargs['environment'],
+            {'FOO': 'ONE', 'BAR': 'NEW', 'OTHER': 'THREE'})
+
 
 def get_config_filename_for_files(filenames):
     project_dir = tempfile.mkdtemp()

tests/unit/service_test.py

@@ -11,14 +11,15 @@ from requests import Response
 from compose import Service
 from compose.container import Container
 from compose.service import (
-    ConfigError,
-    split_port,
-    build_port_bindings,
-    parse_volume_spec,
-    build_volume_binding,
     APIError,
+    ConfigError,
+    build_port_bindings,
+    build_volume_binding,
     get_container_name,
+    parse_environment,
     parse_repository_tag,
+    parse_volume_spec,
+    split_port,
 )
 
 
@@ -326,28 +327,47 @@ class ServiceEnvironmentTest(unittest.TestCase):
         self.mock_client = mock.create_autospec(docker.Client)
         self.mock_client.containers.return_value = []
 
-    def test_parse_environment(self):
-        service = Service('foo',
-                environment=['NORMAL=F1', 'CONTAINS_EQUALS=F=2', 'TRAILING_EQUALS='],
-                client=self.mock_client,
-                image='image_name',
-            )
-        options = service._get_container_create_options({})
+    def test_parse_environment_as_list(self):
+        environment =[
+            'NORMAL=F1',
+            'CONTAINS_EQUALS=F=2',
+            'TRAILING_EQUALS='
+        ]
         self.assertEqual(
-            options['environment'],
-            {'NORMAL': 'F1', 'CONTAINS_EQUALS': 'F=2', 'TRAILING_EQUALS': ''}
-            )
+            parse_environment(environment),
+            {'NORMAL': 'F1', 'CONTAINS_EQUALS': 'F=2', 'TRAILING_EQUALS': ''})
+
+    def test_parse_environment_as_dict(self):
+        environment = {
+            'NORMAL': 'F1',
+            'CONTAINS_EQUALS': 'F=2',
+            'TRAILING_EQUALS': None,
+        }
+        self.assertEqual(parse_environment(environment), environment)
+
+    def test_parse_environment_invalid(self):
+        with self.assertRaises(ConfigError):
+            parse_environment('a=b')
+
+    def test_parse_environment_empty(self):
+        self.assertEqual(parse_environment(None), {})
 
     @mock.patch.dict(os.environ)
     def test_resolve_environment(self):
         os.environ['FILE_DEF'] = 'E1'
         os.environ['FILE_DEF_EMPTY'] = 'E2'
         os.environ['ENV_DEF'] = 'E3'
-        service = Service('foo',
-                environment={'FILE_DEF': 'F1', 'FILE_DEF_EMPTY': '', 'ENV_DEF': None, 'NO_DEF': None},
-                client=self.mock_client,
-                image='image_name',
-            )
+        service = Service(
+            'foo',
+            environment={
+                'FILE_DEF': 'F1',
+                'FILE_DEF_EMPTY': '',
+                'ENV_DEF': None,
+                'NO_DEF': None
+            },
+            client=self.mock_client,
+            image='image_name',
+        )
         options = service._get_container_create_options({})
         self.assertEqual(
             options['environment'],
@@ -381,7 +401,6 @@ class ServiceEnvironmentTest(unittest.TestCase):
     def test_env_nonexistent_file(self):
         self.assertRaises(ConfigError, lambda: Service('foo', env_file='tests/fixtures/env/nonexistent.env'))
 
-
     @mock.patch.dict(os.environ)
     def test_resolve_environment_from_file(self):
         os.environ['FILE_DEF'] = 'E1'

tox.ini

@@ -8,9 +8,9 @@ deps =
     -rrequirements-dev.txt
 commands =
     nosetests -v {posargs}
-    flake8 fig
+    flake8 compose
 
 [flake8]
 # ignore line-length for now
 ignore = E501,E203
-exclude = fig/packages
+exclude = compose/packages