From 763b04aaa8ddc17a52e39df781121c15b49cf694 Mon Sep 17 00:00:00 2001 From: Lucas <12496191+lucashuy@users.noreply.github.com> Date: Tue, 1 Nov 2022 14:49:23 -0700 Subject: [PATCH 1/8] Added --skip-prepare-iac option --- .../custom_options/hook_package_id_option.py | 15 +++++++-- samcli/commands/_utils/options.py | 31 +++++++++++++++++++ samcli/commands/build/command.py | 3 ++ tests/unit/commands/_utils/test_options.py | 21 +++++++++++++ 4 files changed, 68 insertions(+), 2 deletions(-) diff --git a/samcli/commands/_utils/custom_options/hook_package_id_option.py b/samcli/commands/_utils/custom_options/hook_package_id_option.py index b17e1ab5fe..51b8a25ac2 100644 --- a/samcli/commands/_utils/custom_options/hook_package_id_option.py +++ b/samcli/commands/_utils/custom_options/hook_package_id_option.py @@ -74,15 +74,26 @@ def handle_parse_result(self, ctx, opts, args): LOG.debug("beta-feature flag is disabled and prepare hook is not run") return super().handle_parse_result(ctx, opts, args) + iac_project_path = os.getcwd() + output_dir_path = os.path.join(iac_project_path, ".aws-sam-iacs", "iacs_metadata") + + # check if user provided --skip-prepare-iac + skip_prepare_hook = opts.get("skip_prepare_iac") + metadata_file_path = os.path.join(output_dir_path, "template.json") + # call prepare hook built_template_path = DEFAULT_BUILT_TEMPLATE_PATH if not self._force_prepare and os.path.exists(built_template_path): LOG.info("Skip Running Prepare hook. The current application is already prepared.") + elif skip_prepare_hook: + if os.path.exists(metadata_file_path): + LOG.info("Skipping prepare hook, --skip-prepare-iac was provided and metadata file already exists.") + opts["template_file"] = metadata_file_path + else: + raise click.UsageError("No metadata file found, rerun without the --skip-prepare-iac flag.") else: LOG.info("Running Prepare Hook to prepare the current application") - iac_project_path = os.getcwd() - output_dir_path = os.path.join(iac_project_path, ".aws-sam-iacs", "iacs_metadata") if not os.path.exists(output_dir_path): os.makedirs(output_dir_path, exist_ok=True) debug = opts.get("debug", False) diff --git a/samcli/commands/_utils/options.py b/samcli/commands/_utils/options.py index 69432bd4d9..f5873b1741 100644 --- a/samcli/commands/_utils/options.py +++ b/samcli/commands/_utils/options.py @@ -200,6 +200,23 @@ def resolve_s3_callback(ctx, param, provided_value, artifact, exc_set, exc_not_s return provided_value +def skip_prepare_iac_callback(ctx: click.core.Context, param: click.Option, provided_value: bool): + """ + Callback for --skip-prepare-iac to check if --hook-package-id is also specified + + Parameters + ---------- + ctx: click.core.Context + Click context + param: click.Option + Parameter properties + provided_value: bool + True if option was provided + """ + if provided_value and not ctx.params.get("hook_package_id", None): + raise click.BadOptionUsage(option_name=param.name, ctx=ctx, message="Missing option --hook-package-id") + + def template_common_option(f): """ Common ClI option for template @@ -666,6 +683,20 @@ def hook_package_id_click_option(force_prepare=True, invalid_coexist_options=Non ) +def skip_prepare_iac_option(): + """ + Click option to skip the hook preparation stage + """ + return click.option( + "--skip-prepare-iac", + is_flag=True, + required=False, + callback=skip_prepare_iac_callback, + help="Skips the preparation stage for the hook package if the metadata file has already been generated. " + "This option should be used together with --hook-package-id.", + ) + + @parameterized_option def resolve_s3_option(f, guided=False): return resolve_s3_click_option(guided)(f) diff --git a/samcli/commands/build/command.py b/samcli/commands/build/command.py index fa4866c0fb..ad580926f1 100644 --- a/samcli/commands/build/command.py +++ b/samcli/commands/build/command.py @@ -10,6 +10,7 @@ from samcli.cli.context import Context from samcli.commands._utils.experimental import experimental, ExperimentalFlag, is_experimental_enabled from samcli.commands._utils.options import ( + skip_prepare_iac_option, template_option_without_build, docker_common_options, parameter_override_option, @@ -84,6 +85,7 @@ force_prepare=True, invalid_coexist_options=["t", "template-file", "template", "parameter-overrides"], ) +@skip_prepare_iac_option() @configuration_option(provider=TomlProvider(section="parameters")) @use_container_build_option @click.option( @@ -172,6 +174,7 @@ def cli( config_file: str, config_env: str, hook_package_id: Optional[str], + skip_prepare_iac: bool, ) -> None: """ `sam build` command entry point diff --git a/tests/unit/commands/_utils/test_options.py b/tests/unit/commands/_utils/test_options.py index 7467608365..3e9ded0f73 100644 --- a/tests/unit/commands/_utils/test_options.py +++ b/tests/unit/commands/_utils/test_options.py @@ -20,6 +20,7 @@ resolve_s3_callback, image_repositories_callback, _space_separated_list_func_type, + skip_prepare_iac_callback, ) from samcli.commands._utils.parameterized_option import parameterized_option from samcli.commands.package.exceptions import PackageResolveS3AndS3SetError, PackageResolveS3AndS3NotSetError @@ -494,3 +495,23 @@ def test_option_dec_with_value(self): def test_option_dec_without_value(self): self.assertEqual(TestParameterizedOption.some_function_without_value(), 4) + +class TestSkipPrepareIacOption(TestCase): + def test_skip_with_hook_package(self): + ctx_mock = Mock() + ctx_mock.params = {"hook_package_id": "test"} + + skip_prepare_iac_callback(ctx_mock, Mock(), True) + + def test_skip_without_hook_package(self): + ctx_mock = Mock() + ctx_mock.command = Mock() + ctx_mock.params = {} + + param_mock = Mock() + param_mock.name = "test" + + with self.assertRaises(click.BadOptionUsage) as ex: + skip_prepare_iac_callback(ctx_mock, param_mock, True) + + self.assertEqual(str(ex.exception), "Missing option --hook-package-id") \ No newline at end of file From cd0783fd0f97013554b4d31104f1a376f787d87b Mon Sep 17 00:00:00 2001 From: Lucas <12496191+lucashuy@users.noreply.github.com> Date: Wed, 2 Nov 2022 14:36:57 -0700 Subject: [PATCH 2/8] Updated and added more tests --- .../test_hook_package_id_option.py | 255 +++++++++--------- tests/unit/commands/_utils/test_options.py | 3 +- 2 files changed, 129 insertions(+), 129 deletions(-) diff --git a/tests/unit/commands/_utils/custom_options/test_hook_package_id_option.py b/tests/unit/commands/_utils/custom_options/test_hook_package_id_option.py index 94082c327e..57904c4b9a 100644 --- a/tests/unit/commands/_utils/custom_options/test_hook_package_id_option.py +++ b/tests/unit/commands/_utils/custom_options/test_hook_package_id_option.py @@ -13,6 +13,12 @@ def setUp(self): self.name = "hook-package-id" self.opt = "--hook-package-id" self.terraform = "terraform" + self.invalid_coexist_options = ["t", "template", "template-file", "parameters-override"] + self.metadata_path = "path/metadata.json" + self.cwd_path = "path/current" + + self.iac_hook_wrapper_instance_mock = MagicMock() + self.iac_hook_wrapper_instance_mock.prepare.return_value = self.metadata_path @patch("samcli.commands._utils.custom_options.hook_package_id_option.get_available_hook_packages_ids") @patch("samcli.lib.hook.hook_wrapper.IacHookWrapper._load_hook_package") @@ -44,15 +50,13 @@ def test_invalid_hook_package_id(self, load_hook_package_mock, get_available_hoo @patch("samcli.commands._utils.custom_options.hook_package_id_option.IacHookWrapper") def test_invalid_coexist_options(self, iac_hook_wrapper_mock): - - invalid_coexist_options = ["t", "template", "template-file", "parameters-override"] iac_hook_wrapper_instance_mock = MagicMock() iac_hook_wrapper_mock.return_value = iac_hook_wrapper_instance_mock hook_package_id_option = HookPackageIdOption( param_decls=(self.name, self.opt), force_prepare=False, - invalid_coexist_options=invalid_coexist_options, + invalid_coexist_options=self.invalid_coexist_options, ) ctx = MagicMock() opts = {"hook_package_id": self.terraform, "template_file": "any/path/template.yaml"} @@ -62,7 +66,7 @@ def test_invalid_coexist_options(self, iac_hook_wrapper_mock): self.assertEqual( e.exception.message, - f"Parameters hook-package-id, and {','.join(invalid_coexist_options)} can not be used together", + f"Parameters hook-package-id, and {','.join(self.invalid_coexist_options)} can not be used together", ) @patch("samcli.commands._utils.custom_options.hook_package_id_option.prompt_experimental") @@ -71,21 +75,15 @@ def test_invalid_coexist_options(self, iac_hook_wrapper_mock): def test_valid_hook_package_with_only_hook_id_option( self, iac_hook_wrapper_mock, getcwd_mock, prompt_experimental_mock ): - metadata_path = "path/metadata.json" - cwd_path = "path/current" - invalid_coexist_options = ["t", "template", "template-file", "parameters-override"] - - iac_hook_wrapper_instance_mock = MagicMock() - iac_hook_wrapper_instance_mock.prepare.return_value = metadata_path - iac_hook_wrapper_mock.return_value = iac_hook_wrapper_instance_mock + iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True - getcwd_mock.return_value = cwd_path + getcwd_mock.return_value = self.cwd_path hook_package_id_option = HookPackageIdOption( param_decls=(self.name, self.opt), force_prepare=True, - invalid_coexist_options=invalid_coexist_options, + invalid_coexist_options=self.invalid_coexist_options, ) ctx = MagicMock() opts = { @@ -93,31 +91,24 @@ def test_valid_hook_package_with_only_hook_id_option( } args = [] hook_package_id_option.handle_parse_result(ctx, opts, args) - iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( - os.path.join(cwd_path, ".aws-sam-iacs", "iacs_metadata"), cwd_path, False, None, None + self.iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( + os.path.join(self.cwd_path, ".aws-sam-iacs", "iacs_metadata"), self.cwd_path, False, None, None ) - self.assertEqual(opts.get("template_file"), metadata_path) + self.assertEqual(opts.get("template_file"), self.metadata_path) @patch("samcli.commands._utils.custom_options.hook_package_id_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.getcwd") @patch("samcli.commands._utils.custom_options.hook_package_id_option.IacHookWrapper") def test_valid_hook_package_with_other_options(self, iac_hook_wrapper_mock, getcwd_mock, prompt_experimental_mock): - - metadata_path = "path/metadata.json" - cwd_path = "path/current" - invalid_coexist_options = ["t", "template", "template-file", "parameters-override"] - - iac_hook_wrapper_instance_mock = MagicMock() - iac_hook_wrapper_instance_mock.prepare.return_value = metadata_path - iac_hook_wrapper_mock.return_value = iac_hook_wrapper_instance_mock + iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True - getcwd_mock.return_value = cwd_path + getcwd_mock.return_value = self.cwd_path hook_package_id_option = HookPackageIdOption( param_decls=(self.name, self.opt), force_prepare=True, - invalid_coexist_options=invalid_coexist_options, + invalid_coexist_options=self.invalid_coexist_options, ) ctx = MagicMock() opts = { @@ -128,14 +119,14 @@ def test_valid_hook_package_with_other_options(self, iac_hook_wrapper_mock, getc } args = [] hook_package_id_option.handle_parse_result(ctx, opts, args) - iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( - os.path.join(cwd_path, ".aws-sam-iacs", "iacs_metadata"), - cwd_path, + self.iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( + os.path.join(self.cwd_path, ".aws-sam-iacs", "iacs_metadata"), + self.cwd_path, True, "test", "us-east-1", ) - self.assertEqual(opts.get("template_file"), metadata_path) + self.assertEqual(opts.get("template_file"), self.metadata_path) @patch("samcli.commands._utils.custom_options.hook_package_id_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.getcwd") @@ -144,24 +135,17 @@ def test_valid_hook_package_with_other_options(self, iac_hook_wrapper_mock, getc def test_valid_hook_package_with_skipping_prepare_hook_and_built_path_exists( self, iac_hook_wrapper_mock, path_exists_mock, getcwd_mock, prompt_experimental_mock ): - - metadata_path = "path/metadata.json" - cwd_path = "path/current" - invalid_coexist_options = ["t", "template", "template-file", "parameters-override"] - - iac_hook_wrapper_instance_mock = MagicMock() - iac_hook_wrapper_instance_mock.prepare.return_value = metadata_path - iac_hook_wrapper_mock.return_value = iac_hook_wrapper_instance_mock + iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True - getcwd_mock.return_value = cwd_path + getcwd_mock.return_value = self.cwd_path path_exists_mock.return_value = True hook_package_id_option = HookPackageIdOption( param_decls=(self.name, self.opt), force_prepare=False, - invalid_coexist_options=invalid_coexist_options, + invalid_coexist_options=self.invalid_coexist_options, ) ctx = MagicMock() opts = { @@ -169,7 +153,7 @@ def test_valid_hook_package_with_skipping_prepare_hook_and_built_path_exists( } args = [] hook_package_id_option.handle_parse_result(ctx, opts, args) - iac_hook_wrapper_instance_mock.prepare.assert_not_called() + self.iac_hook_wrapper_instance_mock.prepare.assert_not_called() self.assertEqual(opts.get("template_file"), None) @patch("samcli.commands._utils.custom_options.hook_package_id_option.prompt_experimental") @@ -179,22 +163,15 @@ def test_valid_hook_package_with_skipping_prepare_hook_and_built_path_exists( def test_valid_hook_package_with_disable_terraform_beta_feature( self, iac_hook_wrapper_mock, path_exists_mock, getcwd_mock, prompt_experimental_mock ): - - metadata_path = "path/metadata.json" - cwd_path = "path/current" - invalid_coexist_options = ["t", "template", "template-file", "parameters-override"] - - iac_hook_wrapper_instance_mock = MagicMock() - iac_hook_wrapper_instance_mock.prepare.return_value = metadata_path - iac_hook_wrapper_mock.return_value = iac_hook_wrapper_instance_mock + iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = False - getcwd_mock.return_value = cwd_path + getcwd_mock.return_value = self.cwd_path hook_package_id_option = HookPackageIdOption( param_decls=(self.name, self.opt), force_prepare=True, - invalid_coexist_options=invalid_coexist_options, + invalid_coexist_options=self.invalid_coexist_options, ) ctx = MagicMock() opts = { @@ -202,7 +179,7 @@ def test_valid_hook_package_with_disable_terraform_beta_feature( } args = [] hook_package_id_option.handle_parse_result(ctx, opts, args) - iac_hook_wrapper_instance_mock.prepare.assert_not_called() + self.iac_hook_wrapper_instance_mock.prepare.assert_not_called() self.assertEqual(opts.get("template_file"), None) @patch("samcli.commands._utils.custom_options.hook_package_id_option.prompt_experimental") @@ -212,22 +189,15 @@ def test_valid_hook_package_with_disable_terraform_beta_feature( def test_valid_hook_package_with_no_beta_feature_option( self, iac_hook_wrapper_mock, path_exists_mock, getcwd_mock, prompt_experimental_mock ): - - metadata_path = "path/metadata.json" - cwd_path = "path/current" - invalid_coexist_options = ["t", "template", "template-file", "parameters-override"] - - iac_hook_wrapper_instance_mock = MagicMock() - iac_hook_wrapper_instance_mock.prepare.return_value = metadata_path - iac_hook_wrapper_mock.return_value = iac_hook_wrapper_instance_mock + iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = False - getcwd_mock.return_value = cwd_path + getcwd_mock.return_value = self.cwd_path hook_package_id_option = HookPackageIdOption( param_decls=(self.name, self.opt), force_prepare=True, - invalid_coexist_options=invalid_coexist_options, + invalid_coexist_options=self.invalid_coexist_options, ) ctx = MagicMock() opts = { @@ -237,7 +207,7 @@ def test_valid_hook_package_with_no_beta_feature_option( args = [] hook_package_id_option.handle_parse_result(ctx, opts, args) prompt_experimental_mock.assert_not_called() - iac_hook_wrapper_instance_mock.prepare.assert_not_called() + self.iac_hook_wrapper_instance_mock.prepare.assert_not_called() self.assertEqual(opts.get("template_file"), None) @patch("samcli.commands._utils.custom_options.hook_package_id_option.prompt_experimental") @@ -247,22 +217,15 @@ def test_valid_hook_package_with_no_beta_feature_option( def test_valid_hook_package_with_beta_feature_option( self, iac_hook_wrapper_mock, path_exists_mock, getcwd_mock, prompt_experimental_mock ): - - metadata_path = "path/metadata.json" - cwd_path = "path/current" - invalid_coexist_options = ["t", "template", "template-file", "parameters-override"] - - iac_hook_wrapper_instance_mock = MagicMock() - iac_hook_wrapper_instance_mock.prepare.return_value = metadata_path - iac_hook_wrapper_mock.return_value = iac_hook_wrapper_instance_mock + iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = False - getcwd_mock.return_value = cwd_path + getcwd_mock.return_value = self.cwd_path hook_package_id_option = HookPackageIdOption( param_decls=(self.name, self.opt), force_prepare=True, - invalid_coexist_options=invalid_coexist_options, + invalid_coexist_options=self.invalid_coexist_options, ) ctx = MagicMock() opts = { @@ -272,14 +235,14 @@ def test_valid_hook_package_with_beta_feature_option( args = [] hook_package_id_option.handle_parse_result(ctx, opts, args) prompt_experimental_mock.assert_not_called() - iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( - os.path.join(cwd_path, ".aws-sam-iacs", "iacs_metadata"), - cwd_path, + self.iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( + os.path.join(self.cwd_path, ".aws-sam-iacs", "iacs_metadata"), + self.cwd_path, False, None, None, ) - self.assertEqual(opts.get("template_file"), metadata_path) + self.assertEqual(opts.get("template_file"), self.metadata_path) @patch("samcli.commands._utils.custom_options.hook_package_id_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.getcwd") @@ -288,24 +251,17 @@ def test_valid_hook_package_with_beta_feature_option( def test_valid_hook_package_with_skipping_prepare_hook_and_built_path_doesnot_exist( self, iac_hook_wrapper_mock, path_exists_mock, getcwd_mock, prompt_experimental_mock ): - - metadata_path = "path/metadata.json" - cwd_path = "path/current" - invalid_coexist_options = ["t", "template", "template-file", "parameters-override"] - - iac_hook_wrapper_instance_mock = MagicMock() - iac_hook_wrapper_instance_mock.prepare.return_value = metadata_path - iac_hook_wrapper_mock.return_value = iac_hook_wrapper_instance_mock + iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True - getcwd_mock.return_value = cwd_path + getcwd_mock.return_value = self.cwd_path path_exists_mock.return_value = False hook_package_id_option = HookPackageIdOption( param_decls=(self.name, self.opt), force_prepare=False, - invalid_coexist_options=invalid_coexist_options, + invalid_coexist_options=self.invalid_coexist_options, ) ctx = MagicMock() opts = { @@ -313,10 +269,10 @@ def test_valid_hook_package_with_skipping_prepare_hook_and_built_path_doesnot_ex } args = [] hook_package_id_option.handle_parse_result(ctx, opts, args) - iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( - os.path.join(cwd_path, ".aws-sam-iacs", "iacs_metadata"), cwd_path, False, None, None + self.iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( + os.path.join(self.cwd_path, ".aws-sam-iacs", "iacs_metadata"), self.cwd_path, False, None, None ) - self.assertEqual(opts.get("template_file"), metadata_path) + self.assertEqual(opts.get("template_file"), self.metadata_path) @patch("samcli.commands._utils.custom_options.hook_package_id_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.getcwd") @@ -325,24 +281,17 @@ def test_valid_hook_package_with_skipping_prepare_hook_and_built_path_doesnot_ex def test_valid_hook_package_with_use_container_and_build_image( self, iac_hook_wrapper_mock, path_exists_mock, getcwd_mock, prompt_experimental_mock ): - - metadata_path = "path/metadata.json" - cwd_path = "path/current" - invalid_coexist_options = ["t", "template", "template-file", "parameters-override"] - - iac_hook_wrapper_instance_mock = MagicMock() - iac_hook_wrapper_instance_mock.prepare.return_value = metadata_path - iac_hook_wrapper_mock.return_value = iac_hook_wrapper_instance_mock + iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True - getcwd_mock.return_value = cwd_path + getcwd_mock.return_value = self.cwd_path path_exists_mock.return_value = False hook_package_id_option = HookPackageIdOption( param_decls=(self.name, self.opt), force_prepare=False, - invalid_coexist_options=invalid_coexist_options, + invalid_coexist_options=self.invalid_coexist_options, ) ctx = MagicMock() ctx.command.name = "build" @@ -353,10 +302,10 @@ def test_valid_hook_package_with_use_container_and_build_image( } args = [] hook_package_id_option.handle_parse_result(ctx, opts, args) - iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( - os.path.join(cwd_path, ".aws-sam-iacs", "iacs_metadata"), cwd_path, False, None, None + self.iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( + os.path.join(self.cwd_path, ".aws-sam-iacs", "iacs_metadata"), self.cwd_path, False, None, None ) - self.assertEqual(opts.get("template_file"), metadata_path) + self.assertEqual(opts.get("template_file"), self.metadata_path) @patch("samcli.commands._utils.custom_options.hook_package_id_option.prompt_experimental") @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.getcwd") @@ -365,24 +314,17 @@ def test_valid_hook_package_with_use_container_and_build_image( def test_invalid_hook_package_with_use_container_and_no_build_image( self, iac_hook_wrapper_mock, path_exists_mock, getcwd_mock, prompt_experimental_mock ): - - metadata_path = "path/metadata.json" - cwd_path = "path/current" - invalid_coexist_options = ["t", "template", "template-file", "parameters-override"] - - iac_hook_wrapper_instance_mock = MagicMock() - iac_hook_wrapper_instance_mock.prepare.return_value = metadata_path - iac_hook_wrapper_mock.return_value = iac_hook_wrapper_instance_mock + iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True - getcwd_mock.return_value = cwd_path + getcwd_mock.return_value = self.cwd_path path_exists_mock.return_value = False hook_package_id_option = HookPackageIdOption( param_decls=(self.name, self.opt), force_prepare=False, - invalid_coexist_options=invalid_coexist_options, + invalid_coexist_options=self.invalid_coexist_options, ) ctx = MagicMock() ctx.command.name = "build" @@ -391,9 +333,9 @@ def test_invalid_hook_package_with_use_container_and_no_build_image( "use_container": True, } args = [] - with self.assertRaises( + with self.assertRaisesRegex( click.UsageError, - msg="Missing required parameter --build-image.", + "Missing required parameter --build-image.", ): hook_package_id_option.handle_parse_result(ctx, opts, args) @@ -404,24 +346,17 @@ def test_invalid_hook_package_with_use_container_and_no_build_image( def test_valid_hook_package_with_use_container_false_and_no_build_image( self, iac_hook_wrapper_mock, path_exists_mock, getcwd_mock, prompt_experimental_mock ): - - metadata_path = "path/metadata.json" - cwd_path = "path/current" - invalid_coexist_options = ["t", "template", "template-file", "parameters-override"] - - iac_hook_wrapper_instance_mock = MagicMock() - iac_hook_wrapper_instance_mock.prepare.return_value = metadata_path - iac_hook_wrapper_mock.return_value = iac_hook_wrapper_instance_mock + iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock prompt_experimental_mock.return_value = True - getcwd_mock.return_value = cwd_path + getcwd_mock.return_value = self.cwd_path path_exists_mock.return_value = False hook_package_id_option = HookPackageIdOption( param_decls=(self.name, self.opt), force_prepare=False, - invalid_coexist_options=invalid_coexist_options, + invalid_coexist_options=self.invalid_coexist_options, ) ctx = MagicMock() ctx.command.name = "build" @@ -431,7 +366,71 @@ def test_valid_hook_package_with_use_container_false_and_no_build_image( } args = [] hook_package_id_option.handle_parse_result(ctx, opts, args) - iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( - os.path.join(cwd_path, ".aws-sam-iacs", "iacs_metadata"), cwd_path, False, None, None + self.iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( + os.path.join(self.cwd_path, ".aws-sam-iacs", "iacs_metadata"), self.cwd_path, False, None, None + ) + self.assertEqual(opts.get("template_file"), self.metadata_path) + + @patch("samcli.commands._utils.custom_options.hook_package_id_option.prompt_experimental") + @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.getcwd") + @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.path.exists") + @patch("samcli.commands._utils.custom_options.hook_package_id_option.IacHookWrapper") + def test_invalid_hook_package_with_skip_prepare_iac_missing_metadata_file( + self, iac_hook_wrapper_mock, path_exists_mock, getcwd_mock, prompt_experimental_mock + ): + iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock + prompt_experimental_mock.return_value = True + + getcwd_mock.return_value = self.cwd_path + + path_exists_mock.return_value = False + + hook_package_id_option = HookPackageIdOption( + param_decls=(self.name, self.opt), + force_prepare=True, + invalid_coexist_options=self.invalid_coexist_options, + ) + ctx = MagicMock() + ctx.command.name = "build" + opts = { + "hook_package_id": self.terraform, + "skip_prepare_iac": True, + } + args = [] + with self.assertRaisesRegex( + click.UsageError, "No metadata file found, rerun without the --skip-prepare-iac flag." + ): + hook_package_id_option.handle_parse_result(ctx, opts, args) + + @patch("samcli.commands._utils.custom_options.hook_package_id_option.prompt_experimental") + @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.getcwd") + @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.path.exists") + @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.path.join") + @patch("samcli.commands._utils.custom_options.hook_package_id_option.IacHookWrapper") + def test_valid_hook_package_with_skip_prepare_iac_valid_metadata_file( + self, iac_hook_wrapper_mock, path_join_mock, path_exists_mock, getcwd_mock, prompt_experimental_mock + ): + iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock + prompt_experimental_mock.return_value = True + + getcwd_mock.return_value = self.cwd_path + + path_exists_mock.return_value = True + path_join_mock.return_value = self.metadata_path + + hook_package_id_option = HookPackageIdOption( + param_decls=(self.name, self.opt), + force_prepare=True, + invalid_coexist_options=self.invalid_coexist_options, ) - self.assertEqual(opts.get("template_file"), metadata_path) + ctx = MagicMock() + ctx.command.name = "build" + opts = { + "hook_package_id": self.terraform, + "skip_prepare_iac": True, + } + args = [] + hook_package_id_option.handle_parse_result(ctx, opts, args) + + self.iac_hook_wrapper_instance_mock.prepare.assert_not_called() + self.assertEqual(opts.get("template_file"), self.metadata_path) diff --git a/tests/unit/commands/_utils/test_options.py b/tests/unit/commands/_utils/test_options.py index 3e9ded0f73..180cea085b 100644 --- a/tests/unit/commands/_utils/test_options.py +++ b/tests/unit/commands/_utils/test_options.py @@ -496,6 +496,7 @@ def test_option_dec_with_value(self): def test_option_dec_without_value(self): self.assertEqual(TestParameterizedOption.some_function_without_value(), 4) + class TestSkipPrepareIacOption(TestCase): def test_skip_with_hook_package(self): ctx_mock = Mock() @@ -514,4 +515,4 @@ def test_skip_without_hook_package(self): with self.assertRaises(click.BadOptionUsage) as ex: skip_prepare_iac_callback(ctx_mock, param_mock, True) - self.assertEqual(str(ex.exception), "Missing option --hook-package-id") \ No newline at end of file + self.assertEqual(str(ex.exception), "Missing option --hook-package-id") From 1528b12a9e40945ef8d27a4ec51dad090f1e1ee2 Mon Sep 17 00:00:00 2001 From: Lucas <12496191+lucashuy@users.noreply.github.com> Date: Wed, 2 Nov 2022 15:12:06 -0700 Subject: [PATCH 3/8] Added flag to invoke and start-lambda --- samcli/commands/_utils/options.py | 6 +- samcli/commands/build/command.py | 2 +- samcli/commands/local/invoke/cli.py | 4 +- samcli/commands/local/start_lambda/cli.py | 4 +- tests/unit/commands/local/invoke/test_cli.py | 213 +++---------------- 5 files changed, 37 insertions(+), 192 deletions(-) diff --git a/samcli/commands/_utils/options.py b/samcli/commands/_utils/options.py index f5873b1741..6bebe61a0c 100644 --- a/samcli/commands/_utils/options.py +++ b/samcli/commands/_utils/options.py @@ -683,7 +683,7 @@ def hook_package_id_click_option(force_prepare=True, invalid_coexist_options=Non ) -def skip_prepare_iac_option(): +def skip_prepare_iac_click_option(): """ Click option to skip the hook preparation stage """ @@ -697,6 +697,10 @@ def skip_prepare_iac_option(): ) +def skip_prepare_iac_option(f): + return skip_prepare_iac_click_option()(f) + + @parameterized_option def resolve_s3_option(f, guided=False): return resolve_s3_click_option(guided)(f) diff --git a/samcli/commands/build/command.py b/samcli/commands/build/command.py index ad580926f1..d3ccad3d10 100644 --- a/samcli/commands/build/command.py +++ b/samcli/commands/build/command.py @@ -85,7 +85,7 @@ force_prepare=True, invalid_coexist_options=["t", "template-file", "template", "parameter-overrides"], ) -@skip_prepare_iac_option() +@skip_prepare_iac_option @configuration_option(provider=TomlProvider(section="parameters")) @use_container_build_option @click.option( diff --git a/samcli/commands/local/invoke/cli.py b/samcli/commands/local/invoke/cli.py index 45c47451a9..7c2cdab7fc 100644 --- a/samcli/commands/local/invoke/cli.py +++ b/samcli/commands/local/invoke/cli.py @@ -7,7 +7,7 @@ from samcli.cli.main import pass_context, common_options as cli_framework_options, aws_creds_options, print_cmdline_args from samcli.commands._utils.experimental import experimental, is_experimental_enabled, ExperimentalFlag -from samcli.commands._utils.options import hook_package_id_click_option +from samcli.commands._utils.options import hook_package_id_click_option, skip_prepare_iac_option from samcli.commands.local.cli_common.options import invoke_common_options, local_common_options from samcli.commands.local.lib.exceptions import InvalidIntermediateImageError from samcli.lib.telemetry.metric import track_command @@ -39,6 +39,7 @@ @hook_package_id_click_option( force_prepare=False, invalid_coexist_options=["t", "template-file", "template", "parameter-overrides"] ) +@skip_prepare_iac_option @configuration_option(provider=TomlProvider(section="parameters")) @click.option( "--event", @@ -83,6 +84,7 @@ def cli( container_host_interface, invoke_image, hook_package_id, + skip_prepare_iac, ): """ `sam local invoke` command entry point diff --git a/samcli/commands/local/start_lambda/cli.py b/samcli/commands/local/start_lambda/cli.py index 903f057066..48d891a97d 100644 --- a/samcli/commands/local/start_lambda/cli.py +++ b/samcli/commands/local/start_lambda/cli.py @@ -7,7 +7,7 @@ from samcli.cli.main import pass_context, common_options as cli_framework_options, aws_creds_options, print_cmdline_args from samcli.commands._utils.experimental import experimental, is_experimental_enabled, ExperimentalFlag -from samcli.commands._utils.options import hook_package_id_click_option +from samcli.commands._utils.options import hook_package_id_click_option, skip_prepare_iac_option from samcli.commands.local.cli_common.options import ( invoke_common_options, service_common_options, @@ -63,6 +63,7 @@ @hook_package_id_click_option( force_prepare=False, invalid_coexist_options=["t", "template-file", "template", "parameter-overrides"] ) +@skip_prepare_iac_option @configuration_option(provider=TomlProvider(section="parameters")) @service_common_options(3001) @invoke_common_options @@ -103,6 +104,7 @@ def cli( container_host_interface, invoke_image, hook_package_id, + skip_prepare_iac, ): """ `sam local start-lambda` command entry point diff --git a/tests/unit/commands/local/invoke/test_cli.py b/tests/unit/commands/local/invoke/test_cli.py index b5befb1c4d..4943d5dcde 100644 --- a/tests/unit/commands/local/invoke/test_cli.py +++ b/tests/unit/commands/local/invoke/test_cli.py @@ -46,22 +46,13 @@ def setUp(self): self.invoke_image = ("amazon/aws-sam-cli-emulation-image-python3.6",) self.hook_package_id = None - @patch("samcli.commands.local.cli_common.invoke_context.InvokeContext") - @patch("samcli.commands.local.invoke.cli._get_event") - def test_cli_must_setup_context_and_invoke(self, get_event_mock, InvokeContextMock): - event_data = "data" - get_event_mock.return_value = event_data - - ctx_mock = Mock() - ctx_mock.region = self.region_name - ctx_mock.profile = self.profile - - # Mock the __enter__ method to return a object inside a context manager - context_mock = Mock() - InvokeContextMock.return_value.__enter__.return_value = context_mock + self.ctx_mock = Mock() + self.ctx_mock.region = self.region_name + self.ctx_mock.profile = self.profile + def call_cli(self): invoke_cli( - ctx=ctx_mock, + ctx=self.ctx_mock, function_identifier=self.function_id, template=self.template, event=self.eventfile, @@ -85,6 +76,18 @@ def test_cli_must_setup_context_and_invoke(self, get_event_mock, InvokeContextMo hook_package_id=self.hook_package_id, ) + @patch("samcli.commands.local.cli_common.invoke_context.InvokeContext") + @patch("samcli.commands.local.invoke.cli._get_event") + def test_cli_must_setup_context_and_invoke(self, get_event_mock, InvokeContextMock): + event_data = "data" + get_event_mock.return_value = event_data + + # Mock the __enter__ method to return a object inside a context manager + context_mock = Mock() + InvokeContextMock.return_value.__enter__.return_value = context_mock + + self.call_cli() + InvokeContextMock.assert_called_with( template_file=self.template, function_identifier=self.function_id, @@ -116,39 +119,13 @@ def test_cli_must_setup_context_and_invoke(self, get_event_mock, InvokeContextMo @patch("samcli.commands.local.cli_common.invoke_context.InvokeContext") @patch("samcli.commands.local.invoke.cli._get_event") def test_cli_must_invoke_with_no_event(self, get_event_mock, InvokeContextMock): - self.event = None - - ctx_mock = Mock() - ctx_mock.region = self.region_name - ctx_mock.profile = self.profile + self.eventfile = None # Mock the __enter__ method to return a object inside a context manager context_mock = Mock() InvokeContextMock.return_value.__enter__.return_value = context_mock - invoke_cli( - ctx=ctx_mock, - function_identifier=self.function_id, - template=self.template, - event=self.event, - no_event=self.no_event, - env_vars=self.env_vars, - debug_port=self.debug_ports, - debug_args=self.debug_args, - debugger_path=self.debugger_path, - container_env_vars=self.container_env_vars, - docker_volume_basedir=self.docker_volume_basedir, - docker_network=self.docker_network, - log_file=self.log_file, - skip_pull_image=self.skip_pull_image, - parameter_overrides=self.parameter_overrides, - layer_cache_basedir=self.layer_cache_basedir, - force_image_build=self.force_image_build, - shutdown=self.shutdown, - container_host=self.container_host, - container_host_interface=self.container_host_interface, - invoke_image=self.invoke_image, - hook_package_id=self.hook_package_id, - ) + + self.call_cli() InvokeContextMock.assert_called_with( template_file=self.template, @@ -192,10 +169,6 @@ def test_must_raise_user_exception_on_function_not_found( event_data = "data" get_event_mock.return_value = event_data - ctx_mock = Mock() - ctx_mock.region = self.region_name - ctx_mock.profile = self.profile - # Mock the __enter__ method to return a object inside a context manager context_mock = Mock() InvokeContextMock.return_value.__enter__.return_value = context_mock @@ -203,31 +176,7 @@ def test_must_raise_user_exception_on_function_not_found( context_mock.local_lambda_runner.invoke.side_effect = side_effect_exception with self.assertRaises(UserException) as ex_ctx: - - invoke_cli( - ctx=ctx_mock, - function_identifier=self.function_id, - template=self.template, - event=self.eventfile, - no_event=self.no_event, - env_vars=self.env_vars, - debug_port=self.debug_ports, - debug_args=self.debug_args, - debugger_path=self.debugger_path, - container_env_vars=self.container_env_vars, - docker_volume_basedir=self.docker_volume_basedir, - docker_network=self.docker_network, - log_file=self.log_file, - skip_pull_image=self.skip_pull_image, - parameter_overrides=self.parameter_overrides, - layer_cache_basedir=self.layer_cache_basedir, - force_image_build=self.force_image_build, - shutdown=self.shutdown, - container_host=self.container_host, - container_host_interface=self.container_host_interface, - invoke_image=self.invoke_image, - hook_package_id=self.hook_package_id, - ) + self.call_cli() msg = str(ex_ctx.exception) self.assertEqual(msg, expected_exectpion_message) @@ -248,10 +197,6 @@ def test_must_raise_user_exception_on_function_local_invoke_image_not_found_for_ event_data = "data" get_event_mock.return_value = event_data - ctx_mock = Mock() - ctx_mock.region = self.region_name - ctx_mock.profile = self.profile - # Mock the __enter__ method to return a object inside a context manager context_mock = Mock() InvokeContextMock.return_value.__enter__.return_value = context_mock @@ -259,31 +204,7 @@ def test_must_raise_user_exception_on_function_local_invoke_image_not_found_for_ context_mock.local_lambda_runner.invoke.side_effect = side_effect_exception with self.assertRaises(UserException) as ex_ctx: - - invoke_cli( - ctx=ctx_mock, - function_identifier=self.function_id, - template=self.template, - event=self.eventfile, - no_event=self.no_event, - env_vars=self.env_vars, - debug_port=self.debug_ports, - debug_args=self.debug_args, - debugger_path=self.debugger_path, - container_env_vars=self.container_env_vars, - docker_volume_basedir=self.docker_volume_basedir, - docker_network=self.docker_network, - log_file=self.log_file, - skip_pull_image=self.skip_pull_image, - parameter_overrides=self.parameter_overrides, - layer_cache_basedir=self.layer_cache_basedir, - force_image_build=self.force_image_build, - shutdown=self.shutdown, - container_host=self.container_host, - container_host_interface=self.container_host_interface, - invoke_image=self.invoke_image, - hook_package_id=self.hook_package_id, - ) + self.call_cli() msg = str(ex_ctx.exception) self.assertEqual(msg, expected_exectpion_message) @@ -306,38 +227,10 @@ def test_must_raise_user_exception_on_invalid_sam_template( event_data = "data" get_event_mock.return_value = event_data - ctx_mock = Mock() - ctx_mock.region = self.region_name - ctx_mock.profile = self.profile - InvokeContextMock.side_effect = exeception_to_raise with self.assertRaises(UserException) as ex_ctx: - - invoke_cli( - ctx=ctx_mock, - function_identifier=self.function_id, - template=self.template, - event=self.eventfile, - no_event=self.no_event, - env_vars=self.env_vars, - debug_port=self.debug_ports, - debug_args=self.debug_args, - debugger_path=self.debugger_path, - container_env_vars=self.container_env_vars, - docker_volume_basedir=self.docker_volume_basedir, - docker_network=self.docker_network, - log_file=self.log_file, - skip_pull_image=self.skip_pull_image, - parameter_overrides=self.parameter_overrides, - layer_cache_basedir=self.layer_cache_basedir, - force_image_build=self.force_image_build, - shutdown=self.shutdown, - container_host=self.container_host, - container_host_interface=self.container_host_interface, - invoke_image=self.invoke_image, - hook_package_id=self.hook_package_id, - ) + self.call_cli() msg = str(ex_ctx.exception) self.assertEqual(msg, execption_message) @@ -348,38 +241,10 @@ def test_must_raise_user_exception_on_invalid_env_vars(self, get_event_mock, Inv event_data = "data" get_event_mock.return_value = event_data - ctx_mock = Mock() - ctx_mock.region = self.region_name - ctx_mock.profile = self.profile - InvokeContextMock.side_effect = OverridesNotWellDefinedError("bad env vars") with self.assertRaises(UserException) as ex_ctx: - - invoke_cli( - ctx=ctx_mock, - function_identifier=self.function_id, - template=self.template, - event=self.eventfile, - no_event=self.no_event, - env_vars=self.env_vars, - debug_port=self.debug_ports, - debug_args=self.debug_args, - debugger_path=self.debugger_path, - container_env_vars=self.container_env_vars, - docker_volume_basedir=self.docker_volume_basedir, - docker_network=self.docker_network, - log_file=self.log_file, - skip_pull_image=self.skip_pull_image, - parameter_overrides=self.parameter_overrides, - layer_cache_basedir=self.layer_cache_basedir, - force_image_build=self.force_image_build, - shutdown=self.shutdown, - container_host=self.container_host, - container_host_interface=self.container_host_interface, - invoke_image=self.invoke_image, - hook_package_id=self.hook_package_id, - ) + self.call_cli() msg = str(ex_ctx.exception) self.assertEqual(msg, "bad env vars") @@ -400,10 +265,6 @@ def test_must_raise_user_exception_on_function_no_free_ports( event_data = "data" get_event_mock.return_value = event_data - ctx_mock = Mock() - ctx_mock.region = self.region_name - ctx_mock.profile = self.profile - # Mock the __enter__ method to return a object inside a context manager context_mock = Mock() InvokeContextMock.return_value.__enter__.return_value = context_mock @@ -411,31 +272,7 @@ def test_must_raise_user_exception_on_function_no_free_ports( context_mock.local_lambda_runner.invoke.side_effect = side_effect_exception with self.assertRaises(UserException) as ex_ctx: - - invoke_cli( - ctx=ctx_mock, - function_identifier=self.function_id, - template=self.template, - event=self.eventfile, - no_event=self.no_event, - env_vars=self.env_vars, - debug_port=self.debug_ports, - debug_args=self.debug_args, - debugger_path=self.debugger_path, - container_env_vars=self.container_env_vars, - docker_volume_basedir=self.docker_volume_basedir, - docker_network=self.docker_network, - log_file=self.log_file, - skip_pull_image=self.skip_pull_image, - parameter_overrides=self.parameter_overrides, - layer_cache_basedir=self.layer_cache_basedir, - force_image_build=self.force_image_build, - shutdown=self.shutdown, - container_host=self.container_host, - container_host_interface=self.container_host_interface, - invoke_image=self.invoke_image, - hook_package_id=self.hook_package_id, - ) + self.call_cli() msg = str(ex_ctx.exception) self.assertEqual(msg, expected_exectpion_message) From fbb5336132283b24df0e3b4828ded3ff1ff0507a Mon Sep 17 00:00:00 2001 From: Lucas <12496191+lucashuy@users.noreply.github.com> Date: Thu, 3 Nov 2022 16:26:48 -0700 Subject: [PATCH 4/8] Moved skip prepare to terraform hook --- .../custom_options/hook_package_id_option.py | 21 +-- samcli/commands/_utils/options.py | 14 +- samcli/commands/build/command.py | 6 +- samcli/commands/local/invoke/cli.py | 6 +- samcli/commands/local/start_lambda/cli.py | 6 +- .../terraform/hooks/prepare/hook.py | 124 +++++++++--------- samcli/lib/hook/hook_wrapper.py | 4 + .../test_hook_package_id_option.py | 76 +---------- tests/unit/commands/_utils/test_options.py | 8 +- .../terraform/hooks/prepare/test_hook.py | 33 ++++- tests/unit/lib/hook/test_hook_wrapper.py | 2 + 11 files changed, 137 insertions(+), 163 deletions(-) diff --git a/samcli/commands/_utils/custom_options/hook_package_id_option.py b/samcli/commands/_utils/custom_options/hook_package_id_option.py index 51b8a25ac2..3f3d5b79e0 100644 --- a/samcli/commands/_utils/custom_options/hook_package_id_option.py +++ b/samcli/commands/_utils/custom_options/hook_package_id_option.py @@ -74,33 +74,26 @@ def handle_parse_result(self, ctx, opts, args): LOG.debug("beta-feature flag is disabled and prepare hook is not run") return super().handle_parse_result(ctx, opts, args) - iac_project_path = os.getcwd() - output_dir_path = os.path.join(iac_project_path, ".aws-sam-iacs", "iacs_metadata") - - # check if user provided --skip-prepare-iac - skip_prepare_hook = opts.get("skip_prepare_iac") - metadata_file_path = os.path.join(output_dir_path, "template.json") - # call prepare hook built_template_path = DEFAULT_BUILT_TEMPLATE_PATH if not self._force_prepare and os.path.exists(built_template_path): LOG.info("Skip Running Prepare hook. The current application is already prepared.") - elif skip_prepare_hook: - if os.path.exists(metadata_file_path): - LOG.info("Skipping prepare hook, --skip-prepare-iac was provided and metadata file already exists.") - opts["template_file"] = metadata_file_path - else: - raise click.UsageError("No metadata file found, rerun without the --skip-prepare-iac flag.") else: LOG.info("Running Prepare Hook to prepare the current application") + iac_project_path = os.getcwd() + output_dir_path = os.path.join(iac_project_path, ".aws-sam-iacs", "iacs_metadata") + if not os.path.exists(output_dir_path): os.makedirs(output_dir_path, exist_ok=True) + debug = opts.get("debug", False) aws_profile = opts.get("profile") aws_region = opts.get("region") + skip_prepare_infra = opts.get("skip_prepare_infra", False) + metadata_file = iac_hook_wrapper.prepare( - output_dir_path, iac_project_path, debug, aws_profile, aws_region + output_dir_path, iac_project_path, debug, aws_profile, aws_region, skip_prepare_infra ) LOG.info("Prepare Hook is done, and metadata file generated at %s", metadata_file) diff --git a/samcli/commands/_utils/options.py b/samcli/commands/_utils/options.py index 6bebe61a0c..302c946fac 100644 --- a/samcli/commands/_utils/options.py +++ b/samcli/commands/_utils/options.py @@ -200,9 +200,9 @@ def resolve_s3_callback(ctx, param, provided_value, artifact, exc_set, exc_not_s return provided_value -def skip_prepare_iac_callback(ctx: click.core.Context, param: click.Option, provided_value: bool): +def skip_prepare_infra_callback(ctx: click.core.Context, param: click.Option, provided_value: bool): """ - Callback for --skip-prepare-iac to check if --hook-package-id is also specified + Callback for --skip-prepare-infra to check if --hook-package-id is also specified Parameters ---------- @@ -683,22 +683,22 @@ def hook_package_id_click_option(force_prepare=True, invalid_coexist_options=Non ) -def skip_prepare_iac_click_option(): +def skip_prepare_infra_click_option(): """ Click option to skip the hook preparation stage """ return click.option( - "--skip-prepare-iac", + "--skip-prepare-infra", is_flag=True, required=False, - callback=skip_prepare_iac_callback, + callback=skip_prepare_infra_callback, help="Skips the preparation stage for the hook package if the metadata file has already been generated. " "This option should be used together with --hook-package-id.", ) -def skip_prepare_iac_option(f): - return skip_prepare_iac_click_option()(f) +def skip_prepare_infra_option(f): + return skip_prepare_infra_click_option()(f) @parameterized_option diff --git a/samcli/commands/build/command.py b/samcli/commands/build/command.py index d3ccad3d10..0a849f84c0 100644 --- a/samcli/commands/build/command.py +++ b/samcli/commands/build/command.py @@ -10,7 +10,7 @@ from samcli.cli.context import Context from samcli.commands._utils.experimental import experimental, ExperimentalFlag, is_experimental_enabled from samcli.commands._utils.options import ( - skip_prepare_iac_option, + skip_prepare_infra_option, template_option_without_build, docker_common_options, parameter_override_option, @@ -85,8 +85,8 @@ force_prepare=True, invalid_coexist_options=["t", "template-file", "template", "parameter-overrides"], ) -@skip_prepare_iac_option @configuration_option(provider=TomlProvider(section="parameters")) +@skip_prepare_infra_option @use_container_build_option @click.option( "--container-env-var", @@ -174,7 +174,7 @@ def cli( config_file: str, config_env: str, hook_package_id: Optional[str], - skip_prepare_iac: bool, + skip_prepare_infra: bool, ) -> None: """ `sam build` command entry point diff --git a/samcli/commands/local/invoke/cli.py b/samcli/commands/local/invoke/cli.py index 7c2cdab7fc..509dee1fc7 100644 --- a/samcli/commands/local/invoke/cli.py +++ b/samcli/commands/local/invoke/cli.py @@ -7,7 +7,7 @@ from samcli.cli.main import pass_context, common_options as cli_framework_options, aws_creds_options, print_cmdline_args from samcli.commands._utils.experimental import experimental, is_experimental_enabled, ExperimentalFlag -from samcli.commands._utils.options import hook_package_id_click_option, skip_prepare_iac_option +from samcli.commands._utils.options import hook_package_id_click_option, skip_prepare_infra_option from samcli.commands.local.cli_common.options import invoke_common_options, local_common_options from samcli.commands.local.lib.exceptions import InvalidIntermediateImageError from samcli.lib.telemetry.metric import track_command @@ -39,8 +39,8 @@ @hook_package_id_click_option( force_prepare=False, invalid_coexist_options=["t", "template-file", "template", "parameter-overrides"] ) -@skip_prepare_iac_option @configuration_option(provider=TomlProvider(section="parameters")) +@skip_prepare_infra_option @click.option( "--event", "-e", @@ -84,7 +84,7 @@ def cli( container_host_interface, invoke_image, hook_package_id, - skip_prepare_iac, + skip_prepare_infra, ): """ `sam local invoke` command entry point diff --git a/samcli/commands/local/start_lambda/cli.py b/samcli/commands/local/start_lambda/cli.py index 48d891a97d..1a64e69cfe 100644 --- a/samcli/commands/local/start_lambda/cli.py +++ b/samcli/commands/local/start_lambda/cli.py @@ -7,7 +7,7 @@ from samcli.cli.main import pass_context, common_options as cli_framework_options, aws_creds_options, print_cmdline_args from samcli.commands._utils.experimental import experimental, is_experimental_enabled, ExperimentalFlag -from samcli.commands._utils.options import hook_package_id_click_option, skip_prepare_iac_option +from samcli.commands._utils.options import hook_package_id_click_option, skip_prepare_infra_option from samcli.commands.local.cli_common.options import ( invoke_common_options, service_common_options, @@ -63,8 +63,8 @@ @hook_package_id_click_option( force_prepare=False, invalid_coexist_options=["t", "template-file", "template", "parameter-overrides"] ) -@skip_prepare_iac_option @configuration_option(provider=TomlProvider(section="parameters")) +@skip_prepare_infra_option @service_common_options(3001) @invoke_common_options @experimental @@ -104,7 +104,7 @@ def cli( container_host_interface, invoke_image, hook_package_id, - skip_prepare_iac, + skip_prepare_infra, ): """ `sam local start-lambda` command entry point diff --git a/samcli/hook_packages/terraform/hooks/prepare/hook.py b/samcli/hook_packages/terraform/hooks/prepare/hook.py index 77bb95b40b..2240c17532 100644 --- a/samcli/hook_packages/terraform/hooks/prepare/hook.py +++ b/samcli/hook_packages/terraform/hooks/prepare/hook.py @@ -58,6 +58,7 @@ PropertyBuilder = Callable[[dict, TFResource], Any] PropertyBuilderMapping = Dict[str, PropertyBuilder] +TERRAFORM_METADATA_FILE = "template.json" TERRAFORM_BUILD_SCRIPT = "copy_terraform_built_artifacts.py" TF_BACKEND_OVERRIDE_FILENAME = "z_samcli_backend_override" @@ -109,68 +110,73 @@ def prepare(params: dict) -> dict: output_dir_path = os.path.normpath(os.path.join(terraform_application_dir, output_dir_path)) LOG.debug("The normalized OutputDirPath value is %s", output_dir_path) - try: - # initialize terraform application - LOG.info("Initializing Terraform application") - run(["terraform", "init", "-input=false"], check=True, capture_output=True, cwd=terraform_application_dir) - - # get json output of terraform plan - LOG.info("Creating terraform plan and getting JSON output") - - with osutils.tempfile_platform_independent() as temp_file: - run( - # input false to avoid SAM CLI to stuck in case if the Terraform project expects input, and customer - # does not provide it. - ["terraform", "plan", "-out", temp_file.name, "-input=false"], - check=True, - capture_output=True, - cwd=terraform_application_dir, - ) - result = run( - ["terraform", "show", "-json", temp_file.name], - check=True, - capture_output=True, - cwd=terraform_application_dir, - ) - tf_json = json.loads(result.stdout) - - # convert terraform to cloudformation - LOG.info("Generating metadata file") - cfn_dict = _translate_to_cfn(tf_json, output_dir_path, terraform_application_dir) - - if cfn_dict.get("Resources"): - _update_resources_paths(cfn_dict.get("Resources"), terraform_application_dir) # type: ignore + skip_prepare_infra = params.get("SkipPrepareInfra") + metadata_file_path = os.path.join(output_dir_path, TERRAFORM_METADATA_FILE) - # store in supplied output dir - if not os.path.exists(output_dir_path): - os.makedirs(output_dir_path, exist_ok=True) - metadataFilePath = os.path.join(output_dir_path, "template.json") - LOG.info("Finished generating metadata file. Storing in %s", metadataFilePath) - with open(metadataFilePath, "w+") as metadata_file: - json.dump(cfn_dict, metadata_file) - - return {"iac_applications": {"MainApplication": {"metadata_file": metadataFilePath}}} - - except CalledProcessError as e: - stderr_output = str(e.stderr) - - # stderr can take on bytes or just be a plain string depending on terminal - if isinstance(e.stderr, bytes): - stderr_output = e.stderr.decode("utf-8") + if skip_prepare_infra and os.path.exists(metadata_file_path): + LOG.info("Skipping preparation stage, the metadata file already exists at %s", metadata_file_path) + else: + try: + # initialize terraform application + LOG.info("Initializing Terraform application") + run(["terraform", "init", "-input=false"], check=True, capture_output=True, cwd=terraform_application_dir) + + # get json output of terraform plan + LOG.info("Creating terraform plan and getting JSON output") + + with osutils.tempfile_platform_independent() as temp_file: + run( + # input false to avoid SAM CLI to stuck in case if the Terraform project expects input, and customer + # does not provide it. + ["terraform", "plan", "-out", temp_file.name, "-input=false"], + check=True, + capture_output=True, + cwd=terraform_application_dir, + ) + result = run( + ["terraform", "show", "-json", temp_file.name], + check=True, + capture_output=True, + cwd=terraform_application_dir, + ) + tf_json = json.loads(result.stdout) + + # convert terraform to cloudformation + LOG.info("Generating metadata file") + cfn_dict = _translate_to_cfn(tf_json, output_dir_path, terraform_application_dir) + + if cfn_dict.get("Resources"): + _update_resources_paths(cfn_dict.get("Resources"), terraform_application_dir) # type: ignore + + # store in supplied output dir + if not os.path.exists(output_dir_path): + os.makedirs(output_dir_path, exist_ok=True) + + LOG.info("Finished generating metadata file. Storing in %s", metadata_file_path) + with open(metadata_file_path, "w+") as metadata_file: + json.dump(cfn_dict, metadata_file) + except CalledProcessError as e: + stderr_output = str(e.stderr) + + # stderr can take on bytes or just be a plain string depending on terminal + if isinstance(e.stderr, bytes): + stderr_output = e.stderr.decode("utf-8") + + # one of the subprocess.run calls resulted in non-zero exit code or some OS error + LOG.debug( + "Error running terraform command: \n" "cmd: %s \n" "stdout: %s \n" "stderr: %s \n", + e.cmd, + e.stdout, + stderr_output, + ) - # one of the subprocess.run calls resulted in non-zero exit code or some OS error - LOG.debug( - "Error running terraform command: \n" "cmd: %s \n" "stdout: %s \n" "stderr: %s \n", - e.cmd, - e.stdout, - stderr_output, - ) + raise PrepareHookException( + f"There was an error while preparing the Terraform application.\n{stderr_output}" + ) from e + except OSError as e: + raise PrepareHookException(f"OSError: {e}") from e - raise PrepareHookException( - f"There was an error while preparing the Terraform application.\n{stderr_output}" - ) from e - except OSError as e: - raise PrepareHookException(f"OSError: {e}") from e + return {"iac_applications": {"MainApplication": {"metadata_file": metadata_file_path}}} def _update_resources_paths(cfn_resources: Dict[str, Any], terraform_application_dir: str) -> None: diff --git a/samcli/lib/hook/hook_wrapper.py b/samcli/lib/hook/hook_wrapper.py index 69bc71fcd5..3563385707 100644 --- a/samcli/lib/hook/hook_wrapper.py +++ b/samcli/lib/hook/hook_wrapper.py @@ -51,6 +51,7 @@ def prepare( debug: bool = False, aws_profile: Optional[str] = None, aws_region: Optional[str] = None, + skip_prepare_infra: bool = False, ) -> str: """ Run the prepare hook to generate the IaC Metadata file. @@ -67,6 +68,8 @@ def prepare( AWS profile to use. Default is None (use default profile) aws_region: str AWS region to use. Default is None (use default region) + skip_prepare_infra: bool + Flag to skip skip prepare hook if we already have the metadata file. Default is False. Returns ------- @@ -78,6 +81,7 @@ def prepare( "IACProjectPath": iac_project_path if iac_project_path else str(Path.cwd()), "OutputDirPath": output_dir_path, "Debug": debug, + "SkipPrepareInfra": skip_prepare_infra, } if aws_profile: params["Profile"] = aws_profile diff --git a/tests/unit/commands/_utils/custom_options/test_hook_package_id_option.py b/tests/unit/commands/_utils/custom_options/test_hook_package_id_option.py index 57904c4b9a..832f7b32c7 100644 --- a/tests/unit/commands/_utils/custom_options/test_hook_package_id_option.py +++ b/tests/unit/commands/_utils/custom_options/test_hook_package_id_option.py @@ -92,7 +92,7 @@ def test_valid_hook_package_with_only_hook_id_option( args = [] hook_package_id_option.handle_parse_result(ctx, opts, args) self.iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( - os.path.join(self.cwd_path, ".aws-sam-iacs", "iacs_metadata"), self.cwd_path, False, None, None + os.path.join(self.cwd_path, ".aws-sam-iacs", "iacs_metadata"), self.cwd_path, False, None, None, False ) self.assertEqual(opts.get("template_file"), self.metadata_path) @@ -125,6 +125,7 @@ def test_valid_hook_package_with_other_options(self, iac_hook_wrapper_mock, getc True, "test", "us-east-1", + False, ) self.assertEqual(opts.get("template_file"), self.metadata_path) @@ -241,6 +242,7 @@ def test_valid_hook_package_with_beta_feature_option( False, None, None, + False, ) self.assertEqual(opts.get("template_file"), self.metadata_path) @@ -248,7 +250,7 @@ def test_valid_hook_package_with_beta_feature_option( @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.getcwd") @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.path.exists") @patch("samcli.commands._utils.custom_options.hook_package_id_option.IacHookWrapper") - def test_valid_hook_package_with_skipping_prepare_hook_and_built_path_doesnot_exist( + def test_valid_hook_package_with_skipping_prepare_hook_and_built_path_does_not_exist( self, iac_hook_wrapper_mock, path_exists_mock, getcwd_mock, prompt_experimental_mock ): iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock @@ -270,7 +272,7 @@ def test_valid_hook_package_with_skipping_prepare_hook_and_built_path_doesnot_ex args = [] hook_package_id_option.handle_parse_result(ctx, opts, args) self.iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( - os.path.join(self.cwd_path, ".aws-sam-iacs", "iacs_metadata"), self.cwd_path, False, None, None + os.path.join(self.cwd_path, ".aws-sam-iacs", "iacs_metadata"), self.cwd_path, False, None, None, False ) self.assertEqual(opts.get("template_file"), self.metadata_path) @@ -303,7 +305,7 @@ def test_valid_hook_package_with_use_container_and_build_image( args = [] hook_package_id_option.handle_parse_result(ctx, opts, args) self.iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( - os.path.join(self.cwd_path, ".aws-sam-iacs", "iacs_metadata"), self.cwd_path, False, None, None + os.path.join(self.cwd_path, ".aws-sam-iacs", "iacs_metadata"), self.cwd_path, False, None, None, False ) self.assertEqual(opts.get("template_file"), self.metadata_path) @@ -367,70 +369,6 @@ def test_valid_hook_package_with_use_container_false_and_no_build_image( args = [] hook_package_id_option.handle_parse_result(ctx, opts, args) self.iac_hook_wrapper_instance_mock.prepare.assert_called_once_with( - os.path.join(self.cwd_path, ".aws-sam-iacs", "iacs_metadata"), self.cwd_path, False, None, None - ) - self.assertEqual(opts.get("template_file"), self.metadata_path) - - @patch("samcli.commands._utils.custom_options.hook_package_id_option.prompt_experimental") - @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.getcwd") - @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.path.exists") - @patch("samcli.commands._utils.custom_options.hook_package_id_option.IacHookWrapper") - def test_invalid_hook_package_with_skip_prepare_iac_missing_metadata_file( - self, iac_hook_wrapper_mock, path_exists_mock, getcwd_mock, prompt_experimental_mock - ): - iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock - prompt_experimental_mock.return_value = True - - getcwd_mock.return_value = self.cwd_path - - path_exists_mock.return_value = False - - hook_package_id_option = HookPackageIdOption( - param_decls=(self.name, self.opt), - force_prepare=True, - invalid_coexist_options=self.invalid_coexist_options, - ) - ctx = MagicMock() - ctx.command.name = "build" - opts = { - "hook_package_id": self.terraform, - "skip_prepare_iac": True, - } - args = [] - with self.assertRaisesRegex( - click.UsageError, "No metadata file found, rerun without the --skip-prepare-iac flag." - ): - hook_package_id_option.handle_parse_result(ctx, opts, args) - - @patch("samcli.commands._utils.custom_options.hook_package_id_option.prompt_experimental") - @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.getcwd") - @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.path.exists") - @patch("samcli.commands._utils.custom_options.hook_package_id_option.os.path.join") - @patch("samcli.commands._utils.custom_options.hook_package_id_option.IacHookWrapper") - def test_valid_hook_package_with_skip_prepare_iac_valid_metadata_file( - self, iac_hook_wrapper_mock, path_join_mock, path_exists_mock, getcwd_mock, prompt_experimental_mock - ): - iac_hook_wrapper_mock.return_value = self.iac_hook_wrapper_instance_mock - prompt_experimental_mock.return_value = True - - getcwd_mock.return_value = self.cwd_path - - path_exists_mock.return_value = True - path_join_mock.return_value = self.metadata_path - - hook_package_id_option = HookPackageIdOption( - param_decls=(self.name, self.opt), - force_prepare=True, - invalid_coexist_options=self.invalid_coexist_options, + os.path.join(self.cwd_path, ".aws-sam-iacs", "iacs_metadata"), self.cwd_path, False, None, None, False ) - ctx = MagicMock() - ctx.command.name = "build" - opts = { - "hook_package_id": self.terraform, - "skip_prepare_iac": True, - } - args = [] - hook_package_id_option.handle_parse_result(ctx, opts, args) - - self.iac_hook_wrapper_instance_mock.prepare.assert_not_called() self.assertEqual(opts.get("template_file"), self.metadata_path) diff --git a/tests/unit/commands/_utils/test_options.py b/tests/unit/commands/_utils/test_options.py index 180cea085b..6715bc620f 100644 --- a/tests/unit/commands/_utils/test_options.py +++ b/tests/unit/commands/_utils/test_options.py @@ -20,7 +20,7 @@ resolve_s3_callback, image_repositories_callback, _space_separated_list_func_type, - skip_prepare_iac_callback, + skip_prepare_infra_callback, ) from samcli.commands._utils.parameterized_option import parameterized_option from samcli.commands.package.exceptions import PackageResolveS3AndS3SetError, PackageResolveS3AndS3NotSetError @@ -497,12 +497,12 @@ def test_option_dec_without_value(self): self.assertEqual(TestParameterizedOption.some_function_without_value(), 4) -class TestSkipPrepareIacOption(TestCase): +class TestSkipPrepareInfraOption(TestCase): def test_skip_with_hook_package(self): ctx_mock = Mock() ctx_mock.params = {"hook_package_id": "test"} - skip_prepare_iac_callback(ctx_mock, Mock(), True) + skip_prepare_infra_callback(ctx_mock, Mock(), True) def test_skip_without_hook_package(self): ctx_mock = Mock() @@ -513,6 +513,6 @@ def test_skip_without_hook_package(self): param_mock.name = "test" with self.assertRaises(click.BadOptionUsage) as ex: - skip_prepare_iac_callback(ctx_mock, param_mock, True) + skip_prepare_infra_callback(ctx_mock, param_mock, True) self.assertEqual(str(ex.exception), "Missing option --hook-package-id") diff --git a/tests/unit/hook_packages/terraform/hooks/prepare/test_hook.py b/tests/unit/hook_packages/terraform/hooks/prepare/test_hook.py index 6d1c74153c..7965aa9f07 100644 --- a/tests/unit/hook_packages/terraform/hooks/prepare/test_hook.py +++ b/tests/unit/hook_packages/terraform/hooks/prepare/test_hook.py @@ -780,6 +780,7 @@ def setUp(self) -> None: "Debug": False, "Profile": None, "Region": None, + "SkipPrepareInfra": False, } def test_get_s3_object_hash(self): @@ -2504,6 +2505,13 @@ def test_enrich_resources_and_generate_makefile_invalid_source_type( sam_metadata_resources, cfn_resources, "/output/dir", "/terraform/project/root" ) + @parameterized.expand( + [ + (False, False), + (False, True), + (True, False), + ] + ) @patch("samcli.hook_packages.terraform.hooks.prepare.hook._update_resources_paths") @patch("samcli.hook_packages.terraform.hooks.prepare.hook._translate_to_cfn") @patch("builtins.open") @@ -2513,6 +2521,8 @@ def test_enrich_resources_and_generate_makefile_invalid_source_type( @patch("samcli.hook_packages.terraform.hooks.prepare.hook.run") def test_prepare( self, + skip_option, + path_exists, mock_subprocess_run, mock_json, mock_os, @@ -2538,10 +2548,12 @@ def test_prepare( named_temporary_file_mock.return_value.__enter__.return_value.name = tf_plan_filename mock_json.loads.return_value = self.tf_json_with_child_modules_and_s3_source_mapping mock_translate_to_cfn.return_value = mock_cfn_dict - mock_os.path.exists.return_value = True + mock_os.path.exists.side_effect = [path_exists, True] mock_os.path.join.return_value = metadata_file_path mock_open.return_value.__enter__.return_value = mock_metadata_file + self.prepare_params["SkipPrepareInfra"] = skip_option + expected_prepare_output_dict = {"iac_applications": {"MainApplication": {"metadata_file": metadata_file_path}}} iac_prepare_output = prepare(self.prepare_params) @@ -3371,3 +3383,22 @@ def test_check_dummy_remote_values_for_image_uri(self): }, } ) + + @patch("samcli.hook_packages.terraform.hooks.prepare.hook.os") + @patch("samcli.hook_packages.terraform.hooks.prepare.hook.LOG.info") + @patch("samcli.hook_packages.terraform.hooks.prepare.hook.run") + def test_skip_prepare_infra_with_metadata_file(self, run_mock, log_info_mock, os_mock): + os_path_join = Mock() + os_mock.path.join = os_path_join + os_mock.path.exists.return_value = True + + self.prepare_params["SkipPrepareInfra"] = True + + prepare(self.prepare_params) + + log_info_mock.assert_called_once() + log_info_mock.assert_called_with( + "Skipping preparation stage, the metadata file already exists at %s", os_path_join() + ) + + run_mock.assert_not_called() diff --git a/tests/unit/lib/hook/test_hook_wrapper.py b/tests/unit/lib/hook/test_hook_wrapper.py index 92fd417f39..658c0fc4f5 100644 --- a/tests/unit/lib/hook/test_hook_wrapper.py +++ b/tests/unit/lib/hook/test_hook_wrapper.py @@ -165,6 +165,7 @@ def test_prepare_with_no_defaults(self, execute_mock, load_hook_package_mock): "Debug": True, "Profile": "my_profile", "Region": "us-east-1", + "SkipPrepareInfra": False, }, ) self.assertEqual(actual, "path/to/metadata") @@ -190,6 +191,7 @@ def test_prepare_with_defaults(self, execute_mock, load_hook_package_mock, cwd_m "IACProjectPath": "path/to/cwd", "OutputDirPath": "path/to/output_dir", "Debug": False, + "SkipPrepareInfra": False, }, ) self.assertEqual(actual, "path/to/metadata") From b7b3def133238a5289da4dd852b840a34726bb31 Mon Sep 17 00:00:00 2001 From: Lucas <12496191+lucashuy@users.noreply.github.com> Date: Fri, 4 Nov 2022 09:42:57 -0700 Subject: [PATCH 5/8] Added checks for default_map --- samcli/commands/_utils/options.py | 12 ++++++++---- tests/unit/commands/_utils/test_options.py | 17 ++++++++++++++--- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/samcli/commands/_utils/options.py b/samcli/commands/_utils/options.py index 4462404e00..b500455081 100644 --- a/samcli/commands/_utils/options.py +++ b/samcli/commands/_utils/options.py @@ -200,7 +200,7 @@ def resolve_s3_callback(ctx, param, provided_value, artifact, exc_set, exc_not_s return provided_value -def skip_prepare_infra_callback(ctx: click.core.Context, param: click.Option, provided_value: bool): +def skip_prepare_infra_callback(ctx, param, provided_value): """ Callback for --skip-prepare-infra to check if --hook-package-id is also specified @@ -213,7 +213,10 @@ def skip_prepare_infra_callback(ctx: click.core.Context, param: click.Option, pr provided_value: bool True if option was provided """ - if provided_value and not ctx.params.get("hook_package_id", None): + is_option_provided = provided_value or ctx.default_map.get("skip_prepare_infra") + is_hook_provided = ctx.params.get("hook_package_id") or ctx.default_map.get("hook_package_id") + + if is_option_provided and not is_hook_provided: raise click.BadOptionUsage(option_name=param.name, ctx=ctx, message="Missing option --hook-package-id") @@ -719,8 +722,9 @@ def skip_prepare_infra_click_option(): is_flag=True, required=False, callback=skip_prepare_infra_callback, - help="Skips the preparation stage for the hook package if the metadata file has already been generated. " - "This option should be used together with --hook-package-id.", + help="This option should be used to skip the preparation stage if there " + "have not been any infrastructure changes. The --hook-name option should " + "also be specified when using this option.", ) diff --git a/tests/unit/commands/_utils/test_options.py b/tests/unit/commands/_utils/test_options.py index 6715bc620f..43289e1bbb 100644 --- a/tests/unit/commands/_utils/test_options.py +++ b/tests/unit/commands/_utils/test_options.py @@ -7,6 +7,7 @@ from unittest import TestCase from unittest.mock import patch, MagicMock +from parameterized import parameterized import click import pytest @@ -498,15 +499,25 @@ def test_option_dec_without_value(self): class TestSkipPrepareInfraOption(TestCase): - def test_skip_with_hook_package(self): + @parameterized.expand( + [ + ({}, {"hook_package_id": "test"}, True), + ({"hook_package_id": "test"}, {}, True), + ({"skip_prepare_infra": True}, {"hook_package_id": "test"}, False), + ({"skip_prepare_infra": True, "hook_package_id": "test"}, {}, False), + ] + ) + def test_skip_with_hook_package(self, default_map, params, provided_value): ctx_mock = Mock() - ctx_mock.params = {"hook_package_id": "test"} + ctx_mock.default_map = default_map + ctx_mock.params = params - skip_prepare_infra_callback(ctx_mock, Mock(), True) + skip_prepare_infra_callback(ctx_mock, Mock(), provided_value) def test_skip_without_hook_package(self): ctx_mock = Mock() ctx_mock.command = Mock() + ctx_mock.default_map = {} ctx_mock.params = {} param_mock = Mock() From 6e1a242bcd765402bf2c8af41e4cd54f34238196 Mon Sep 17 00:00:00 2001 From: Lucas <12496191+lucashuy@users.noreply.github.com> Date: Fri, 4 Nov 2022 16:37:01 -0700 Subject: [PATCH 6/8] Changed hook package to hook name and remove LOG assert --- samcli/commands/_utils/options.py | 6 +++--- tests/unit/commands/_utils/test_options.py | 10 +++++----- .../hook_packages/terraform/hooks/prepare/test_hook.py | 8 +------- 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/samcli/commands/_utils/options.py b/samcli/commands/_utils/options.py index b500455081..199477c2ba 100644 --- a/samcli/commands/_utils/options.py +++ b/samcli/commands/_utils/options.py @@ -202,7 +202,7 @@ def resolve_s3_callback(ctx, param, provided_value, artifact, exc_set, exc_not_s def skip_prepare_infra_callback(ctx, param, provided_value): """ - Callback for --skip-prepare-infra to check if --hook-package-id is also specified + Callback for --skip-prepare-infra to check if --hook-name is also specified Parameters ---------- @@ -214,10 +214,10 @@ def skip_prepare_infra_callback(ctx, param, provided_value): True if option was provided """ is_option_provided = provided_value or ctx.default_map.get("skip_prepare_infra") - is_hook_provided = ctx.params.get("hook_package_id") or ctx.default_map.get("hook_package_id") + is_hook_provided = ctx.params.get("hook_name") or ctx.default_map.get("hook_name") if is_option_provided and not is_hook_provided: - raise click.BadOptionUsage(option_name=param.name, ctx=ctx, message="Missing option --hook-package-id") + raise click.BadOptionUsage(option_name=param.name, ctx=ctx, message="Missing option --hook-name") def template_common_option(f): diff --git a/tests/unit/commands/_utils/test_options.py b/tests/unit/commands/_utils/test_options.py index 43289e1bbb..d05771b74a 100644 --- a/tests/unit/commands/_utils/test_options.py +++ b/tests/unit/commands/_utils/test_options.py @@ -501,10 +501,10 @@ def test_option_dec_without_value(self): class TestSkipPrepareInfraOption(TestCase): @parameterized.expand( [ - ({}, {"hook_package_id": "test"}, True), - ({"hook_package_id": "test"}, {}, True), - ({"skip_prepare_infra": True}, {"hook_package_id": "test"}, False), - ({"skip_prepare_infra": True, "hook_package_id": "test"}, {}, False), + ({}, {"hook_name": "test"}, True), + ({"hook_name": "test"}, {}, True), + ({"skip_prepare_infra": True}, {"hook_name": "test"}, False), + ({"skip_prepare_infra": True, "hook_name": "test"}, {}, False), ] ) def test_skip_with_hook_package(self, default_map, params, provided_value): @@ -526,4 +526,4 @@ def test_skip_without_hook_package(self): with self.assertRaises(click.BadOptionUsage) as ex: skip_prepare_infra_callback(ctx_mock, param_mock, True) - self.assertEqual(str(ex.exception), "Missing option --hook-package-id") + self.assertEqual(str(ex.exception), "Missing option --hook-name") diff --git a/tests/unit/hook_packages/terraform/hooks/prepare/test_hook.py b/tests/unit/hook_packages/terraform/hooks/prepare/test_hook.py index c44c0e3f83..2dc43645ee 100644 --- a/tests/unit/hook_packages/terraform/hooks/prepare/test_hook.py +++ b/tests/unit/hook_packages/terraform/hooks/prepare/test_hook.py @@ -3566,9 +3566,8 @@ def test_check_dummy_remote_values_for_image_uri(self): ) @patch("samcli.hook_packages.terraform.hooks.prepare.hook.os") - @patch("samcli.hook_packages.terraform.hooks.prepare.hook.LOG.info") @patch("samcli.hook_packages.terraform.hooks.prepare.hook.run") - def test_skip_prepare_infra_with_metadata_file(self, run_mock, log_info_mock, os_mock): + def test_skip_prepare_infra_with_metadata_file(self, run_mock, os_mock): os_path_join = Mock() os_mock.path.join = os_path_join os_mock.path.exists.return_value = True @@ -3577,11 +3576,6 @@ def test_skip_prepare_infra_with_metadata_file(self, run_mock, log_info_mock, os prepare(self.prepare_params) - log_info_mock.assert_called_once() - log_info_mock.assert_called_with( - "Skipping preparation stage, the metadata file already exists at %s", os_path_join() - ) - run_mock.assert_not_called() def test_add_metadata_resource_to_metadata_list(self): From 52a69af52acbf7f4af6b4809eb7f68ab6722d587 Mon Sep 17 00:00:00 2001 From: Lucas <12496191+lucashuy@users.noreply.github.com> Date: Fri, 4 Nov 2022 16:46:41 -0700 Subject: [PATCH 7/8] Added log message if we provided --skip-prepare-infra but have no metadata file --- samcli/hook_packages/terraform/hooks/prepare/hook.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/samcli/hook_packages/terraform/hooks/prepare/hook.py b/samcli/hook_packages/terraform/hooks/prepare/hook.py index 5d7f4fb2a9..4bfdfbf68e 100644 --- a/samcli/hook_packages/terraform/hooks/prepare/hook.py +++ b/samcli/hook_packages/terraform/hooks/prepare/hook.py @@ -133,6 +133,12 @@ def prepare(params: dict) -> dict: if skip_prepare_infra and os.path.exists(metadata_file_path): LOG.info("Skipping preparation stage, the metadata file already exists at %s", metadata_file_path) else: + if skip_prepare_infra: + LOG.info( + "The option to skip infrastructure preparation was provided, but we could not find " + "the metadata file. Preparing anyways." + ) + try: # initialize terraform application LOG.info("Initializing Terraform application") From 0d581f68f41c703c76ff8d931a7f10629292ebf3 Mon Sep 17 00:00:00 2001 From: Lucas <12496191+lucashuy@users.noreply.github.com> Date: Fri, 4 Nov 2022 17:04:18 -0700 Subject: [PATCH 8/8] Removed lingering extra decorator from earlier merge --- samcli/commands/build/command.py | 1 - samcli/commands/local/invoke/cli.py | 1 - samcli/commands/local/start_lambda/cli.py | 1 - 3 files changed, 3 deletions(-) diff --git a/samcli/commands/build/command.py b/samcli/commands/build/command.py index d08872ab3f..392f4da3ad 100644 --- a/samcli/commands/build/command.py +++ b/samcli/commands/build/command.py @@ -86,7 +86,6 @@ force_prepare=True, invalid_coexist_options=["t", "template-file", "template", "parameter-overrides"], ) -@configuration_option(provider=TomlProvider(section="parameters")) @skip_prepare_infra_option @use_container_build_option @click.option( diff --git a/samcli/commands/local/invoke/cli.py b/samcli/commands/local/invoke/cli.py index b87eb6b253..56ad797848 100644 --- a/samcli/commands/local/invoke/cli.py +++ b/samcli/commands/local/invoke/cli.py @@ -40,7 +40,6 @@ @hook_package_id_click_option( force_prepare=False, invalid_coexist_options=["t", "template-file", "template", "parameter-overrides"] ) -@configuration_option(provider=TomlProvider(section="parameters")) @skip_prepare_infra_option @click.option( "--event", diff --git a/samcli/commands/local/start_lambda/cli.py b/samcli/commands/local/start_lambda/cli.py index 2bba490499..bcf88c3263 100644 --- a/samcli/commands/local/start_lambda/cli.py +++ b/samcli/commands/local/start_lambda/cli.py @@ -64,7 +64,6 @@ @hook_package_id_click_option( force_prepare=False, invalid_coexist_options=["t", "template-file", "template", "parameter-overrides"] ) -@configuration_option(provider=TomlProvider(section="parameters")) @skip_prepare_infra_option @service_common_options(3001) @invoke_common_options