From b591ad6bb0c9d3d538d8fb066d8e4da312852405 Mon Sep 17 00:00:00 2001 From: Affe Null Date: Mon, 8 Jan 2024 15:30:33 +0100 Subject: [PATCH] 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. --- src/cobbler_tftp/cli.py | 49 +++++++------------ src/cobbler_tftp/settings/__init__.py | 19 ++++--- .../application_settings/test_settings.py | 8 +-- 3 files changed, 32 insertions(+), 44 deletions(-) diff --git a/src/cobbler_tftp/cli.py b/src/cobbler_tftp/cli.py index 38e9763..420aa30 100644 --- a/src/cobbler_tftp/cli.py +++ b/src/cobbler_tftp/cli.py @@ -17,14 +17,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 +37,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 +60,22 @@ 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, enable_automigration, config, settings): """ 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 - ) - else: - click.echo("Cobbler-tftp will be running as a daemon in the background.") + 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, daemon, enable_automigration, settings + ) + print(application_settings) @cli.command() @@ -103,7 +92,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()