From f47431d591fddd3a7791fc2e4fbafeae7cc6496a Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Sat, 14 Feb 2015 14:09:55 -0500 Subject: [PATCH] Resolves #927 - fix merging command line environment with a list in the config Signed-off-by: Daniel Nephin --- compose/cli/main.py | 24 +++++++-------- compose/service.py | 26 +++++++++++----- tests/unit/cli_test.py | 33 ++++++++++++++++++++- tests/unit/service_test.py | 61 +++++++++++++++++++++++++------------- tox.ini | 4 +-- 5 files changed, 103 insertions(+), 45 deletions(-) diff --git a/compose/cli/main.py b/compose/cli/main.py index 858f19479..eee59bb74 100644 --- a/compose/cli/main.py +++ b/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') diff --git a/compose/service.py b/compose/service.py index a21b78b76..df6dd6ab5 100644 --- a/compose/service.py +++ b/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): diff --git a/tests/unit/cli_test.py b/tests/unit/cli_test.py index 57e2f327f..d9a191ef0 100644 --- a/tests/unit/cli_test.py +++ b/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() diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index c7b122fc2..012a51ab6 100644 --- a/tests/unit/service_test.py +++ b/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' diff --git a/tox.ini b/tox.ini index a20d984b7..6e83fc414 100644 --- a/tox.ini +++ b/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