From 9fac81e2fb0beaf9b7191ec6fb3e4d61f328fca1 Mon Sep 17 00:00:00 2001 From: Affe Null Date: Mon, 8 Jan 2024 15:30:33 +0100 Subject: [PATCH 1/2] Use consistent types for CLI configuration Add enable/disable options for configurable booleans and pass all of them to build_settings and load_cli_options and fix the type errors in these functions. Convert the configuration file path to a Path object as expected by the settings module. --- src/cobbler_tftp/cli.py | 59 ++++++++++--------- src/cobbler_tftp/settings/__init__.py | 19 +++--- .../application_settings/test_settings.py | 8 +-- 3 files changed, 43 insertions(+), 43 deletions(-) diff --git a/src/cobbler_tftp/cli.py b/src/cobbler_tftp/cli.py index 38e9763..e54fb2e 100644 --- a/src/cobbler_tftp/cli.py +++ b/src/cobbler_tftp/cli.py @@ -2,6 +2,9 @@ Cobbler-tftp will be managable as a command-line service. """ +from pathlib import Path +from typing import List, Optional + import click import yaml @@ -17,14 +20,6 @@ except importlib_metadata.PackageNotFoundError: __version__ = "unknown (not installed)" -with open( - "src/cobbler_tftp/settings/data/settings.yml", "r", encoding="utf-8" -) as stream: - try: - SETTINGS = yaml.safe_load(stream) - except yaml.YAMLError as exc: - print(exc) - _context_settings = dict(help_option_names=["-h", "--help"]) @@ -45,17 +40,17 @@ def cli(ctx): @cli.command() @click.option( - "--no-daemon", - "-dd", + "--daemon/--no-daemon", + "-d/-dd", is_flag=True, - default=not SETTINGS["is_daemon"], # type: ignore - help="Stop cobbler-tftp from running as daemon.", + default=None, + help="Force cobbler-tftp to run as daemon or not.", ) @click.option( - "--enable-automigration", + "--enable-automigration/--disable-automigration", is_flag=True, - default=SETTINGS["auto_migrate_settings"], # type: ignore - help="Enable auto migration of settings.", + default=None, + help="Enable or disable auto migration of settings.", ) @click.option( "--config", "-c", type=click.Path(), help="Set location of configuration file." @@ -68,25 +63,31 @@ def cli(ctx): ..<...>.=.\n Your settings must use single quotes. If a single quote appears within a value it must be escaped.""", ) -def start(no_daemon: bool, enable_automigration, config, settings): +def start( + daemon: Optional[bool], + enable_automigration: Optional[bool], + config: Optional[str], + settings: List[str], +): """ Start the cobbler-tftp server. """ click.echo(cli.__doc__) click.echo("Initializing Cobbler-tftp server...") - if no_daemon: - click.echo("'--no-daemon' flag set. Server running in the foreground...") - settings_factory: SettingsFactory = SettingsFactory() - # settings_file = SettingsFactory.load_config_file(settings_factory, config) - # environment_variables = SettingsFactory.load_env_variables(settings_factory) - # cli_arguments = SettingsFactory.load_cli_options( - # settings_factory, daemon, enable_automigration, settings - # ) - application_settings = SettingsFactory.build_settings( - settings_factory, config, settings - ) + settings_factory: SettingsFactory = SettingsFactory() + # settings_file = SettingsFactory.load_config_file(settings_factory, config) + # environment_variables = SettingsFactory.load_env_variables(settings_factory) + # cli_arguments = SettingsFactory.load_cli_options( + # settings_factory, daemon, enable_automigration, settings + # ) + if config is None: + config_path = None else: - click.echo("Cobbler-tftp will be running as a daemon in the background.") + config_path = Path(config) + application_settings = SettingsFactory.build_settings( + settings_factory, config_path, daemon, enable_automigration, settings + ) + print(application_settings) @cli.command() @@ -103,7 +104,7 @@ def print_default_config(): Print the default application parameters. """ settings_factory: SettingsFactory = SettingsFactory() - click.echo(settings_factory.build_settings(None, [])) + click.echo(settings_factory.build_settings(None)) @cli.command() diff --git a/src/cobbler_tftp/settings/__init__.py b/src/cobbler_tftp/settings/__init__.py index 441065e..a68e3d9 100644 --- a/src/cobbler_tftp/settings/__init__.py +++ b/src/cobbler_tftp/settings/__init__.py @@ -93,7 +93,11 @@ def __init__(self) -> None: self._settings_dict: SettingsDict = {} def build_settings( - self, config_path: Optional[Path], cli_arguments: List[str] + self, + config_path: Optional[Path], + daemon: Optional[bool] = None, + enable_automigration: Optional[bool] = None, + cli_arguments: List[str] = [], ) -> Settings: """ Build new Settings object using parameters from all sources. @@ -110,7 +114,7 @@ def build_settings( self.load_env_variables() # Load CLI options - self.load_cli_options(cli_arguments) + self.load_cli_options(daemon, enable_automigration, cli_arguments) if not migrations.validate(self._settings_dict): raise ValueError( @@ -219,7 +223,7 @@ def load_cli_options( self, daemon: Optional[bool] = None, enable_automigration: Optional[bool] = None, - settings: Optional[Dict[str, Union[str, Path]]] = None, + settings: List[str] = [], ) -> SettingsDict: """ Get parameters and flags from CLI. @@ -234,16 +238,11 @@ def load_cli_options( :return _settings_dict: Settings dictionary. """ - if not daemon and not enable_automigration and not settings: - return self._settings_dict - - if daemon: + if daemon is not None: self._settings_dict["is_daemon"] = daemon - if enable_automigration: + if enable_automigration is not None: self._settings_dict["auto_migrate_settings"] = enable_automigration - if settings is None: - raise ValueError for setting in settings: option_list = setting.split("=", 1) diff --git a/tests/unittests/application_settings/test_settings.py b/tests/unittests/application_settings/test_settings.py index e6d4ecd..fafb2a4 100644 --- a/tests/unittests/application_settings/test_settings.py +++ b/tests/unittests/application_settings/test_settings.py @@ -27,7 +27,7 @@ def test_build_settings_with_default_config_file( config file """ # Call the build_settings method with None as the config_path argument - settings = settings_factory.build_settings(None, None) + settings = settings_factory.build_settings(None) # Assert that the expected values are set in the Settings object assert isinstance(settings, Settings) @@ -42,7 +42,7 @@ def test_build_settings_with_valid_config_file( settings_factory: SettingsFactory, mocker ): valid_file_path = Path("tests/test_data/valid_config.yml") - settings = settings_factory.build_settings(valid_file_path, None) + settings = settings_factory.build_settings(valid_file_path) assert isinstance(settings, Settings) assert settings.auto_migrate_settings is True @@ -60,7 +60,7 @@ def test_build_settings_with_invalid_config_file( path = Path("tests/test_data/invalid_config.yml") with pytest.raises(ValueError) as exc: - settings_factory.build_settings(path, None) + settings_factory.build_settings(path) assert """Validation Error: Configuration Parameters could not be validated!\n This may be due to an invalid configuration file or path.""" in str( @@ -83,7 +83,7 @@ def test_build_settings_with_missing_config_file( expected_message = f"Warning: No configuration file found at {path}! Using default configuration file...\n" # Act - settings = settings_factory.build_settings(path, None) + settings = settings_factory.build_settings(path) # Assert captured_message = capsys.readouterr() From 0d13a78f758516e58c389b99738b6410e2a717cf Mon Sep 17 00:00:00 2001 From: Affe Null Date: Mon, 8 Jan 2024 15:34:50 +0100 Subject: [PATCH 2/2] Ignore some importlib-resources type errors During static type checking, always install the old version of importlib-resources that works with Python 3.6 because the contents() and path() functions were removed in the latest version but the alternative to them only works with newer Python versions. Ignore types for files().joinpath() because this old version does not provide type definitions for its return type. --- setup.cfg | 2 +- src/cobbler_tftp/settings/__init__.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/setup.cfg b/setup.cfg index 9a81295..d49515b 100644 --- a/setup.cfg +++ b/setup.cfg @@ -45,7 +45,7 @@ tests_require = lint_requires = importlib-metadata>=3.10.1 - importlib-resources>=5.4.0 + importlib-resources==5.4.0 pre-commit>=2.0.1 black>=22.1.0 pyright>=0.0.13 diff --git a/src/cobbler_tftp/settings/__init__.py b/src/cobbler_tftp/settings/__init__.py index a68e3d9..c96cbbc 100644 --- a/src/cobbler_tftp/settings/__init__.py +++ b/src/cobbler_tftp/settings/__init__.py @@ -169,7 +169,7 @@ def load_config_file(self, config_path: Union[Path, None]) -> SettingsDict: config_file_content = ( files("cobbler_tftp.settings.data") .joinpath("settings.yml") - .read_text(encoding="UTF-8") + .read_text(encoding="UTF-8") # type: ignore ) self._settings_dict = yaml.safe_load(config_file_content) except yaml.YAMLError: @@ -177,7 +177,7 @@ def load_config_file(self, config_path: Union[Path, None]) -> SettingsDict: elif config_path and Path.exists(config_path): try: config_file_content = ( - files(config_import_path).joinpath(config_file).read_text("utf-8") + files(config_import_path).joinpath(config_file).read_text("utf-8") # type: ignore ) self._settings_dict = yaml.safe_load(config_file_content) except yaml.YAMLError: