From e019b78f94ec1cff24e4693e5fffe84b3b6e978d Mon Sep 17 00:00:00 2001 From: Sriram Madapusi Vasudevan Date: Fri, 22 Nov 2019 14:46:26 -0800 Subject: [PATCH 1/2] fix: add version to `samconfig.toml` file - support version key, any float is okay. - if a config file is present and the version key is missing, we do not process it. - if a config file is missing, thats fine. this check does not get in the way. - validation logic to determine if a SAM CLI version is compatible can be written later. --- samcli/cli/cli_config_file.py | 9 +++ samcli/commands/exceptions.py | 6 ++ samcli/lib/config/exceptions.py | 7 ++ samcli/lib/config/samconfig.py | 36 +++++++--- samcli/lib/config/version.py | 6 ++ tests/unit/cli/test_cli_config_file.py | 20 +++++- tests/unit/commands/deploy/test_command.py | 77 ++++++++++++++++++++++ tests/unit/lib/samconfig/test_samconfig.py | 30 +++++++++ 8 files changed, 178 insertions(+), 13 deletions(-) create mode 100644 samcli/lib/config/exceptions.py create mode 100644 samcli/lib/config/version.py diff --git a/samcli/cli/cli_config_file.py b/samcli/cli/cli_config_file.py index 032717fbfe..db346472ee 100644 --- a/samcli/cli/cli_config_file.py +++ b/samcli/cli/cli_config_file.py @@ -10,6 +10,9 @@ import logging import click + +from samcli.commands.exceptions import ConfigException +from samcli.lib.config.exceptions import SamConfigVersionException from samcli.cli.context import get_cmd_names from samcli.lib.config.samconfig import SamConfig, DEFAULT_ENV @@ -53,6 +56,7 @@ def __call__(self, config_dir, config_env, cmd_names): # NOTE(TheSriram): change from tomlkit table type to normal dictionary, # so that click defaults work out of the box. + samconfig.sanity_check() resolved_config = {k: v for k, v in samconfig.get_all(cmd_names, self.section, env=config_env).items()} LOG.debug("Configuration values read from the file: %s", resolved_config) @@ -65,6 +69,11 @@ def __call__(self, config_dir, config_env, cmd_names): self.section, str(ex), ) + + except SamConfigVersionException as ex: + LOG.debug("%s %s", samconfig.path(), str(ex)) + raise ConfigException(f"Syntax invalid in samconfig.toml: {str(ex)}") + except Exception as ex: LOG.debug("Error reading configuration file: %s %s", samconfig.path(), str(ex)) diff --git a/samcli/commands/exceptions.py b/samcli/commands/exceptions.py index 159d05bea4..932bccf480 100644 --- a/samcli/commands/exceptions.py +++ b/samcli/commands/exceptions.py @@ -5,6 +5,12 @@ import click +class ConfigException(click.ClickException): + """ + Exception class when configuration file fails checks. + """ + + class UserException(click.ClickException): """ Base class for all exceptions that need to be surfaced to the user. Typically, we will display the exception diff --git a/samcli/lib/config/exceptions.py b/samcli/lib/config/exceptions.py new file mode 100644 index 0000000000..50297ce722 --- /dev/null +++ b/samcli/lib/config/exceptions.py @@ -0,0 +1,7 @@ +""" +Exceptions to be used by samconfig.py +""" + + +class SamConfigVersionException(Exception): + pass diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index bc47a44ae4..ef14a1f54e 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -9,6 +9,9 @@ import tomlkit +from samcli.lib.config.version import SAM_CONFIG_VERSION, VERSION_KEY +from samcli.lib.config.exceptions import SamConfigVersionException + LOG = logging.getLogger(__name__) DEFAULT_CONFIG_FILE_NAME = "samconfig.toml" @@ -21,7 +24,6 @@ class SamConfig: """ document = None - VERSION = "0.1" def __init__(self, config_dir, filename=None): """ @@ -149,15 +151,16 @@ def config_dir(template_file_path=None): return os.getcwd() def _read(self): - if self.document: - return self.document - - try: - txt = self.filepath.read_text() - self.document = tomlkit.loads(txt) - except OSError: - self.document = tomlkit.document() - + if not self.document: + try: + txt = self.filepath.read_text() + self.document = tomlkit.loads(txt) + self._version_sanity_check(self._version()) + except OSError: + self.document = tomlkit.document() + + if self.document.body: + self._version_sanity_check(self._version()) return self.document def _write(self): @@ -166,8 +169,21 @@ def _write(self): if not self.exists(): open(self.filepath, "a+").close() + current_version = self._version() if self._version() else SAM_CONFIG_VERSION + try: + self.document.add(VERSION_KEY, current_version) + except tomlkit.exceptions.KeyAlreadyPresent: + # NOTE(TheSriram): Do not attempt to re-write an existing version + pass self.filepath.write_text(tomlkit.dumps(self.document)) + def _version(self): + return self.document.get(VERSION_KEY, None) + + def _version_sanity_check(self, version): + if not isinstance(version, float): + raise SamConfigVersionException(f"'{VERSION_KEY}' key is not present or is in unrecognized format. ") + @staticmethod def _to_key(cmd_names): # construct a parsed name that is of the format: a_b_c_d diff --git a/samcli/lib/config/version.py b/samcli/lib/config/version.py new file mode 100644 index 0000000000..bd5a7f330f --- /dev/null +++ b/samcli/lib/config/version.py @@ -0,0 +1,6 @@ +""" +Constants and helper functions for samconfig.toml's versioning. +""" + +SAM_CONFIG_VERSION = 0.1 +VERSION_KEY = "version" diff --git a/tests/unit/cli/test_cli_config_file.py b/tests/unit/cli/test_cli_config_file.py index 490e4029b0..63caae55a9 100644 --- a/tests/unit/cli/test_cli_config_file.py +++ b/tests/unit/cli/test_cli_config_file.py @@ -2,9 +2,9 @@ from pathlib import Path from unittest import TestCase -from unittest.mock import MagicMock, patch - +from unittest.mock import MagicMock +from samcli.commands.exceptions import ConfigException from samcli.cli.cli_config_file import TomlProvider, configuration_option, configuration_callback, get_ctx_defaults from samcli.lib.config.samconfig import SamConfig @@ -27,11 +27,25 @@ def setUp(self): def test_toml_valid_with_section(self): config_dir = tempfile.gettempdir() configpath = Path(config_dir, "samconfig.toml") - configpath.write_text("[config_env.topic.parameters]\nword='clarity'\n") + configpath.write_text("version=0.1\n[config_env.topic.parameters]\nword='clarity'\n") self.assertEqual( TomlProvider(section=self.parameters)(config_dir, self.config_env, [self.cmd_name]), {"word": "clarity"} ) + def test_toml_valid_with_no_version(self): + config_dir = tempfile.gettempdir() + configpath = Path(config_dir, "samconfig.toml") + configpath.write_text("[config_env.topic.parameters]\nword='clarity'\n") + with self.assertRaises(ConfigException): + TomlProvider(section=self.parameters)(config_dir, self.config_env, [self.cmd_name]) + + def test_toml_valid_with_invalid_version(self): + config_dir = tempfile.gettempdir() + configpath = Path(config_dir, "samconfig.toml") + configpath.write_text("version='abc'\n[config_env.topic.parameters]\nword='clarity'\n") + with self.assertRaises(ConfigException): + TomlProvider(section=self.parameters)(config_dir, self.config_env, [self.cmd_name]) + def test_toml_invalid_empty_dict(self): config_dir = tempfile.gettempdir() configpath = Path(config_dir, "samconfig.toml") diff --git a/tests/unit/commands/deploy/test_command.py b/tests/unit/commands/deploy/test_command.py index 5f7331494d..4ea77dc5d5 100644 --- a/tests/unit/commands/deploy/test_command.py +++ b/tests/unit/commands/deploy/test_command.py @@ -421,3 +421,80 @@ def test_all_args_guided_no_params_no_save_config( self.assertEqual(mock_save_config.call_count, 0) mock_managed_stack.assert_called_with(profile=self.profile, region="us-east-1") self.assertEqual(context_mock.run.call_count, 1) + + @patch("samcli.commands.package.command.click") + @patch("samcli.commands.package.package_context.PackageContext") + @patch("samcli.commands.deploy.command.click") + @patch("samcli.commands.deploy.deploy_context.DeployContext") + @patch("samcli.commands.deploy.command.save_config") + @patch("samcli.commands.deploy.command.manage_stack") + @patch("samcli.commands.deploy.command.get_template_parameters") + @patch("samcli.commands.deploy.command.get_config_ctx") + def test_all_args_guided_no_params_no_save_config( + self, + mock_get_config_ctx, + mock_get_template_parameters, + mock_managed_stack, + mock_save_config, + mock_deploy_context, + mock_deploy_click, + mock_package_context, + mock_package_click, + ): + + context_mock = Mock() + mock_sam_config = MagicMock() + mock_sam_config.exists = MagicMock(return_value=True) + mock_get_config_ctx.return_value = (None, mock_sam_config) + mock_get_template_parameters.return_value = {} + mock_deploy_context.return_value.__enter__.return_value = context_mock + mock_deploy_click.prompt = MagicMock(side_effect=["sam-app", "us-east-1", ("CAPABILITY_IAM",)]) + mock_deploy_click.confirm = MagicMock(side_effect=[True, False, False]) + + mock_managed_stack.return_value = "managed-s3-bucket" + + do_cli( + template_file=self.template_file, + stack_name=self.stack_name, + s3_bucket=None, + force_upload=self.force_upload, + s3_prefix=self.s3_prefix, + kms_key_id=self.kms_key_id, + parameter_overrides=self.parameter_overrides, + capabilities=self.capabilities, + no_execute_changeset=self.no_execute_changeset, + role_arn=self.role_arn, + notification_arns=self.notification_arns, + fail_on_empty_changeset=self.fail_on_empty_changset, + tags=self.tags, + region=self.region, + profile=self.profile, + use_json=self.use_json, + metadata=self.metadata, + guided=True, + confirm_changeset=True, + ) + + mock_deploy_context.assert_called_with( + template_file=ANY, + stack_name="sam-app", + s3_bucket="managed-s3-bucket", + force_upload=self.force_upload, + s3_prefix="sam-app", + kms_key_id=self.kms_key_id, + parameter_overrides=self.parameter_overrides, + capabilities=self.capabilities, + no_execute_changeset=self.no_execute_changeset, + role_arn=self.role_arn, + notification_arns=self.notification_arns, + fail_on_empty_changeset=self.fail_on_empty_changset, + tags=self.tags, + region="us-east-1", + profile=self.profile, + confirm_changeset=True, + ) + + context_mock.run.assert_called_with() + self.assertEqual(mock_save_config.call_count, 0) + mock_managed_stack.assert_called_with(profile=self.profile, region="us-east-1") + self.assertEqual(context_mock.run.call_count, 1) diff --git a/tests/unit/lib/samconfig/test_samconfig.py b/tests/unit/lib/samconfig/test_samconfig.py index 54575c1198..d6277db9ed 100644 --- a/tests/unit/lib/samconfig/test_samconfig.py +++ b/tests/unit/lib/samconfig/test_samconfig.py @@ -3,6 +3,8 @@ from unittest import TestCase +from samcli.lib.config.exceptions import SamConfigVersionException +from samcli.lib.config.version import VERSION_KEY, SAM_CONFIG_VERSION from samcli.lib.config.samconfig import SamConfig, DEFAULT_CONFIG_FILE_NAME @@ -20,6 +22,7 @@ def _setup_config(self): self.samconfig.flush() self.assertTrue(self.samconfig.exists()) self.assertTrue(self.samconfig.sanity_check()) + self.assertEqual(SAM_CONFIG_VERSION, self.samconfig.document.get(VERSION_KEY)) def test_init(self): self.assertEqual(self.samconfig.filepath, Path(self.config_dir, DEFAULT_CONFIG_FILE_NAME)) @@ -36,3 +39,30 @@ def test_check_config_exists(self): def test_check_sanity(self): self.assertTrue(self.samconfig.sanity_check()) + + def test_check_version_non_supported_type(self): + self._setup_config() + self.samconfig.document.remove(VERSION_KEY) + self.samconfig.document.add(VERSION_KEY, "aadeff") + with self.assertRaises(SamConfigVersionException): + self.samconfig.sanity_check() + + def test_check_version_no_version_exists(self): + self._setup_config() + self.samconfig.document.remove(VERSION_KEY) + with self.assertRaises(SamConfigVersionException): + self.samconfig.sanity_check() + + def test_check_version_float(self): + self._setup_config() + self.samconfig.document.remove(VERSION_KEY) + self.samconfig.document.add(VERSION_KEY, 0.2) + self.samconfig.sanity_check() + + def test_write_config_file_non_standard_version(self): + self._setup_config() + self.samconfig.document.remove(VERSION_KEY) + self.samconfig.document.add(VERSION_KEY, 0.2) + self.samconfig.put(cmd_names=["local", "start", "api"], section="parameters", key="skip_pull_image", value=True) + self.samconfig.sanity_check() + self.assertEqual(self.samconfig.document.get(VERSION_KEY), 0.2) From a0d573a4d957bb644351443ad6640aa4b6b8aa15 Mon Sep 17 00:00:00 2001 From: Sriram Madapusi Vasudevan Date: Fri, 22 Nov 2019 15:57:49 -0800 Subject: [PATCH 2/2] bugfix: do not continously read everytime a samconfig.put is called --- samcli/lib/config/samconfig.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index ef14a1f54e..e8f6eaff7c 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -103,8 +103,8 @@ def put(self, cmd_names, section, key, value, env=DEFAULT_ENV): If the data is invalid """ - self._read() if not self.document: + self._read() # Empty document prepare the initial structure. self.document.update({env: {self._to_key(cmd_names): {section: {key: value}}}}) # Only update appropriate key value pairs within a section