From db45723e383a7f958cfca7b9724ebc7d43e75e0a Mon Sep 17 00:00:00 2001 From: Wilton Wang Date: Tue, 16 Mar 2021 22:04:42 -0700 Subject: [PATCH 01/10] Added Latest Tag Check --- tests/integration/buildcmd/build_integ_base.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/buildcmd/build_integ_base.py b/tests/integration/buildcmd/build_integ_base.py index a5c958bfff..b2211e53b4 100644 --- a/tests/integration/buildcmd/build_integ_base.py +++ b/tests/integration/buildcmd/build_integ_base.py @@ -136,6 +136,7 @@ def verify_pulling_only_latest_tag(self, runtime): len(images) > 1, f"Other version of the build image {image_name} was pulled", ) + self.assertEqual(f"public.ecr.aws/sam/build-{runtime}:latest", images[0].tags[0]) def _make_parameter_override_arg(self, overrides): return " ".join(["ParameterKey={},ParameterValue={}".format(key, value) for key, value in overrides.items()]) From f6a6dfb849417d1f731727e1b793c1a314d04e88 Mon Sep 17 00:00:00 2001 From: Wilton Wang Date: Tue, 16 Mar 2021 22:05:02 -0700 Subject: [PATCH 02/10] Added --build-image Option --- samcli/commands/build/build_context.py | 2 + samcli/commands/build/command.py | 44 +++++++++++++++++++ samcli/lib/build/app_builder.py | 24 ++++++---- samcli/local/docker/lambda_build_container.py | 4 +- 4 files changed, 65 insertions(+), 9 deletions(-) diff --git a/samcli/commands/build/build_context.py b/samcli/commands/build/build_context.py index 96c44a5013..9df2081b5d 100644 --- a/samcli/commands/build/build_context.py +++ b/samcli/commands/build/build_context.py @@ -44,6 +44,7 @@ def __init__( skip_pull_image: bool = False, container_env_var: Optional[dict] = None, container_env_var_file: Optional[str] = None, + build_images: Optional[dict] = None, ) -> None: self._resource_identifier = resource_identifier @@ -65,6 +66,7 @@ def __init__( self._cached = cached self._container_env_var = container_env_var self._container_env_var_file = container_env_var_file + self._build_images = build_images self._function_provider: Optional[SamFunctionProvider] = None self._layer_provider: Optional[SamLayerProvider] = None diff --git a/samcli/commands/build/command.py b/samcli/commands/build/command.py index 080e9839b3..7582b90bf9 100644 --- a/samcli/commands/build/command.py +++ b/samcli/commands/build/command.py @@ -124,6 +124,14 @@ type=click.Path(), # Must be a json file help="Path to environment variable json file (e.g., env_vars.json) to pass into build containers", ) +@click.option( + "--build-image", + "-bi", + default=None, + multiple=True, # Can pass in multiple build images + required=False, + help="", +) @click.option( "--parallel", "-p", @@ -175,6 +183,7 @@ def cli( docker_network: Optional[str], container_env_var: Optional[List[str]], container_env_var_file: Optional[str], + build_image: Optional[str], skip_pull_image: bool, parameter_overrides: dict, config_file: str, @@ -204,6 +213,7 @@ def cli( mode, container_env_var, container_env_var_file, + build_image, ) # pragma: no cover @@ -224,6 +234,7 @@ def do_cli( # pylint: disable=too-many-locals, too-many-statements mode: Optional[str], container_env_var: Optional[List[str]], container_env_var_file: Optional[str], + build_image: Optional[List[str]], ) -> None: """ Implementation of the ``cli`` method @@ -250,6 +261,7 @@ def do_cli( # pylint: disable=too-many-locals, too-many-statements LOG.info("Starting Build inside a container") processed_env_vars = _process_env_var(container_env_var) + processed_build_images = _process_image_options(build_image) with BuildContext( function_identifier, @@ -267,6 +279,7 @@ def do_cli( # pylint: disable=too-many-locals, too-many-statements mode=mode, container_env_var=processed_env_vars, container_env_var_file=container_env_var_file, + build_images=processed_build_images, ) as ctx: try: builder = ApplicationBuilder( @@ -282,6 +295,7 @@ def do_cli( # pylint: disable=too-many-locals, too-many-statements parallel=parallel, container_env_var=processed_env_vars, container_env_var_file=container_env_var_file, + build_images=processed_build_images, ) except FunctionNotFound as ex: raise UserException(str(ex), wrapped_from=ex.__class__.__name__) from ex @@ -418,3 +432,33 @@ def _process_env_var(container_env_var: Optional[List[str]]) -> Dict: processed_env_vars[location_key][env_var_name] = value return processed_env_vars + + +def _process_image_options(image_args: Optional[List[str]]) -> Dict: + """ + Parameters + ---------- + image_args : list + List of command line image options in the format of + Function1=public.ecr.aws/abc/abc:latest or + public.ecr.aws/abc/abc:latest + + Returns + ------- + dictionary + Function as key and the corresponding image URI as value. + Global default image URI is contained in the None key. + """ + build_images: Dict[str, str] = dict() + for build_image_string in image_args: + if "=" in build_image_string: + parts = build_image_string.split("=", 1) + function_name = parts[0] + image_uri = parts[1] + else: + function_name = None + image_uri = build_image_string + + build_images[function_name] = image_uri + + return build_images diff --git a/samcli/lib/build/app_builder.py b/samcli/lib/build/app_builder.py index c957bb1aad..9a7d440ad9 100644 --- a/samcli/lib/build/app_builder.py +++ b/samcli/lib/build/app_builder.py @@ -67,6 +67,7 @@ def __init__( docker_client: Optional[docker.DockerClient] = None, container_env_var: Optional[Dict] = None, container_env_var_file: Optional[str] = None, + build_images: Optional[dict] = None, ) -> None: """ Initialize the class @@ -122,6 +123,7 @@ def __init__( self._colored = Colored() self._container_env_var = container_env_var self._container_env_var_file = container_env_var_file + self._build_images = build_images def build(self) -> Dict[str, str]: """ @@ -520,21 +522,27 @@ def _build_function( # pylint: disable=R1710 options = ApplicationBuilder._get_build_options(function_name, config.language, handler) # By default prefer to build in-process for speed - build_method = self._build_function_in_process if self._container_manager: - build_method = self._build_function_on_container - return build_method( + image = None + if function_name in self._build_images: + image = self._build_images[function_name] + elif None in self._build_images: + image = self._build_images[None] + + return self._build_function_on_container( config, code_dir, artifact_dir, - scratch_dir, manifest_path, runtime, options, container_env_vars, + build_image=image, + ) + else: + return self._build_function_in_process( + config, code_dir, artifact_dir, scratch_dir, manifest_path, runtime, options ) - - return build_method(config, code_dir, artifact_dir, scratch_dir, manifest_path, runtime, options) # pylint: disable=fixme # FIXME: we need to throw an exception here, packagetype could be something else @@ -572,7 +580,6 @@ def _build_function_in_process( manifest_path: str, runtime: str, options: Optional[dict], - container_env_vars: Optional[Dict] = None, ) -> str: builder = LambdaBuilder( @@ -604,11 +611,11 @@ def _build_function_on_container( config: CONFIG, source_dir: str, artifacts_dir: str, - scratch_dir: str, manifest_path: str, runtime: str, options: Optional[Dict], container_env_vars: Optional[Dict] = None, + build_image=None, ) -> str: # _build_function_on_container() is only called when self._container_manager if not None if not self._container_manager: @@ -642,6 +649,7 @@ def _build_function_on_container( executable_search_paths=config.executable_search_paths, mode=self._mode, env_vars=container_env_vars, + image=build_image, ) try: diff --git a/samcli/local/docker/lambda_build_container.py b/samcli/local/docker/lambda_build_container.py index 5d74a210d8..f8316fc948 100644 --- a/samcli/local/docker/lambda_build_container.py +++ b/samcli/local/docker/lambda_build_container.py @@ -36,6 +36,7 @@ def __init__( # pylint: disable=too-many-locals log_level=None, mode=None, env_vars=None, + image=None, ): abs_manifest_path = pathlib.Path(manifest_path).resolve() @@ -74,7 +75,8 @@ def __init__( # pylint: disable=too-many-locals mode, ) - image = LambdaBuildContainer._get_image(runtime) + if image is None: + image = LambdaBuildContainer._get_image(runtime) entry = LambdaBuildContainer._get_entrypoint(request_json) cmd = [] From 3b109b1076525b52dac4828947c26c580182faf4 Mon Sep 17 00:00:00 2001 From: Wilton Wang Date: Thu, 18 Mar 2021 01:03:17 -0700 Subject: [PATCH 03/10] Added Unit Tests --- samcli/commands/build/command.py | 26 ++--- samcli/lib/build/app_builder.py | 32 +++--- tests/unit/commands/buildcmd/test_command.py | 29 ++++- .../unit/commands/samconfig/test_samconfig.py | 53 +++++++++ .../unit/lib/build_module/test_app_builder.py | 108 +++++++++++++++--- 5 files changed, 198 insertions(+), 50 deletions(-) diff --git a/samcli/commands/build/command.py b/samcli/commands/build/command.py index 7582b90bf9..deb113e90b 100644 --- a/samcli/commands/build/command.py +++ b/samcli/commands/build/command.py @@ -4,7 +4,7 @@ import os import logging -from typing import List, Optional, Dict +from typing import List, Optional, Dict, Tuple import click from samcli.cli.context import Context @@ -181,9 +181,9 @@ def cli( parallel: bool, manifest: Optional[str], docker_network: Optional[str], - container_env_var: Optional[List[str]], + container_env_var: Optional[Tuple[str]], container_env_var_file: Optional[str], - build_image: Optional[str], + build_image: Optional[Tuple[str]], skip_pull_image: bool, parameter_overrides: dict, config_file: str, @@ -232,9 +232,9 @@ def do_cli( # pylint: disable=too-many-locals, too-many-statements skip_pull_image: bool, parameter_overrides: Dict, mode: Optional[str], - container_env_var: Optional[List[str]], + container_env_var: Optional[Tuple[str]], container_env_var_file: Optional[str], - build_image: Optional[List[str]], + build_image: Optional[Tuple[str]], ) -> None: """ Implementation of the ``cli`` method @@ -390,12 +390,12 @@ def _get_mode_value_from_envvar(name: str, choices: List[str]) -> Optional[str]: return mode -def _process_env_var(container_env_var: Optional[List[str]]) -> Dict: +def _process_env_var(container_env_var: Optional[Tuple[str]]) -> Dict: """ Parameters ---------- - container_env_var : list - the list of command line env vars received from --container-env-var flag + container_env_var : Tuple + the tuple of command line env vars received from --container-env-var flag Each input format needs to be either function specific format (FuncName.VarName=Value) or global format (VarName=Value) @@ -434,14 +434,14 @@ def _process_env_var(container_env_var: Optional[List[str]]) -> Dict: return processed_env_vars -def _process_image_options(image_args: Optional[List[str]]) -> Dict: +def _process_image_options(image_args: Optional[Tuple[str]]) -> Dict: """ Parameters ---------- - image_args : list - List of command line image options in the format of - Function1=public.ecr.aws/abc/abc:latest or - public.ecr.aws/abc/abc:latest + image_args : Tuple + Tuple of command line image options in the format of + "Function1=public.ecr.aws/abc/abc:latest" or + "public.ecr.aws/abc/abc:latest" Returns ------- diff --git a/samcli/lib/build/app_builder.py b/samcli/lib/build/app_builder.py index 9a7d440ad9..3ede6b9297 100644 --- a/samcli/lib/build/app_builder.py +++ b/samcli/lib/build/app_builder.py @@ -430,9 +430,8 @@ def _build_layer( # By default prefer to build in-process for speed build_runtime = specified_workflow - build_method = self._build_function_in_process + options = ApplicationBuilder._get_build_options(layer_name, config.language, None) if self._container_manager: - build_method = self._build_function_on_container if config.language == "provided": LOG.warning( "For container layer build, first compatible runtime is chosen as build target for container." @@ -440,18 +439,14 @@ def _build_layer( # Only set to this value if specified workflow is makefile # which will result in config language as provided build_runtime = compatible_runtimes[0] - options = ApplicationBuilder._get_build_options(layer_name, config.language, None) + self._build_function_on_container( + config, code_dir, artifact_subdir, manifest_path, build_runtime, options, container_env_vars + ) + else: + self._build_function_in_process( + config, code_dir, artifact_subdir, scratch_dir, manifest_path, build_runtime, options + ) - build_method( - config, - code_dir, - artifact_subdir, - scratch_dir, - manifest_path, - build_runtime, - options, - container_env_vars, - ) # Not including subfolder in return so that we copy subfolder, instead of copying artifacts inside it. return artifact_dir @@ -524,10 +519,11 @@ def _build_function( # pylint: disable=R1710 # By default prefer to build in-process for speed if self._container_manager: image = None - if function_name in self._build_images: - image = self._build_images[function_name] - elif None in self._build_images: - image = self._build_images[None] + if self._build_images is not None: + if function_name in self._build_images: + image = self._build_images[function_name] + elif None in self._build_images: + image = self._build_images[None] return self._build_function_on_container( config, @@ -537,7 +533,7 @@ def _build_function( # pylint: disable=R1710 runtime, options, container_env_vars, - build_image=image, + image, ) else: return self._build_function_in_process( diff --git a/tests/unit/commands/buildcmd/test_command.py b/tests/unit/commands/buildcmd/test_command.py index ca0e2e2eca..773bab6743 100644 --- a/tests/unit/commands/buildcmd/test_command.py +++ b/tests/unit/commands/buildcmd/test_command.py @@ -5,7 +5,7 @@ from unittest.mock import Mock, patch, call from parameterized import parameterized -from samcli.commands.build.command import do_cli, _get_mode_value_from_envvar, _process_env_var +from samcli.commands.build.command import do_cli, _get_mode_value_from_envvar, _process_env_var, _process_image_options from samcli.commands.exceptions import UserException from samcli.lib.build.app_builder import ( BuildError, @@ -67,6 +67,7 @@ def test_must_succeed_build(self, os_mock, move_template_mock, ApplicationBuilde "mode", (""), "container_env_var_file", + (), ) ApplicationBuilderMock.assert_called_once_with( @@ -82,6 +83,7 @@ def test_must_succeed_build(self, os_mock, move_template_mock, ApplicationBuilde parallel="parallel", container_env_var={}, container_env_var_file="container_env_var_file", + build_images={}, ) builder_mock.build.assert_called_once() builder_mock.update_template.assert_has_calls( @@ -156,6 +158,7 @@ def test_must_catch_known_exceptions(self, exception, wrapped_exception, Applica "mode", (""), "container_env_var_file", + (), ) self.assertEqual(str(ctx.exception), str(exception)) @@ -187,6 +190,7 @@ def test_must_catch_function_not_found_exception(self, ApplicationBuilderMock, B "mode", (""), "container_env_var_file", + (), ) self.assertEqual(str(ctx.exception), "Function Not Found") @@ -257,3 +261,26 @@ def test_none_env_var_does_not_error_out(self): result = _process_env_var(container_env_vars) self.assertEqual(result, {}) + + +class TestImageParsing(TestCase): + def check(self, image_options, expected): + self.assertEqual(_process_image_options(image_options), expected) + + def test_empty_list(self): + self.check([], {}) + + def test_default_image(self): + self.check(["image1"], {None: "image1"}) + + def test_one_function_image(self): + self.check(["Function1=image1"], {"Function1": "image1"}) + + def test_one_function_with_default_image(self): + self.check(["Function1=image1", "image2"], {"Function1": "image1", None: "image2"}) + + def test_two_functions_with_default_image(self): + self.check( + ["Function1=image1", "Function2=image2", "image3"], + {"Function1": "image1", "Function2": "image2", None: "image3"}, + ) diff --git a/tests/unit/commands/samconfig/test_samconfig.py b/tests/unit/commands/samconfig/test_samconfig.py index d27469ec45..c5fcb47fd5 100644 --- a/tests/unit/commands/samconfig/test_samconfig.py +++ b/tests/unit/commands/samconfig/test_samconfig.py @@ -147,6 +147,7 @@ def test_build(self, do_cli_mock): None, (), None, + (), ) @patch("samcli.commands.build.command.do_cli") @@ -198,6 +199,58 @@ def test_build_with_container_env_vars(self, do_cli_mock): None, (), "env_vars_file", + (), + ) + + @patch("samcli.commands.build.command.do_cli") + def test_build_with_build_images(self, do_cli_mock): + config_values = { + "resource_logical_id": "foo", + "template_file": "mytemplate.yaml", + "base_dir": "basedir", + "build_dir": "builddir", + "cache_dir": "cachedir", + "cache": False, + "use_container": True, + "manifest": "requirements.txt", + "docker_network": "mynetwork", + "skip_pull_image": True, + "parameter_overrides": "ParameterKey=Key,ParameterValue=Value ParameterKey=Key2,ParameterValue=Value2", + "build_image": ["Function1=image_1", "image_2"], + } + + with samconfig_parameters(["build"], self.scratch_dir, **config_values) as config_path: + + from samcli.commands.build.command import cli + + LOG.debug(Path(config_path).read_text()) + runner = CliRunner() + result = runner.invoke(cli, []) + + LOG.info(result.output) + LOG.info(result.exception) + if result.exception: + LOG.exception("Command failed", exc_info=result.exc_info) + self.assertIsNone(result.exception) + + do_cli_mock.assert_called_with( + "foo", + str(Path(os.getcwd(), "mytemplate.yaml")), + "basedir", + "builddir", + "cachedir", + True, + True, + False, + False, + "requirements.txt", + "mynetwork", + True, + {"Key": "Value", "Key2": "Value2"}, + None, + (), + None, + ("Function1=image_1", "image_2"), ) @patch("samcli.commands.local.invoke.cli.do_cli") diff --git a/tests/unit/lib/build_module/test_app_builder.py b/tests/unit/lib/build_module/test_app_builder.py index 110eb89493..a18e5dc3b8 100644 --- a/tests/unit/lib/build_module/test_app_builder.py +++ b/tests/unit/lib/build_module/test_app_builder.py @@ -375,9 +375,15 @@ def test_must_build_layer_in_process(self, get_layer_subfolder_mock, osutils_moc self.builder._build_function_in_process = build_function_in_process_mock self.builder._build_layer("layer_name", "code_uri", "python3.8", ["python3.8"], "full_path") - build_function_in_process_mock.assert_called_once() - args, _ = build_function_in_process_mock.call_args_list[0] - self._validate_build_args(args, config_mock) + build_function_in_process_mock.assert_called_once_with( + config_mock, + f"basedir{os.sep}code_uri", + f"full_path{os.sep}python", + f"scratch", + f"basedir{os.sep}code_uri{os.sep}manifest_name", + "python3.8", + None, + ) @patch("samcli.lib.build.app_builder.get_workflow_config") @patch("samcli.lib.build.app_builder.osutils") @@ -397,18 +403,15 @@ def test_must_build_layer_in_container(self, get_layer_subfolder_mock, osutils_m self.builder._build_function_on_container = build_function_on_container_mock self.builder._build_layer("layer_name", "code_uri", "python3.8", ["python3.8"], "full_path") - - build_function_on_container_mock.assert_called_once() - args, _ = build_function_on_container_mock.call_args_list[0] - self._validate_build_args(args, config_mock) - - def _validate_build_args(self, args, config_mock): - self.assertEqual(config_mock, args[0]) - self.assertTrue(args[1].endswith("code_uri")) - self.assertTrue(args[2].endswith("python")) - self.assertEqual("scratch", args[3]) - self.assertTrue(args[4].endswith("manifest_name")) - self.assertEqual("python3.8", args[5]) + build_function_on_container_mock.assert_called_once_with( + config_mock, + f"basedir{os.sep}code_uri", + f"full_path{os.sep}python", + f"basedir{os.sep}code_uri{os.sep}manifest_name", + "python3.8", + None, + None, + ) class TestApplicationBuilder_update_template(TestCase): @@ -864,7 +867,7 @@ def test_must_build_in_container(self, osutils_mock, get_workflow_config_mock): self.builder._build_function(function_name, codeuri, packagetype, runtime, handler, artifacts_dir) self.builder._build_function_on_container.assert_called_with( - config_mock, code_dir, artifacts_dir, scratch_dir, manifest_path, runtime, None, None + config_mock, code_dir, artifacts_dir, manifest_path, runtime, None, None, None ) @patch("samcli.lib.build.app_builder.get_workflow_config") @@ -896,7 +899,75 @@ def test_must_build_in_container_with_env_vars(self, osutils_mock, get_workflow_ ) self.builder._build_function_on_container.assert_called_with( - config_mock, code_dir, artifacts_dir, scratch_dir, manifest_path, runtime, None, {"TEST": "test"} + config_mock, code_dir, artifacts_dir, manifest_path, runtime, None, {"TEST": "test"}, None + ) + + @patch("samcli.lib.build.app_builder.get_workflow_config") + @patch("samcli.lib.build.app_builder.osutils") + def test_must_build_in_container_with_custom_specified_build_image(self, osutils_mock, get_workflow_config_mock): + function_name = "function_name" + codeuri = "path/to/source" + runtime = "runtime" + packagetype = ZIP + scratch_dir = "scratch" + handler = "handler.handle" + image_uri = "image uri" + build_images = {function_name: image_uri} + config_mock = get_workflow_config_mock.return_value = Mock() + config_mock.manifest_name = "manifest_name" + + osutils_mock.mkdir_temp.return_value.__enter__ = Mock(return_value=scratch_dir) + osutils_mock.mkdir_temp.return_value.__exit__ = Mock() + + self.builder._build_function_on_container = Mock() + + code_dir = str(Path("/base/dir/path/to/source").resolve()) + artifacts_dir = str(Path("/build/dir/function_name")) + manifest_path = str(Path(os.path.join(code_dir, config_mock.manifest_name)).resolve()) + + # Settting the container manager will make us use the container + self.builder._container_manager = Mock() + self.builder._build_images = build_images + self.builder._build_function( + function_name, codeuri, packagetype, runtime, handler, artifacts_dir, container_env_vars=None + ) + + self.builder._build_function_on_container.assert_called_with( + config_mock, code_dir, artifacts_dir, manifest_path, runtime, None, None, image_uri + ) + + @patch("samcli.lib.build.app_builder.get_workflow_config") + @patch("samcli.lib.build.app_builder.osutils") + def test_must_build_in_container_with_custom_default_build_image(self, osutils_mock, get_workflow_config_mock): + function_name = "function_name" + codeuri = "path/to/source" + runtime = "runtime" + packagetype = ZIP + scratch_dir = "scratch" + handler = "handler.handle" + image_uri = "image uri" + build_images = {"abc": "efg", None: image_uri} + config_mock = get_workflow_config_mock.return_value = Mock() + config_mock.manifest_name = "manifest_name" + + osutils_mock.mkdir_temp.return_value.__enter__ = Mock(return_value=scratch_dir) + osutils_mock.mkdir_temp.return_value.__exit__ = Mock() + + self.builder._build_function_on_container = Mock() + + code_dir = str(Path("/base/dir/path/to/source").resolve()) + artifacts_dir = str(Path("/build/dir/function_name")) + manifest_path = str(Path(os.path.join(code_dir, config_mock.manifest_name)).resolve()) + + # Settting the container manager will make us use the container + self.builder._container_manager = Mock() + self.builder._build_images = build_images + self.builder._build_function( + function_name, codeuri, packagetype, runtime, handler, artifacts_dir, container_env_vars=None + ) + + self.builder._build_function_on_container.assert_called_with( + config_mock, code_dir, artifacts_dir, manifest_path, runtime, None, None, image_uri ) @@ -971,7 +1042,7 @@ def mock_wait_for_logs(stdout, stderr): self.builder._parse_builder_response.return_value = response result = self.builder._build_function_on_container( - config, "source_dir", "artifacts_dir", "scratch_dir", "manifest_path", "runtime", None + config, "source_dir", "artifacts_dir", "manifest_path", "runtime", None ) self.assertEqual(result, "artifacts_dir") @@ -983,6 +1054,7 @@ def mock_wait_for_logs(stdout, stderr): "source_dir", "manifest_path", "runtime", + image=None, log_level=log_level, optimizations=None, options=None, From 2051d96c1e319e712a78b02a128396aef93dc67f Mon Sep 17 00:00:00 2001 From: Wilton Wang Date: Thu, 18 Mar 2021 01:17:57 -0700 Subject: [PATCH 04/10] Added Help Text and Fixed PyLint --- samcli/commands/build/command.py | 8 +++++++- samcli/lib/build/app_builder.py | 10 ++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/samcli/commands/build/command.py b/samcli/commands/build/command.py index deb113e90b..3ec5081d2c 100644 --- a/samcli/commands/build/command.py +++ b/samcli/commands/build/command.py @@ -130,7 +130,13 @@ default=None, multiple=True, # Can pass in multiple build images required=False, - help="", + help="Docker image URIs for building functions." + "You can specify for each individual function with " + "(--build-image FunctionLogicalID=public.ecr.aws/sam/build-nodejs14.x:latest)." + "You can specify for all functions with just the image URI " + "(--build-image public.ecr.aws/sam/build-nodejs14.x:latest)." + "A combination of the two can be used. If a function does not have build image specified or" + "a image uri for all functions, the default SAM CLI build images will be used.", ) @click.option( "--parallel", diff --git a/samcli/lib/build/app_builder.py b/samcli/lib/build/app_builder.py index 3ede6b9297..b886afc7e5 100644 --- a/samcli/lib/build/app_builder.py +++ b/samcli/lib/build/app_builder.py @@ -104,6 +104,8 @@ def __init__( An optional dictionary of environment variables to pass to the container container_env_var_file : Optional[str] An optional path to file that contains environment variables to pass to the container + build_images : Optional[Dict] + An optional dictionary of build images to be used for building functions """ self._resources_to_build = resources_to_build self._build_dir = build_dir @@ -535,10 +537,10 @@ def _build_function( # pylint: disable=R1710 container_env_vars, image, ) - else: - return self._build_function_in_process( - config, code_dir, artifact_dir, scratch_dir, manifest_path, runtime, options - ) + + return self._build_function_in_process( + config, code_dir, artifact_dir, scratch_dir, manifest_path, runtime, options + ) # pylint: disable=fixme # FIXME: we need to throw an exception here, packagetype could be something else From ab4be563674c94626a631c45c01116ead36fcdb0 Mon Sep 17 00:00:00 2001 From: Wilton Wang Date: Thu, 18 Mar 2021 01:48:20 -0700 Subject: [PATCH 05/10] Fixed Path Validation for Unit Tests --- .../unit/lib/build_module/test_app_builder.py | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/tests/unit/lib/build_module/test_app_builder.py b/tests/unit/lib/build_module/test_app_builder.py index a18e5dc3b8..e991615757 100644 --- a/tests/unit/lib/build_module/test_app_builder.py +++ b/tests/unit/lib/build_module/test_app_builder.py @@ -348,6 +348,14 @@ def test_parallel_and_cached_run_should_pick_parallel_with_cached_strategy( self.assertEqual(result, mock_parallel_build_strategy.build()) +class PathValidator: + def __init__(self, path: str): + self._path = path + + def __eq__(self, other: str): + return bool(other.endswith(self._path)) + + class TestApplicationBuilderForLayerBuild(TestCase): def setUp(self): self.layer1 = Mock() @@ -377,10 +385,10 @@ def test_must_build_layer_in_process(self, get_layer_subfolder_mock, osutils_moc build_function_in_process_mock.assert_called_once_with( config_mock, - f"basedir{os.sep}code_uri", - f"full_path{os.sep}python", - f"scratch", - f"basedir{os.sep}code_uri{os.sep}manifest_name", + PathValidator("code_uri"), + PathValidator("python"), + "scratch", + PathValidator("manifest_name"), "python3.8", None, ) @@ -405,9 +413,9 @@ def test_must_build_layer_in_container(self, get_layer_subfolder_mock, osutils_m self.builder._build_layer("layer_name", "code_uri", "python3.8", ["python3.8"], "full_path") build_function_on_container_mock.assert_called_once_with( config_mock, - f"basedir{os.sep}code_uri", - f"full_path{os.sep}python", - f"basedir{os.sep}code_uri{os.sep}manifest_name", + PathValidator("code_uri"), + PathValidator("python"), + PathValidator("manifest_name"), "python3.8", None, None, From 5bf83471f150f87d274cb73bd459bc948e78bc85 Mon Sep 17 00:00:00 2001 From: Wilton Wang Date: Thu, 18 Mar 2021 11:05:27 -0700 Subject: [PATCH 06/10] Updated Type Hintings --- samcli/commands/build/command.py | 25 ++++++++++--------- samcli/lib/build/app_builder.py | 2 +- .../unit/lib/build_module/test_app_builder.py | 6 ++--- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/samcli/commands/build/command.py b/samcli/commands/build/command.py index 3ec5081d2c..ead091f88a 100644 --- a/samcli/commands/build/command.py +++ b/samcli/commands/build/command.py @@ -4,7 +4,7 @@ import os import logging -from typing import List, Optional, Dict, Tuple +from typing import List, Optional, Dict, Tuple, Union import click from samcli.cli.context import Context @@ -455,16 +455,17 @@ def _process_image_options(image_args: Optional[Tuple[str]]) -> Dict: Function as key and the corresponding image URI as value. Global default image URI is contained in the None key. """ - build_images: Dict[str, str] = dict() - for build_image_string in image_args: - if "=" in build_image_string: - parts = build_image_string.split("=", 1) - function_name = parts[0] - image_uri = parts[1] - else: - function_name = None - image_uri = build_image_string - - build_images[function_name] = image_uri + build_images: Dict[Union[str, None], str] = dict() + if image_args: + for build_image_string in image_args: + function_name: Union[str, None] = None + if "=" in build_image_string: + parts = build_image_string.split("=", 1) + function_name = parts[0] + image_uri = parts[1] + else: + image_uri = build_image_string + + build_images[function_name] = image_uri return build_images diff --git a/samcli/lib/build/app_builder.py b/samcli/lib/build/app_builder.py index b886afc7e5..3f8eef806d 100644 --- a/samcli/lib/build/app_builder.py +++ b/samcli/lib/build/app_builder.py @@ -613,7 +613,7 @@ def _build_function_on_container( runtime: str, options: Optional[Dict], container_env_vars: Optional[Dict] = None, - build_image=None, + build_image: Optional[str] = None, ) -> str: # _build_function_on_container() is only called when self._container_manager if not None if not self._container_manager: diff --git a/tests/unit/lib/build_module/test_app_builder.py b/tests/unit/lib/build_module/test_app_builder.py index e991615757..4fd7acb823 100644 --- a/tests/unit/lib/build_module/test_app_builder.py +++ b/tests/unit/lib/build_module/test_app_builder.py @@ -349,11 +349,11 @@ def test_parallel_and_cached_run_should_pick_parallel_with_cached_strategy( class PathValidator: - def __init__(self, path: str): + def __init__(self, path): self._path = path - def __eq__(self, other: str): - return bool(other.endswith(self._path)) + def __eq__(self, other): + return self._path is None if other is None else other.endswith(self._path) class TestApplicationBuilderForLayerBuild(TestCase): From 240cec2b8259edbc24c37f2d21d643ee261eae54 Mon Sep 17 00:00:00 2001 From: Wilton Wang Date: Fri, 19 Mar 2021 09:53:28 -0700 Subject: [PATCH 07/10] Added _parse_key_value_pair --- samcli/commands/build/command.py | 64 +++++++++++++++++++------------- 1 file changed, 39 insertions(+), 25 deletions(-) diff --git a/samcli/commands/build/command.py b/samcli/commands/build/command.py index ead091f88a..00e9d85dfa 100644 --- a/samcli/commands/build/command.py +++ b/samcli/commands/build/command.py @@ -4,7 +4,7 @@ import os import logging -from typing import List, Optional, Dict, Tuple, Union +from typing import List, Optional, Dict, Tuple import click from samcli.cli.context import Context @@ -130,13 +130,13 @@ default=None, multiple=True, # Can pass in multiple build images required=False, - help="Docker image URIs for building functions." - "You can specify for each individual function with " - "(--build-image FunctionLogicalID=public.ecr.aws/sam/build-nodejs14.x:latest)." + help="Container image URIs for building functions. " "You can specify for all functions with just the image URI " - "(--build-image public.ecr.aws/sam/build-nodejs14.x:latest)." - "A combination of the two can be used. If a function does not have build image specified or" - "a image uri for all functions, the default SAM CLI build images will be used.", + "(--build-image public.ecr.aws/sam/build-nodejs14.x:latest). " + "You can specify for each individual function with " + "(--build-image FunctionLogicalID=public.ecr.aws/sam/build-nodejs14.x:latest). " + "A combination of the two can be used. If a function does not have build image specified or " + "an image URI for all functions, the default SAM CLI build images will be used.", ) @click.option( "--parallel", @@ -416,19 +416,14 @@ def _process_env_var(container_env_var: Optional[Tuple[str]]) -> Dict: for env_var in container_env_var: location_key = "Parameters" - if "=" not in env_var: - LOG.error("Invalid command line --container-env-var input %s, skipped", env_var) - continue + env_var_name, value = _parse_key_value_pair(env_var) - key, value = env_var.split("=", 1) - env_var_name = key - - if not value.strip(): + if not env_var_name or not value: LOG.error("Invalid command line --container-env-var input %s, skipped", env_var) continue - if "." in key: - location_key, env_var_name = key.split(".", 1) + if "." in env_var_name: + location_key, env_var_name = env_var_name.split(".", 1) if not location_key.strip() or not env_var_name.strip(): LOG.error("Invalid command line --container-env-var input %s, skipped", env_var) continue @@ -455,17 +450,36 @@ def _process_image_options(image_args: Optional[Tuple[str]]) -> Dict: Function as key and the corresponding image URI as value. Global default image URI is contained in the None key. """ - build_images: Dict[Union[str, None], str] = dict() + build_images: Dict[Optional[str], str] = dict() if image_args: for build_image_string in image_args: - function_name: Union[str, None] = None - if "=" in build_image_string: - parts = build_image_string.split("=", 1) - function_name = parts[0] - image_uri = parts[1] - else: - image_uri = build_image_string - + function_name, image_uri = _parse_key_value_pair(build_image_string) + if not image_uri: + LOG.error("Invalid command line --build-image input %s, skipped.", build_image_string) build_images[function_name] = image_uri return build_images + + +def _parse_key_value_pair(arg: str) -> Tuple[Optional[str], str]: + """ + Parameters + ---------- + arg : str + Arg in the format of "Value" or "Key=Value" + Returns + ------- + key : Optional[str] + If key is not specified, None will be the key. + value : str + """ + key: Optional[str] + value: str + if "=" in arg: + parts = arg.split("=", 1) + key = parts[0].strip() + value = parts[1].strip() + else: + key = None + value = arg.strip() + return key, value From 00d540fb19a70e1bc534c8c4d9e8d91ac8b0aeec Mon Sep 17 00:00:00 2001 From: Wilton Wang Date: Fri, 19 Mar 2021 10:51:50 -0700 Subject: [PATCH 08/10] Updated tag Handling for Image Pulls --- samcli/local/docker/lambda_build_container.py | 3 ++- samcli/local/docker/manager.py | 4 +++- tests/unit/local/docker/test_lambda_build_container.py | 4 ++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/samcli/local/docker/lambda_build_container.py b/samcli/local/docker/lambda_build_container.py index f8316fc948..dab2a6f154 100644 --- a/samcli/local/docker/lambda_build_container.py +++ b/samcli/local/docker/lambda_build_container.py @@ -19,6 +19,7 @@ class LambdaBuildContainer(Container): """ _IMAGE_URI_PREFIX = "public.ecr.aws/sam/build" + _IMAGE_TAG = "latest" _BUILDERS_EXECUTABLE = "lambda-builders" def __init__( # pylint: disable=too-many-locals @@ -236,4 +237,4 @@ def _convert_to_container_dirs(host_paths_to_convert, host_to_container_path_map @staticmethod def _get_image(runtime): - return f"{LambdaBuildContainer._IMAGE_URI_PREFIX}-{runtime}" + return f"{LambdaBuildContainer._IMAGE_URI_PREFIX}-{runtime}:{LambdaBuildContainer._IMAGE_TAG}" diff --git a/samcli/local/docker/manager.py b/samcli/local/docker/manager.py index bde3401222..dc684b9863 100644 --- a/samcli/local/docker/manager.py +++ b/samcli/local/docker/manager.py @@ -126,7 +126,7 @@ def stop(self, container: Container) -> None: container.stop() container.delete() - def pull_image(self, image_name, tag="latest", stream=None): + def pull_image(self, image_name, tag=None, stream=None): """ Ask Docker to pull the container image with given name. @@ -142,6 +142,8 @@ def pull_image(self, image_name, tag="latest", stream=None): DockerImagePullFailedException If the Docker image was not available in the server """ + if tag is None: + tag = image_name.split(":")[1] if ":" in image_name else "latest" # use a global lock to get the image lock with self._lock: image_lock = self._lock_per_image.get(image_name) diff --git a/tests/unit/local/docker/test_lambda_build_container.py b/tests/unit/local/docker/test_lambda_build_container.py index 08b0c05b15..6b305311a7 100644 --- a/tests/unit/local/docker/test_lambda_build_container.py +++ b/tests/unit/local/docker/test_lambda_build_container.py @@ -159,8 +159,8 @@ def test_must_override_manifest_if_equal_to_source(self): class TestLambdaBuildContainer_get_image(TestCase): @parameterized.expand( [ - ("myruntime", "public.ecr.aws/sam/build-myruntime"), - ("nodejs10.x", "public.ecr.aws/sam/build-nodejs10.x"), + ("myruntime", "public.ecr.aws/sam/build-myruntime:latest"), + ("nodejs10.x", "public.ecr.aws/sam/build-nodejs10.x:latest"), ] ) def test_must_get_image_name(self, runtime, expected_image_name): From 662814854ab7661c65315e4a461d6636053feed7 Mon Sep 17 00:00:00 2001 From: Wilton Wang Date: Wed, 24 Mar 2021 11:25:11 -0700 Subject: [PATCH 09/10] Added Throwing Exception with Invalid Build Images --- samcli/commands/build/command.py | 3 ++- samcli/commands/build/exceptions.py | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/samcli/commands/build/command.py b/samcli/commands/build/command.py index 00e9d85dfa..e44afd20ca 100644 --- a/samcli/commands/build/command.py +++ b/samcli/commands/build/command.py @@ -4,6 +4,7 @@ import os import logging +from samcli.commands.build.exceptions import InvalidBuildImageException from typing import List, Optional, Dict, Tuple import click @@ -455,7 +456,7 @@ def _process_image_options(image_args: Optional[Tuple[str]]) -> Dict: for build_image_string in image_args: function_name, image_uri = _parse_key_value_pair(build_image_string) if not image_uri: - LOG.error("Invalid command line --build-image input %s, skipped.", build_image_string) + raise InvalidBuildImageException(f"Invalid command line --build-image input {build_image_string}.") build_images[function_name] = image_uri return build_images diff --git a/samcli/commands/build/exceptions.py b/samcli/commands/build/exceptions.py index c678c1ae40..af2dea943f 100644 --- a/samcli/commands/build/exceptions.py +++ b/samcli/commands/build/exceptions.py @@ -13,3 +13,9 @@ class MissingBuildMethodException(UserException): """ Exception to be thrown when a layer is tried to build without BuildMethod """ + + +class InvalidBuildImageException(UserException): + """ + Value provided to --build-image is invalid + """ From ad03f5b0183f2e8929a3e0abe70d95b518157400 Mon Sep 17 00:00:00 2001 From: Wilton Wang Date: Wed, 24 Mar 2021 11:57:41 -0700 Subject: [PATCH 10/10] Fixed PyLint Issue --- samcli/commands/build/command.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samcli/commands/build/command.py b/samcli/commands/build/command.py index e44afd20ca..4c2a7e2654 100644 --- a/samcli/commands/build/command.py +++ b/samcli/commands/build/command.py @@ -4,7 +4,6 @@ import os import logging -from samcli.commands.build.exceptions import InvalidBuildImageException from typing import List, Optional, Dict, Tuple import click @@ -20,6 +19,7 @@ from samcli.lib.telemetry.metric import track_command from samcli.cli.cli_config_file import configuration_option, TomlProvider from samcli.lib.utils.version_checker import check_newer_version +from samcli.commands.build.exceptions import InvalidBuildImageException LOG = logging.getLogger(__name__)