From d5a2d37d059cd1de5865fc5eaa05461d94aadfff Mon Sep 17 00:00:00 2001 From: Joffrey F Date: Tue, 7 Mar 2017 17:47:41 -0800 Subject: [PATCH] Properly resolve build args against host environment values Signed-off-by: Joffrey F --- compose/cli/main.py | 4 +++ compose/config/__init__.py | 2 +- compose/config/config.py | 12 ++----- compose/service.py | 11 +++--- tests/integration/service_test.py | 20 ++++++++++- tests/unit/config/config_test.py | 58 ++----------------------------- tests/unit/service_test.py | 18 +++++----- 7 files changed, 43 insertions(+), 82 deletions(-) diff --git a/compose/cli/main.py b/compose/cli/main.py index 78e3d84b5..4d31e25e8 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -22,6 +22,7 @@ from ..bundle import MissingDigests from ..bundle import serialize_bundle from ..config import ConfigurationError from ..config import parse_environment +from ..config import resolve_build_args from ..config.environment import Environment from ..config.serialize import serialize_config from ..config.types import VolumeSpec @@ -219,6 +220,9 @@ class TopLevelCommand(object): """ service_names = options['SERVICE'] build_args = options.get('--build-arg', None) + if build_args: + environment = Environment.from_env_file(self.project_dir) + build_args = resolve_build_args(build_args, environment) if not service_names and build_args: raise UserError("Need service name for --build-arg option") diff --git a/compose/config/__init__.py b/compose/config/__init__.py index b6e5e8d38..b629edf66 100644 --- a/compose/config/__init__.py +++ b/compose/config/__init__.py @@ -7,6 +7,6 @@ from .config import ConfigurationError from .config import DOCKER_CONFIG_KEYS from .config import find from .config import load -from .config import merge_build_args from .config import merge_environment from .config import parse_environment +from .config import resolve_build_args diff --git a/compose/config/config.py b/compose/config/config.py index 718d3bf02..c85ffdabb 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -602,14 +602,8 @@ def resolve_environment(service_dict, environment=None): return dict(resolve_env_var(k, v, environment) for k, v in six.iteritems(env)) -def merge_build_args(base, override, environment): - override_args = parse_build_arguments(override) - override_dict = dict(resolve_env_var(k, v, environment) for k, v in six.iteritems(override_args)) - base.update(override_dict) - - -def resolve_build_args(build, environment): - args = parse_build_arguments(build.get('args')) +def resolve_build_args(buildargs, environment): + args = parse_build_arguments(buildargs) return dict(resolve_env_var(k, v, environment) for k, v in six.iteritems(args)) @@ -1057,7 +1051,7 @@ def normalize_build(service_dict, working_dir, environment): build.update(service_dict['build']) if 'args' in build: build['args'] = build_string_dict( - resolve_build_args(build, environment) + resolve_build_args(build.get('args'), environment) ) service_dict['build'] = build diff --git a/compose/service.py b/compose/service.py index a889dd58c..29ee70476 100644 --- a/compose/service.py +++ b/compose/service.py @@ -21,7 +21,6 @@ from . import __version__ from . import const from . import progress_stream from .config import DOCKER_CONFIG_KEYS -from .config import merge_build_args from .config import merge_environment from .config.types import ServicePort from .config.types import VolumeSpec @@ -804,14 +803,14 @@ class Service(object): return [build_spec(secret) for secret in self.secrets] - def build(self, no_cache=False, pull=False, force_rm=False, build_args=None): + def build(self, no_cache=False, pull=False, force_rm=False, build_args_override=None): log.info('Building %s' % self.name) build_opts = self.options.get('build', {}) - self_args_opts = build_opts.get('args', None) - if self_args_opts and build_args: - merge_build_args(self_args_opts, build_args, self.options.get('environment')) + build_args = build_opts.get('args', {}).copy() + if build_args_override: + build_args.update(build_args_override) # python2 os.stat() doesn't support unicode on some UNIX, so we # encode it to a bytestring to be safe @@ -829,7 +828,7 @@ class Service(object): nocache=no_cache, dockerfile=build_opts.get('dockerfile', None), cache_from=build_opts.get('cache_from', None), - buildargs=self_args_opts + buildargs=build_args ) try: diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index ddc2f3bae..12ec8a993 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -597,12 +597,30 @@ class ServiceTest(DockerClientTestCase): with open(os.path.join(base_dir, 'Dockerfile'), 'w') as f: f.write("FROM busybox\n") f.write("ARG build_version\n") + f.write("RUN echo ${build_version}\n") service = self.create_service('buildwithargs', build={'context': text_type(base_dir), 'args': {"build_version": "1"}}) service.build() assert service.image() + assert "build_version=1" in service.image()['ContainerConfig']['Cmd'] + + def test_build_with_build_args_override(self): + base_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, base_dir) + + with open(os.path.join(base_dir, 'Dockerfile'), 'w') as f: + f.write("FROM busybox\n") + f.write("ARG build_version\n") + f.write("RUN echo ${build_version}\n") + + service = self.create_service('buildwithargs', + build={'context': text_type(base_dir), + 'args': {"build_version": "1"}}) + service.build(build_args_override={'build_version': '2'}) + assert service.image() + assert "build_version=2" in service.image()['ContainerConfig']['Cmd'] def test_start_container_stays_unprivileged(self): service = self.create_service('web') @@ -1057,7 +1075,7 @@ class ServiceTest(DockerClientTestCase): one_off_container = service.create_container(one_off=True) self.assertNotEqual(one_off_container.name, 'my-web-container') - @pytest.mark.skipif(True, reason="Broken on 1.11.0rc1") + @pytest.mark.skipif(True, reason="Broken on 1.11.0 - 17.03.0") def test_log_drive_invalid(self): service = self.create_service('web', logging={'driver': 'xxx'}) expected_error_msg = "logger: no log driver named 'xxx' is registered" diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 3e3bd2bbf..d7d342afa 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -15,7 +15,6 @@ import yaml from ...helpers import build_config_details from compose.config import config from compose.config import types -from compose.config.config import merge_build_args from compose.config.config import resolve_build_args from compose.config.config import resolve_environment from compose.config.config import V1 @@ -1523,7 +1522,7 @@ class ConfigTest(unittest.TestCase): assert actual == { 'image': 'alpine:edge', 'volumes': ['.:/app'], - 'ports': ['5432'] + 'ports': types.ServicePort.parse('5432') } def test_merge_service_dicts_heterogeneous_2(self): @@ -1542,40 +1541,7 @@ class ConfigTest(unittest.TestCase): assert actual == { 'image': 'alpine:edge', 'volumes': ['.:/app'], - 'ports': ['5432'] - } - - def test_merge_build_args(self): - base = { - 'build': { - 'context': '.', - 'args': { - 'ONE': '1', - 'TWO': '2', - }, - } - } - override = { - 'build': { - 'args': { - 'TWO': 'dos', - 'THREE': '3', - }, - } - } - actual = config.merge_service_dicts( - base, - override, - DEFAULT_VERSION) - assert actual == { - 'build': { - 'context': '.', - 'args': { - 'ONE': '1', - 'TWO': 'dos', - 'THREE': '3', - }, - } + 'ports': types.ServicePort.parse('5432') } def test_merge_logging_v1(self): @@ -2878,28 +2844,10 @@ class EnvTest(unittest.TestCase): } } self.assertEqual( - resolve_build_args(build, Environment.from_env_file(build['context'])), + resolve_build_args(build['args'], Environment.from_env_file(build['context'])), {'arg1': 'value1', 'empty_arg': '', 'env_arg': 'value2', 'no_env': None}, ) - @mock.patch.dict(os.environ) - def test_merge_build_args(self): - os.environ['env_arg'] = 'value2' - - base = { - 'arg1': 'arg1_value', - 'arg2': 'arg2_value' - } - override = { - 'arg1': 'arg1_new_value', - 'arg2': 'arg2_value' - } - self.assertEqual(base['arg1'], 'arg1_value') - - merge_build_args(base, override, os.environ) - - self.assertEqual(base['arg1'], 'arg1_new_value') - @pytest.mark.xfail(IS_WINDOWS_PLATFORM, reason='paths use slash') @mock.patch.dict(os.environ) def test_resolve_path(self): diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index ca7410414..b3c8c4d7d 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -461,7 +461,7 @@ class ServiceTest(unittest.TestCase): forcerm=False, nocache=False, rm=True, - buildargs=None, + buildargs={}, cache_from=None, ) @@ -498,7 +498,7 @@ class ServiceTest(unittest.TestCase): forcerm=False, nocache=False, rm=True, - buildargs=None, + buildargs={}, cache_from=None, ) @@ -518,19 +518,17 @@ class ServiceTest(unittest.TestCase): b'{"stream": "Successfully built 12345"}', ] - build_args = [ - 'arg1=arg1_new_value', - 'arg2=arg2_value' - ] + build_args = { + 'arg1': 'arg1_new_value', + } service = Service('foo', client=self.mock_client, build={'context': '.', 'args': {'arg1': 'arg1', 'arg2': 'arg2'}}) - service.build(build_args=build_args) + service.build(build_args_override=build_args) called_build_args = self.mock_client.build.call_args[1]['buildargs'] - for arg in called_build_args: - if "arg1=" in arg: - self.assertEquals(arg, 'arg1=arg1_new_value') + assert called_build_args['arg1'] == build_args['arg1'] + assert called_build_args['arg2'] == 'arg2' def test_config_dict(self): self.mock_client.inspect_image.return_value = {'Id': 'abcd'}