From c9c02f221f608bc3ecd00243e0448ee98f1d4774 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Wed, 7 Jun 2023 16:33:15 -0700 Subject: [PATCH 1/7] Add hierarchy for samconfig default filetypes --- samcli/lib/config/file_manager.py | 2 +- samcli/lib/config/samconfig.py | 25 +++++- .../unit/commands/samconfig/test_samconfig.py | 35 -------- tests/unit/lib/samconfig/test_samconfig.py | 80 ++++++++++++++++++- 4 files changed, 101 insertions(+), 41 deletions(-) diff --git a/samcli/lib/config/file_manager.py b/samcli/lib/config/file_manager.py index a898a5add4..abbdade71d 100644 --- a/samcli/lib/config/file_manager.py +++ b/samcli/lib/config/file_manager.py @@ -187,7 +187,7 @@ def read(filepath: Path) -> Any: A dictionary-like yaml object, which represents the contents of the YAML file at the provided location. """ - yaml_doc = YamlFileManager.yaml.load("") + yaml_doc = {} try: yaml_doc = YamlFileManager.yaml.load(filepath.read_text()) except OSError as e: diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index 6b3e99bdd8..720d42b6a8 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -14,7 +14,8 @@ LOG = logging.getLogger(__name__) DEFAULT_CONFIG_FILE_EXTENSION = ".toml" -DEFAULT_CONFIG_FILE_NAME = f"samconfig{DEFAULT_CONFIG_FILE_EXTENSION}" +DEFAULT_CONFIG_FILE = "samconfig" +DEFAULT_CONFIG_FILE_NAME = DEFAULT_CONFIG_FILE + DEFAULT_CONFIG_FILE_EXTENSION DEFAULT_ENV = "default" DEFAULT_GLOBAL_CMDNAME = "global" @@ -24,7 +25,7 @@ class SamConfig: Class to represent `samconfig` config options. """ - FILE_MANAGER_MAPPER: Dict[str, Type[FileManager]] = { + FILE_MANAGER_MAPPER: Dict[str, Type[FileManager]] = { # keys ordered by priority ".toml": TomlFileManager, ".yaml": YamlFileManager, ".yml": YamlFileManager, @@ -44,7 +45,7 @@ def __init__(self, config_dir, filename=None): could automatically support auto-resolving multiple config files within same directory. """ self.document = {} - self.filepath = Path(config_dir, filename or DEFAULT_CONFIG_FILE_NAME) + self.filepath = Path(config_dir, filename or self._get_default_file(config_dir=config_dir)) self.file_manager = self.FILE_MANAGER_MAPPER.get(self.filepath.suffix, None) if not self.file_manager: LOG.warning( @@ -247,6 +248,24 @@ def _deduplicate_global_parameters(self, cmd_name_key, section, key, env=DEFAULT # Only keep the global parameter del self.document[env][cmd_name_key][section][key] + def _get_default_file(self, config_dir: str) -> str: + """Return a defaultly-named config file, if it exists, otherwise the current default.""" + config_files_found = 0 + config_file = DEFAULT_CONFIG_FILE_NAME + + for extension in reversed(self.FILE_MANAGER_MAPPER.keys()): + filename = DEFAULT_CONFIG_FILE + extension + if Path(config_dir, filename).exists(): + config_files_found += 1 + config_file = filename + + if config_files_found == 0: # Config file doesn't exist (yet!) + LOG.debug(f"No config file exists yet. Creating one as {config_file}.") + elif config_files_found > 1: # Multiple config files; let user know which is used + LOG.debug(f"More than one samconfig file found. Using {config_file}.") + + return config_file + @staticmethod def _version_sanity_check(version: Any) -> None: if not isinstance(version, float): diff --git a/tests/unit/commands/samconfig/test_samconfig.py b/tests/unit/commands/samconfig/test_samconfig.py index b35b755a1f..b12a211790 100644 --- a/tests/unit/commands/samconfig/test_samconfig.py +++ b/tests/unit/commands/samconfig/test_samconfig.py @@ -14,9 +14,6 @@ from unittest import TestCase from unittest.mock import patch, ANY import logging -from parameterized import parameterized -from samcli.lib.config.exceptions import SamConfigFileReadException -from samcli.lib.config.file_manager import JsonFileManager, TomlFileManager, YamlFileManager from samcli.lib.config.samconfig import SamConfig, DEFAULT_ENV from samcli.lib.utils.packagetype import ZIP, IMAGE @@ -1248,38 +1245,6 @@ def test_secondary_option_name_template_validate(self, do_cli_mock): do_cli_mock.assert_called_with(ANY, str(Path(os.getcwd(), "mytemplate.yaml")), False) -class TestSamConfigFileManager(TestCase): - def test_file_manager_not_declared(self): - config_dir = tempfile.gettempdir() - config_path = Path(config_dir, "samconfig") - - with self.assertRaises(SamConfigFileReadException): - SamConfig(config_path, filename="samconfig") - - def test_file_manager_unsupported(self): - config_dir = tempfile.gettempdir() - config_path = Path(config_dir, "samconfig.jpeg") - - with self.assertRaises(SamConfigFileReadException): - SamConfig(config_path, filename="samconfig.jpeg") - - @parameterized.expand( - [ - ("samconfig.toml", TomlFileManager), - ("samconfig.yaml", YamlFileManager), - ("samconfig.yml", YamlFileManager), - ("samconfig.json", JsonFileManager), - ] - ) - def test_file_manager(self, filename, expected_file_manager): - config_dir = tempfile.gettempdir() - config_path = Path(config_dir, filename) - - samconfig = SamConfig(config_path, filename=filename) - - self.assertIs(samconfig.file_manager, expected_file_manager) - - @contextmanager def samconfig_parameters(cmd_names, config_dir=None, env=None, **kwargs): """ diff --git a/tests/unit/lib/samconfig/test_samconfig.py b/tests/unit/lib/samconfig/test_samconfig.py index 346353501b..742d38c0c0 100644 --- a/tests/unit/lib/samconfig/test_samconfig.py +++ b/tests/unit/lib/samconfig/test_samconfig.py @@ -1,9 +1,12 @@ import os from pathlib import Path +from parameterized import parameterized +import tempfile from unittest import TestCase -from samcli.lib.config.exceptions import SamConfigVersionException -from samcli.lib.config.samconfig import SamConfig, DEFAULT_CONFIG_FILE_NAME, DEFAULT_GLOBAL_CMDNAME, DEFAULT_ENV +from samcli.lib.config.exceptions import SamConfigFileReadException, SamConfigVersionException +from samcli.lib.config.file_manager import JsonFileManager, TomlFileManager, YamlFileManager +from samcli.lib.config.samconfig import DEFAULT_CONFIG_FILE, SamConfig, DEFAULT_CONFIG_FILE_NAME, DEFAULT_GLOBAL_CMDNAME, DEFAULT_ENV from samcli.lib.config.version import VERSION_KEY, SAM_CONFIG_VERSION from samcli.lib.utils import osutils @@ -221,3 +224,76 @@ def test_write_config_file_will_create_the_file_if_not_exist(self): samconfig.put(cmd_names=["any", "command"], section="any-section", key="any-key", value="any-value") samconfig.flush() self.assertTrue(samconfig.exists()) + + def test_passed_filename_used(self): + config_path = Path(self.config_dir, "myconfigfile.toml") + + self.assertFalse(config_path.exists()) + + self.samconfig = SamConfig(self.config_dir, filename="myconfigfile.toml") + self.samconfig.put( # put some config options so it creates the file + cmd_names=["any", "command"], section="section", key="key", value="value" + ) + self.samconfig.flush() + + self.assertTrue(config_path.exists()) + self.assertFalse(Path(self.config_dir, DEFAULT_CONFIG_FILE_NAME).exists()) + + def test_config_uses_default_if_none_provided(self): + self.samconfig = SamConfig(self.config_dir) + self.samconfig.put( # put some config options so it creates the file + cmd_names=["any", "command"], section="section", key="key", value="value" + ) + self.samconfig.flush() + + self.assertTrue(Path(self.config_dir, DEFAULT_CONFIG_FILE_NAME).exists()) + + def test_config_priority(self): + config_files = [] + extensions_in_priority = list(SamConfig.FILE_MANAGER_MAPPER.keys()) # priority by order in dict + for extension in extensions_in_priority: + filename = DEFAULT_CONFIG_FILE + extension + config = SamConfig(self.config_dir, filename=filename) + config.put( # put some config options so it creates the file + cmd_names=["any", "command"], section="section", key="key", value="value" + ) + config.flush() + config_files.append(config) + + while extensions_in_priority: + config = SamConfig(self.config_dir) + next_priority = extensions_in_priority.pop(0) + self.assertEqual(config.path(), self.config_dir + "/" + DEFAULT_CONFIG_FILE + next_priority) + os.remove(config.path()) + + +class TestSamConfigFileManager(TestCase): + def test_file_manager_not_declared(self): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, "samconfig") + + with self.assertRaises(SamConfigFileReadException): + SamConfig(config_path, filename="samconfig") + + def test_file_manager_unsupported(self): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, "samconfig.jpeg") + + with self.assertRaises(SamConfigFileReadException): + SamConfig(config_path, filename="samconfig.jpeg") + + @parameterized.expand( + [ + ("samconfig.toml", TomlFileManager), + ("samconfig.yaml", YamlFileManager), + ("samconfig.yml", YamlFileManager), + ("samconfig.json", JsonFileManager), + ] + ) + def test_file_manager(self, filename, expected_file_manager): + config_dir = tempfile.gettempdir() + config_path = Path(config_dir, filename) + + samconfig = SamConfig(config_path, filename=filename) + + self.assertIs(samconfig.file_manager, expected_file_manager) From c07e4bae1d1b22779b5e4f3ad7656fbfb18c0f30 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Wed, 7 Jun 2023 16:44:39 -0700 Subject: [PATCH 2/7] Formatting and fixing tests --- tests/unit/lib/samconfig/test_file_manager.py | 2 +- tests/unit/lib/samconfig/test_samconfig.py | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/unit/lib/samconfig/test_file_manager.py b/tests/unit/lib/samconfig/test_file_manager.py index 362d0a3b27..eb9325fc37 100644 --- a/tests/unit/lib/samconfig/test_file_manager.py +++ b/tests/unit/lib/samconfig/test_file_manager.py @@ -134,7 +134,7 @@ def test_read_yaml_file_path_not_valid(self): config_doc = YamlFileManager.read(config_path) - self.assertEqual(config_doc, self.yaml.load("")) + self.assertEqual(config_doc, {}) def test_write_yaml(self): config_dir = tempfile.gettempdir() diff --git a/tests/unit/lib/samconfig/test_samconfig.py b/tests/unit/lib/samconfig/test_samconfig.py index 742d38c0c0..16e94f0078 100644 --- a/tests/unit/lib/samconfig/test_samconfig.py +++ b/tests/unit/lib/samconfig/test_samconfig.py @@ -6,7 +6,13 @@ from samcli.lib.config.exceptions import SamConfigFileReadException, SamConfigVersionException from samcli.lib.config.file_manager import JsonFileManager, TomlFileManager, YamlFileManager -from samcli.lib.config.samconfig import DEFAULT_CONFIG_FILE, SamConfig, DEFAULT_CONFIG_FILE_NAME, DEFAULT_GLOBAL_CMDNAME, DEFAULT_ENV +from samcli.lib.config.samconfig import ( + DEFAULT_CONFIG_FILE, + SamConfig, + DEFAULT_CONFIG_FILE_NAME, + DEFAULT_GLOBAL_CMDNAME, + DEFAULT_ENV, +) from samcli.lib.config.version import VERSION_KEY, SAM_CONFIG_VERSION from samcli.lib.utils import osutils From 48064a69309200894cecab7bba1a3c11c67a3f5f Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Thu, 8 Jun 2023 10:32:15 -0700 Subject: [PATCH 3/7] Implement requested changes --- samcli/lib/config/samconfig.py | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index 720d42b6a8..9793a54569 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -249,7 +249,19 @@ def _deduplicate_global_parameters(self, cmd_name_key, section, key, env=DEFAULT del self.document[env][cmd_name_key][section][key] def _get_default_file(self, config_dir: str) -> str: - """Return a defaultly-named config file, if it exists, otherwise the current default.""" + """Return a defaultly-named config file, if it exists, otherwise the current default. + + Parameters + ---------- + config_dir: str + The name of the directory where the config file is/will be stored. + + Returns + ------- + str + The name of the config file found, if it exists. In the case that it does not exist, the default config + file name is returned instead. + """ config_files_found = 0 config_file = DEFAULT_CONFIG_FILE_NAME @@ -260,9 +272,12 @@ def _get_default_file(self, config_dir: str) -> str: config_file = filename if config_files_found == 0: # Config file doesn't exist (yet!) - LOG.debug(f"No config file exists yet. Creating one as {config_file}.") + LOG.info(f"No config file found. Creating one as {config_file}.") elif config_files_found > 1: # Multiple config files; let user know which is used - LOG.debug(f"More than one samconfig file found. Using {config_file}.") + LOG.info( + f"More than one samconfig file found; using {config_file}." + f" To use another config file, please specify it using the '--config-file' flag." + ) return config_file From 9b4da6615cbfbb33007cc26e15250fccd33818db Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Thu, 8 Jun 2023 10:41:04 -0700 Subject: [PATCH 4/7] Fix logic to properly allow default name --- samcli/cli/cli_config_file.py | 4 +++- samcli/lib/config/samconfig.py | 7 ++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/samcli/cli/cli_config_file.py b/samcli/cli/cli_config_file.py index 8b56d033e9..2929a434e9 100644 --- a/samcli/cli/cli_config_file.py +++ b/samcli/cli/cli_config_file.py @@ -67,7 +67,9 @@ def __call__(self, config_path: Path, config_env: str, cmd_names: List[str]) -> # Use default sam config file name if config_path only contain the directory config_file_path = ( - Path(os.path.abspath(config_path)) if config_path else Path(os.getcwd(), DEFAULT_CONFIG_FILE_NAME) + Path(os.path.abspath(config_path)) + if config_path + else Path(os.getcwd(), SamConfig.get_default_file(os.getcwd())) ) config_file_name = config_file_path.name config_file_dir = config_file_path.parents[0] diff --git a/samcli/lib/config/samconfig.py b/samcli/lib/config/samconfig.py index 9793a54569..a4950fc5c1 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -45,7 +45,7 @@ def __init__(self, config_dir, filename=None): could automatically support auto-resolving multiple config files within same directory. """ self.document = {} - self.filepath = Path(config_dir, filename or self._get_default_file(config_dir=config_dir)) + self.filepath = Path(config_dir, filename or self.get_default_file(config_dir=config_dir)) self.file_manager = self.FILE_MANAGER_MAPPER.get(self.filepath.suffix, None) if not self.file_manager: LOG.warning( @@ -248,7 +248,8 @@ def _deduplicate_global_parameters(self, cmd_name_key, section, key, env=DEFAULT # Only keep the global parameter del self.document[env][cmd_name_key][section][key] - def _get_default_file(self, config_dir: str) -> str: + @staticmethod + def get_default_file(config_dir: str) -> str: """Return a defaultly-named config file, if it exists, otherwise the current default. Parameters @@ -265,7 +266,7 @@ def _get_default_file(self, config_dir: str) -> str: config_files_found = 0 config_file = DEFAULT_CONFIG_FILE_NAME - for extension in reversed(self.FILE_MANAGER_MAPPER.keys()): + for extension in reversed(SamConfig.FILE_MANAGER_MAPPER.keys()): filename = DEFAULT_CONFIG_FILE + extension if Path(config_dir, filename).exists(): config_files_found += 1 From ec3ccc06cdccd0db8c0b292d96ad349507cb7d90 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Thu, 8 Jun 2023 10:57:59 -0700 Subject: [PATCH 5/7] Fix linting issue --- 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 a4950fc5c1..a77e44c70c 100644 --- a/samcli/lib/config/samconfig.py +++ b/samcli/lib/config/samconfig.py @@ -266,7 +266,7 @@ def get_default_file(config_dir: str) -> str: config_files_found = 0 config_file = DEFAULT_CONFIG_FILE_NAME - for extension in reversed(SamConfig.FILE_MANAGER_MAPPER.keys()): + for extension in reversed(list(SamConfig.FILE_MANAGER_MAPPER.keys())): filename = DEFAULT_CONFIG_FILE + extension if Path(config_dir, filename).exists(): config_files_found += 1 From 8bc35ec63425318fadd92b45d90472bf2f00aa09 Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Thu, 8 Jun 2023 11:23:39 -0700 Subject: [PATCH 6/7] Fix failing Windows test --- tests/unit/lib/samconfig/test_samconfig.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/lib/samconfig/test_samconfig.py b/tests/unit/lib/samconfig/test_samconfig.py index 16e94f0078..4250e3d9cd 100644 --- a/tests/unit/lib/samconfig/test_samconfig.py +++ b/tests/unit/lib/samconfig/test_samconfig.py @@ -269,7 +269,7 @@ def test_config_priority(self): while extensions_in_priority: config = SamConfig(self.config_dir) next_priority = extensions_in_priority.pop(0) - self.assertEqual(config.path(), self.config_dir + "/" + DEFAULT_CONFIG_FILE + next_priority) + self.assertEqual(config.filepath, Path(self.config_dir, DEFAULT_CONFIG_FILE + next_priority)) os.remove(config.path()) From a636f919e202ce6062e372c6e1ae8e5ad79a5e7e Mon Sep 17 00:00:00 2001 From: Leonardo Gama Date: Thu, 8 Jun 2023 15:11:20 -0700 Subject: [PATCH 7/7] Update default config name in guided config --- samcli/commands/deploy/guided_config.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/samcli/commands/deploy/guided_config.py b/samcli/commands/deploy/guided_config.py index 10b80189db..78866944cd 100644 --- a/samcli/commands/deploy/guided_config.py +++ b/samcli/commands/deploy/guided_config.py @@ -8,7 +8,7 @@ from samcli.cli.context import get_cmd_names from samcli.commands.deploy.exceptions import GuidedDeployFailedError from samcli.lib.config.exceptions import SamConfigFileReadException -from samcli.lib.config.samconfig import DEFAULT_CONFIG_FILE_NAME, DEFAULT_ENV, SamConfig +from samcli.lib.config.samconfig import DEFAULT_ENV, SamConfig class GuidedConfig: @@ -20,9 +20,10 @@ def get_config_ctx(self, config_file=None): ctx = click.get_current_context() samconfig_dir = getattr(ctx, "samconfig_dir", None) + config_dir = samconfig_dir if samconfig_dir else SamConfig.config_dir(template_file_path=self.template_file) samconfig = SamConfig( - config_dir=samconfig_dir if samconfig_dir else SamConfig.config_dir(template_file_path=self.template_file), - filename=config_file or DEFAULT_CONFIG_FILE_NAME, + config_dir=config_dir, + filename=config_file or SamConfig.get_default_file(config_dir=config_dir), ) return ctx, samconfig