diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 61ee6f2ad..c00209dc5 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -6,11 +6,18 @@ ## Upgrading - +- The `ConfigManagingActor` now takes multiple configuration files as input, and the argument was renamed from `config_file` to `config_files`. If you are using this actor, please update your code. For example: + + ```python + # Old + actor = ConfigManagingActor(config_file="config.toml") + # New + actor = ConfigManagingActor(config_files=["config.toml"]) + ``` ## New Features - +- The `ConfigManagingActor` can now take multiple configuration files as input, allowing to override default configurations with custom configurations. ## Bug Fixes diff --git a/src/frequenz/sdk/config/_config_managing.py b/src/frequenz/sdk/config/_config_managing.py index 8d97fcef4..30ac708d3 100644 --- a/src/frequenz/sdk/config/_config_managing.py +++ b/src/frequenz/sdk/config/_config_managing.py @@ -7,6 +7,7 @@ import pathlib import tomllib from collections import abc +from collections.abc import Mapping, MutableMapping from datetime import timedelta from typing import Any, assert_never @@ -19,21 +20,59 @@ class ConfigManagingActor(Actor): - """An actor that monitors a TOML configuration file for updates. - - When the file is updated, the new configuration is sent, as a [`dict`][], to the - `output` sender. - - When the actor is started, if a configuration file already exists, then it will be - read and sent to the `output` sender before the actor starts monitoring the file - for updates. This way users can rely on the actor to do the initial configuration - reading too. + """An actor that monitors a TOML configuration files for updates. + + When the actor is started the configuration files will be read and sent to the + output sender. Then the actor will start monitoring the files for updates. If any + file is updated, all the configuration files will be re-read and sent to the output + sender. + + If no configuration file could be read, the actor will raise an exception. + + The configuration files are read in the order of the paths, so the last path will + override the configuration set by the previous paths. Dict keys will be merged + recursively, but other objects (like lists) will be replaced by the value in the + last path. + + Example: + If `config1.toml` contains: + + ```toml + var1 = [1, 2] + var2 = 2 + [section] + var3 = [1, 3] + ``` + + And `config2.toml` contains: + + ```toml + var2 = "hello" # Can override with a different type too + var3 = 4 + [section] + var3 = 5 + var4 = 5 + ``` + + Then the final configuration will be: + + ```py + { + "var1": [1, 2], + "var2": "hello", + "var3": 4, + "section": { + "var3": 5, + "var4": 5, + }, + } + ``` """ # pylint: disable-next=too-many-arguments def __init__( self, - config_path: pathlib.Path | str, + config_paths: abc.Sequence[pathlib.Path | str], output: Sender[abc.Mapping[str, Any]], event_types: abc.Set[EventType] = frozenset(EventType), *, @@ -44,7 +83,11 @@ def __init__( """Initialize this instance. Args: - config_path: The path to the TOML file with the configuration. + config_paths: The paths to the TOML files with the configuration. Order + matters, as the configuration will be read and updated in the order + of the paths, so the last path will override the configuration set by + the previous paths. Dict keys will be merged recursively, but other + objects (like lists) will be replaced by the value in the last path. output: The sender to send the configuration to. event_types: The set of event types to monitor. name: The name of the actor. If `None`, `str(id(self))` will @@ -54,11 +97,14 @@ def __init__( polling is enabled. """ super().__init__(name=name) - self._config_path: pathlib.Path = ( - config_path - if isinstance(config_path, pathlib.Path) - else pathlib.Path(config_path) - ) + self._config_paths: list[pathlib.Path] = [ + ( + config_path + if isinstance(config_path, pathlib.Path) + else pathlib.Path(config_path) + ) + for config_path in config_paths + ] self._output: Sender[abc.Mapping[str, Any]] = output self._event_types: abc.Set[EventType] = event_types self._force_polling: bool = force_polling @@ -73,12 +119,22 @@ def _read_config(self) -> abc.Mapping[str, Any]: Raises: ValueError: If config file cannot be read. """ - try: - with self._config_path.open("rb") as toml_file: - return tomllib.load(toml_file) - except ValueError as err: - _logger.error("%s: Can't read config file, err: %s", self, err) - raise + error_count = 0 + config: dict[str, Any] = {} + + for config_path in self._config_paths: + try: + with config_path.open("rb") as toml_file: + data = tomllib.load(toml_file) + config = _recursive_update(config, data) + except ValueError as err: + _logger.error("%s: Can't read config file, err: %s", self, err) + error_count += 1 + + if error_count == len(self._config_paths): + raise ValueError(f"{self}: Can't read any of the config files") + + return config async def send_config(self) -> None: """Send the configuration to the output sender.""" @@ -94,11 +150,13 @@ async def _run(self) -> None: """ await self.send_config() + parent_paths = {p.parent for p in self._config_paths} + # FileWatcher can't watch for non-existing files, so we need to watch for the - # parent directory instead just in case a configuration file doesn't exist yet + # parent directories instead just in case a configuration file doesn't exist yet # or it is deleted and recreated again. file_watcher = FileWatcher( - paths=[self._config_path.parent], + paths=list(parent_paths), event_types=self._event_types, force_polling=self._force_polling, polling_interval=self._polling_interval, @@ -106,9 +164,10 @@ async def _run(self) -> None: try: async for event in file_watcher: - # Since we are watching the whole parent directory, we need to make sure - # we only react to events related to the configuration file. - if not event.path.samefile(self._config_path): + # Since we are watching the whole parent directories, we need to make + # sure we only react to events related to the configuration files we + # are interested in. + if not any(event.path.samefile(p) for p in self._config_paths): continue match event.type: @@ -116,23 +175,47 @@ async def _run(self) -> None: _logger.info( "%s: The configuration file %s was created, sending new config...", self, - self._config_path, + event.path, ) await self.send_config() case EventType.MODIFY: _logger.info( "%s: The configuration file %s was modified, sending update...", self, - self._config_path, + event.path, ) await self.send_config() case EventType.DELETE: _logger.info( "%s: The configuration file %s was deleted, ignoring...", self, - self._config_path, + event.path, ) case _: assert_never(event.type) finally: del file_watcher + + +def _recursive_update( + target: dict[str, Any], overrides: Mapping[str, Any] +) -> dict[str, Any]: + """Recursively updates dictionary d1 with values from dictionary d2. + + Args: + target: The original dictionary to be updated. + overrides: The dictionary with updates. + + Returns: + The updated dictionary. + """ + for key, value in overrides.items(): + if ( + key in target + and isinstance(target[key], MutableMapping) + and isinstance(value, MutableMapping) + ): + _recursive_update(target[key], value) + else: + target[key] = value + return target diff --git a/tests/actor/test_config_manager.py b/tests/actor/test_config_manager.py deleted file mode 100644 index 542081b78..000000000 --- a/tests/actor/test_config_manager.py +++ /dev/null @@ -1,118 +0,0 @@ -# License: MIT -# Copyright © 2022 Frequenz Energy-as-a-Service GmbH - -"""Test for ConfigManager.""" -import os -import pathlib -from collections.abc import Mapping -from typing import Any - -import pytest -from frequenz.channels import Broadcast - -from frequenz.sdk.config import ConfigManagingActor - - -class Item: - """Test item.""" - - item_id: int - name: str - - -def create_content(number: int) -> str: - """Create content to be written to a config file.""" - return f""" - logging_lvl = "ERROR" - var1 = "0" - var2 = "{number}" - """ - - -class TestActorConfigManager: - """Test for ConfigManager.""" - - conf_path = "sdk/config.toml" - conf_content = """ - logging_lvl = 'DEBUG' - var1 = "1" - var_int = "5" - var_float = "3.14" - var_bool = "true" - list_int = "[1,2,3]" - list_float = "[1,2.0,3.5]" - var_off = "off" - list_non_strict_bool = '["false", "0", "true", "1"]' - item_data = '[{"item_id": 1, "name": "My Item"}]' - dict_str_int = '{"a": 1, "b": 2, "c": 3}' - var_none = 'null' - """ - - @pytest.fixture() - def config_file(self, tmp_path: pathlib.Path) -> pathlib.Path: - """Create a test config file.""" - file_path = tmp_path / TestActorConfigManager.conf_path - if not file_path.exists(): - file_path.parent.mkdir() - file_path.touch() - file_path.write_text(TestActorConfigManager.conf_content) - return file_path - - async def test_update(self, config_file: pathlib.Path) -> None: - """Test ConfigManager. - - Check if: - - - the initial content of the content file is correct - - the config file modifications are picked up and the new content is correct - """ - config_channel: Broadcast[Mapping[str, Any]] = Broadcast( - name="Config Channel", resend_latest=True - ) - config_receiver = config_channel.new_receiver() - - async with ConfigManagingActor( - config_file, config_channel.new_sender(), force_polling=False - ): - config = await config_receiver.receive() - assert config is not None - assert config.get("logging_lvl") == "DEBUG" - assert config.get("var1") == "1" - assert config.get("var2") is None - assert config.get("var3") is None - - number = 5 - config_file.write_text(create_content(number=number)) - - config = await config_receiver.receive() - assert config is not None - assert config.get("logging_lvl") == "ERROR" - assert config.get("var1") == "0" - assert config.get("var2") == str(number) - assert config.get("var3") is None - assert config_file.read_text() == create_content(number=number) - - async def test_update_relative_path(self, config_file: pathlib.Path) -> None: - """Test ConfigManagingActor with a relative path.""" - config_channel: Broadcast[Mapping[str, Any]] = Broadcast( - name="Config Channel", resend_latest=True - ) - config_receiver = config_channel.new_receiver() - - current_dir = pathlib.Path.cwd() - relative_path = os.path.relpath(config_file, current_dir) - - async with ConfigManagingActor( - relative_path, config_channel.new_sender(), force_polling=False - ): - config = await config_receiver.receive() - assert config is not None - assert config.get("var2") is None - - number = 8 - config_file.write_text(create_content(number=number)) - - config = await config_receiver.receive() - assert config is not None - assert config.get("var2") == str(number) - assert config_file.read_text() == create_content(number=number) diff --git a/tests/config/test_config_manager.py b/tests/config/test_config_manager.py new file mode 100644 index 000000000..2ef1c68d9 --- /dev/null +++ b/tests/config/test_config_manager.py @@ -0,0 +1,356 @@ +# License: MIT +# Copyright © 2022 Frequenz Energy-as-a-Service GmbH + +"""Test for ConfigManager.""" +import os +import pathlib +from collections import defaultdict +from collections.abc import Mapping, MutableMapping +from dataclasses import dataclass +from typing import Any + +import pytest +from frequenz.channels import Broadcast + +from frequenz.sdk.config import ConfigManagingActor +from frequenz.sdk.config._config_managing import _recursive_update + + +class Item: + """Test item.""" + + item_id: int + name: str + + +def create_content(number: int) -> str: + """Create content to be written to a config file.""" + return f""" + logging_lvl = "ERROR" + var1 = "0" + var2 = "{number}" + """ + + +class TestActorConfigManager: + """Test for ConfigManager.""" + + conf_path = "sdk/config.toml" + conf_content = """ + logging_lvl = 'DEBUG' + var1 = "1" + var_int = "5" + var_float = "3.14" + var_bool = "true" + list_int = "[1,2,3]" + list_float = "[1,2.0,3.5]" + var_off = "off" + list_non_strict_bool = ["false", "0", "true", "1"] + item_data = '[{"item_id": 1, "name": "My Item"}]' + var_none = 'null' + [dict_str_int] + a = 1 + b = 2 + c = 3 + """ + + @pytest.fixture() + def config_file(self, tmp_path: pathlib.Path) -> pathlib.Path: + """Create a test config file.""" + file_path = tmp_path / TestActorConfigManager.conf_path + if not file_path.exists(): + file_path.parent.mkdir() + file_path.touch() + file_path.write_text(TestActorConfigManager.conf_content) + return file_path + + async def test_update(self, config_file: pathlib.Path) -> None: + """Test ConfigManager. + + Check if: + + - the initial content of the content file is correct + - the config file modifications are picked up and the new content is correct + """ + config_channel: Broadcast[Mapping[str, Any]] = Broadcast( + name="Config Channel", resend_latest=True + ) + config_receiver = config_channel.new_receiver() + + async with ConfigManagingActor( + [config_file], config_channel.new_sender(), force_polling=False + ): + config = await config_receiver.receive() + assert config is not None + assert config.get("logging_lvl") == "DEBUG" + assert config.get("var1") == "1" + assert config.get("var2") is None + assert config.get("var3") is None + + number = 5 + config_file.write_text(create_content(number=number)) + + config = await config_receiver.receive() + assert config is not None + assert config.get("logging_lvl") == "ERROR" + assert config.get("var1") == "0" + assert config.get("var2") == str(number) + assert config.get("var3") is None + assert config_file.read_text() == create_content(number=number) + + async def test_update_relative_path(self, config_file: pathlib.Path) -> None: + """Test ConfigManagingActor with a relative path.""" + config_channel: Broadcast[Mapping[str, Any]] = Broadcast( + name="Config Channel", resend_latest=True + ) + config_receiver = config_channel.new_receiver() + + current_dir = pathlib.Path.cwd() + relative_path = os.path.relpath(config_file, current_dir) + + async with ConfigManagingActor( + [relative_path], config_channel.new_sender(), force_polling=False + ): + config = await config_receiver.receive() + assert config is not None + assert config.get("var2") is None + + number = 8 + config_file.write_text(create_content(number=number)) + + config = await config_receiver.receive() + assert config is not None + assert config.get("var2") == str(number) + assert config_file.read_text() == create_content(number=number) + + async def test_update_multiple_files(self, config_file: pathlib.Path) -> None: + """Test ConfigManagingActor with multiple files.""" + config_channel: Broadcast[Mapping[str, Any]] = Broadcast( + name="Config Channel", resend_latest=True + ) + config_receiver = config_channel.new_receiver() + + config_file2 = config_file.parent / "config2.toml" + config_file2.write_text( + """ + logging_lvl = 'ERROR' + var1 = "0" + var2 = "15" + var_float = "3.14" + list_int = "[1,3]" + list_float = "[1,2.0,3.8,12.3]" + var_off = "off" + item_data = '[{"item_id": 1, "name": "My Item 2"}]' + [dict_str_int] + a = 1 + b = 2 + c = 4 + d = 3 + """ + ) + + async with ConfigManagingActor( + [config_file, config_file2], + config_channel.new_sender(), + force_polling=False, + ): + config = await config_receiver.receive() + assert config is not None + # Both config files are read and merged, and config_file2 overrides + # the variables present in config_file. + assert config == { + "logging_lvl": "ERROR", + "var1": "0", + "var2": "15", + "var_int": "5", + "var_float": "3.14", + "var_bool": "true", + "list_int": "[1,3]", + "list_float": "[1,2.0,3.8,12.3]", + "var_off": "off", + "list_non_strict_bool": ["false", "0", "true", "1"], + "item_data": '[{"item_id": 1, "name": "My Item 2"}]', + "dict_str_int": {"a": 1, "b": 2, "c": 4, "d": 3}, + "var_none": "null", + } + + # We overwrite config_file with just two variables + config_file.write_text( + """ + logging_lvl = 'INFO' + list_non_strict_bool = ["false", "0", "true"] + """ + ) + + config = await config_receiver.receive() + assert config is not None + # The config no longer contains variables that were only present in + # config_file. config_file2 still takes precedence, so the result remains + # unchanged except for the removal of variables unique to the old + # config_file. + assert config == { + "logging_lvl": "ERROR", + "var1": "0", + "var2": "15", + "var_float": "3.14", + "list_int": "[1,3]", + "list_float": "[1,2.0,3.8,12.3]", + "var_off": "off", + "item_data": '[{"item_id": 1, "name": "My Item 2"}]', + "list_non_strict_bool": ["false", "0", "true"], + "dict_str_int": {"a": 1, "b": 2, "c": 4, "d": 3}, + } + + # Now we only update logging_lvl in config_file2, it still takes precedence + config_file2.write_text( + """ + logging_lvl = 'DEBUG' + var1 = "0" + var2 = "15" + var_float = "3.14" + list_int = "[1,3]" + list_float = "[1,2.0,3.8,12.3]" + var_off = "off" + item_data = '[{"item_id": 1, "name": "My Item 2"}]' + [dict_str_int] + a = 1 + b = 2 + c = 4 + d = 3 + """ + ) + + config = await config_receiver.receive() + assert config is not None + assert config == { + "logging_lvl": "DEBUG", + "var1": "0", + "var2": "15", + "var_float": "3.14", + "list_int": "[1,3]", + "list_float": "[1,2.0,3.8,12.3]", + "var_off": "off", + "item_data": '[{"item_id": 1, "name": "My Item 2"}]', + "list_non_strict_bool": ["false", "0", "true"], + "dict_str_int": {"a": 1, "b": 2, "c": 4, "d": 3}, + } + + # Now add one variable to config_file not present in config_file2 and remove + # a bunch of variables from config_file2 too, and update a few + config_file.write_text( + """ + logging_lvl = 'INFO' + var10 = "10" + """ + ) + config_file2.write_text( + """ + logging_lvl = 'DEBUG' + var1 = "3" + var_off = "on" + [dict_str_int] + a = 1 + b = 2 + c = 4 + """ + ) + + config = await config_receiver.receive() + assert config is not None + assert config == { + "logging_lvl": "DEBUG", + "var1": "3", + "var10": "10", + "var_off": "on", + "dict_str_int": {"a": 1, "b": 2, "c": 4}, + } + + +@dataclass(frozen=True, kw_only=True) +class RecursiveUpdateTestCase: + """A test case for the recursive_update function.""" + + title: str + d1: dict[str, Any] + d2: MutableMapping[str, Any] + expected: dict[str, Any] + + +# Define all test cases as instances of TestCase +recursive_update_test_cases = [ + RecursiveUpdateTestCase( + title="Basic Update", + d1={"a": 1, "b": 2, "c": {"d": 3, "e": 4}}, + d2={"b": 5, "h": 10, "c": {"d": 30}}, + expected={"a": 1, "b": 5, "h": 10, "c": {"d": 30, "e": 4}}, + ), + RecursiveUpdateTestCase( + title="Nested Update", + d1={"a": {"b": 1, "c": {"d": 2}}, "e": 3}, + d2={"a": {"c": {"d": 20, "f": 4}, "g": 5}, "h": 6}, + expected={"a": {"b": 1, "c": {"d": 20, "f": 4}, "g": 5}, "e": 3, "h": 6}, + ), + RecursiveUpdateTestCase( + title="Non-Dict Overwrite", + d1={"a": {"b": 1}, "c": 2}, + d2={"a": 10, "c": {"d": 3}}, + expected={"a": 10, "c": {"d": 3}}, + ), + RecursiveUpdateTestCase( + title="Empty d2", + d1={"a": 1, "b": {"c": 2}}, + d2={}, + expected={"a": 1, "b": {"c": 2}}, + ), + RecursiveUpdateTestCase( + title="Empty d1", + d1={}, + d2={"a": 1, "b": {"c": 2}}, + expected={"a": 1, "b": {"c": 2}}, + ), + RecursiveUpdateTestCase( + title="Non-Mapping Values Overwrite", + d1={"a": {"b": [1, 2, 3]}, "c": "hello"}, + d2={"a": {"b": [4, 5]}, "c": "world"}, + expected={"a": {"b": [4, 5]}, "c": "world"}, + ), + RecursiveUpdateTestCase( + title="Multiple Data Types", + d1={"int": 1, "float": 1.0, "str": "one", "list": [1, 2, 3], "dict": {"a": 1}}, + d2={"int": 2, "float": 2.0, "str": "two", "list": [4, 5], "dict": {"b": 2}}, + expected={ + "int": 2, + "float": 2.0, + "str": "two", + "list": [4, 5], + "dict": {"a": 1, "b": 2}, + }, + ), + RecursiveUpdateTestCase( + title="Defaultdict Handling", + d1=defaultdict(dict, {"a": {"b": 1}, "c": 2}), + d2={"a": {"c": 3}, "d": 4}, + expected={"a": {"b": 1, "c": 3}, "c": 2, "d": 4}, + ), + # Additional Test Cases + RecursiveUpdateTestCase( + title="Overwrite with Nested Non-Mapping", + d1={"x": {"y": {"z": 1}}}, + d2={"x": {"y": 2}}, + expected={"x": {"y": 2}}, + ), + RecursiveUpdateTestCase( + title="Overwrite with Nested Mapping", + d1={"x": {"y": 2}}, + d2={"x": {"y": {"z": 3}}}, + expected={"x": {"y": {"z": 3}}}, + ), +] + + +@pytest.mark.parametrize( + "test_case", recursive_update_test_cases, ids=lambda tc: tc.title +) +def test_recursive_update(test_case: RecursiveUpdateTestCase) -> None: + """Test the recursive_update function with various input dictionaries.""" + assert _recursive_update(test_case.d1, test_case.d2) == test_case.expected