Properly resolve build args against host environment values

Signed-off-by: Joffrey F <joffrey@docker.com>

7 files changed, 43 insertions, 82 deletions

diff --git a/compose/cli/main.py b/compose/cli/main.py
index 78e3d84b..4d31e25e 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 b6e5e8d3..b629edf6 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 718d3bf0..c85ffdab 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 a889dd58..29ee7047 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 ddc2f3ba..12ec8a99 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 3e3bd2bb..d7d342af 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 ca741041..b3c8c4d7 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'}