fix: add version to samconfig.toml file (#1581)

* 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.

* bugfix: do not continously read everytime a samconfig.put is called

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))
 

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

samcli/lib/config/exceptions.py

@@ -0,0 +1,7 @@
+"""
+Exceptions to be used by samconfig.py
+"""
+
+
+class SamConfigVersionException(Exception):
+    pass

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):
         """
@@ -101,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
@@ -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

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"

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")

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)

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)