From 5b4ffda68977bb84c18321b8432965b8f48d1348 Mon Sep 17 00:00:00 2001 From: Steven Elliott Date: Wed, 10 Sep 2025 23:15:19 +0000 Subject: [PATCH 1/5] Adding ability to take in a properly formatted JSON string and set the FIREWHEEL config. --- docs/source/cli/commands.rst | 13 ++++- docs/source/cli/helper_docs.rst | 11 ++-- src/firewheel/cli/configure_firewheel.py | 66 ++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 5 deletions(-) diff --git a/docs/source/cli/commands.rst b/docs/source/cli/commands.rst index 611ceea8..0f67bb5a 100644 --- a/docs/source/cli/commands.rst +++ b/docs/source/cli/commands.rst @@ -46,6 +46,7 @@ Edit the FIREWHEEL configuration with a text editor. The user must set either th environment variable or use the provided flag to override these environment variables. options: + -e EDITOR, --editor EDITOR Use the specified text editor. @@ -67,6 +68,7 @@ positional arguments: command: ``firewheel config get logging.level``. options: + -a, --all Get the entire FIREWHEEL configuration. @@ -90,13 +92,21 @@ positional arguments: config set ^^^^^^^^^^ -usage: firewheel config set (-f FILE | -s SETTING [VALUE ...]) +usage: firewheel config set (-f FILE | -j JSON | -s SETTING [VALUE ...]) Set a FIREWHEEL configuration. options: -f FILE, --file FILE Add config from a file. + + -j JSON, --json JSON Pass in a JSON string that can set/replace a subset of the configuration. + This should include the top-level config key as well as any sub-keys. + Any keys or sub-keys not present will not be impacted. + For example, to change the value for the config key ``logging.level``, you + can use the command: + ``firewheel config set -j '{"logging":{"level":"INFO"}}'``. + -s SETTING [VALUE ...], --single SETTING [VALUE ...] Set (or create) a particular configuration value. Nested settings can be used with a period separating them. For example, to change @@ -356,3 +366,4 @@ Example: $ firewheel version 2.6.0 + diff --git a/docs/source/cli/helper_docs.rst b/docs/source/cli/helper_docs.rst index e17ee176..9552d176 100644 --- a/docs/source/cli/helper_docs.rst +++ b/docs/source/cli/helper_docs.rst @@ -725,16 +725,18 @@ The repository should be an existing directory on the filesystem. The path may be specified absolute or relative. If the path does not exist, an error message is printed. -Some Model Components may provide an additional install script called ``INSTALL`` which can be executed to perform other setup steps (e.g. installing an extra python package or downloading an external VM resource). -INSTALL scripts can be can be any executable file type as defined by a `shebang `_ line. +Some Model Components may provide an additional installation details which can be executed to perform other setup steps (e.g. installing an extra python package or downloading an external VM resource). +This takes the form of either a ``INSTALL`` directory with a ``vars.yml`` and a ``tasks.yml`` that are Ansible tasks which can be executed. +Alternatively, it can be a single ``INSTALL`` script that can be can be any executable file type as defined by a `shebang `_ line. .. warning:: - The execution of Model Component ``INSTALL`` scripts can be a **DANGEROUS** operation. Please ensure that you **fully trust** the repository developer prior to executing these scripts. + The execution of Model Component ``INSTALL`` scripts can be a **DANGEROUS** operation. + Please ensure that you **fully trust** the repository developer prior to executing these scripts. .. seealso:: - See :ref:`mc_install` for more information on INSTALL scripts. + See :ref:`mc_install` for more information on ``INSTALL`` scripts. When installing a Model Component, users will have a variety of choices to select: @@ -1509,3 +1511,4 @@ Example ``firewheel vm resume --all`` + diff --git a/src/firewheel/cli/configure_firewheel.py b/src/firewheel/cli/configure_firewheel.py index 2b49d6f0..ff18959f 100644 --- a/src/firewheel/cli/configure_firewheel.py +++ b/src/firewheel/cli/configure_firewheel.py @@ -1,5 +1,6 @@ import os import cmd +import json import pprint import argparse import operator @@ -142,6 +143,24 @@ def do_reset(self, args: str = "") -> None: fw_config = Config(config_path=cmd_args.config_path) fw_config.generate_config_from_defaults() + def _argparse_check_json_type(self, json_string): + """ + Parse a JSON string into a Python dictionary. + + Args: + json_string (str): A string representation of a JSON object. + + Returns: + dict: The parsed JSON object as a Python dictionary. + + Raises: + argparse.ArgumentTypeError: If the input string is not a valid JSON. + """ + try: + return json.loads(json_string.replace("'", "")) + except json.decoder.JSONDecodeError as exc: + raise argparse.ArgumentTypeError(f"Invalid JSON string: {json_string}\n\n") from exc + def define_set_parser(self) -> argparse.ArgumentParser: """Create an :py:class:`argparse.ArgumentParser` for :ref:`command_config_set`. @@ -162,6 +181,19 @@ def define_set_parser(self) -> argparse.ArgumentParser: type=argparse.FileType("r"), help="Add config from a file.", ) + group.add_argument( + "-j", + "--json", + type=self._argparse_check_json_type, + help=( + "Pass in a JSON string that can set/replace a subset of the configuration.\n" + "This should include the top-level config key as well as any sub-keys.\n" + "Any keys or sub-keys not present will not be impacted.\n" + "For example, to change the value for the config key ``logging.level``, you\n" + "can use the command:\n" + '``firewheel config set -j \'{"logging":{"level":"INFO"}}\'``.' + ), + ) group.add_argument( "-s", "--single", @@ -177,6 +209,29 @@ def define_set_parser(self) -> argparse.ArgumentParser: ) return parser + def _update_nested_dict(self, original: dict, updates: dict) -> dict: + """ + This function recursively updates the original dictionary with values + from the updates dictionary. If a key in the updates dictionary is a + nested dictionary, it will update the corresponding key in the original + dictionary without removing any existing keys that are not specified in updates. + + Args: + original (dict): The original dictionary to be updated. + updates (dict): A dictionary containing the updates to apply. + + Returns: + dict: The updated original dictionary. + """ + for key, value in updates.items(): + if isinstance(value, dict) and key in original: + # If the key exists and the value is a dictionary, recurse + self._update_nested_dict(original[key], value) + else: + # Update or add the key in the original dictionary + original[key] = value + return original + def do_set(self, args: str) -> None: # noqa: DOC502 """Enable a user to set a particular FIREWHEEL configuration option. @@ -210,6 +265,15 @@ def do_set(self, args: str) -> None: # noqa: DOC502 fw_config.resolve_set(key, value) fw_config.write() + if cmd_args.json is not None: + value = cmd_args.json + self.log.debug("Setting the FIREWHEEL config value for `%s`", value) + fw_config = Config(writable=True) + curr_config = fw_config.get_config() + future_config = self._update_nested_dict(curr_config, value) + fw_config.set_config(future_config) + fw_config.write() + def define_get_parser(self) -> argparse.ArgumentParser: """Create an :py:class:`argparse.ArgumentParser` for :ref:`command_config_get`. @@ -310,6 +374,8 @@ def get_docs(self) -> str: for num, line in enumerate(help_list): if line.startswith(" -") and help_list[num + 1].startswith(" -"): clean_text += line + "\n" + elif line.startswith(" -"): + clean_text += "\n" + line else: clean_text += line clean_text += "\n" From 5ebe0151b52b1c2d56b6c8c25eac40815e78b628 Mon Sep 17 00:00:00 2001 From: Steven Elliott Date: Tue, 11 Nov 2025 14:06:02 +0000 Subject: [PATCH 2/5] Adding some consistency to the documentation --- docs/source/cli/commands.rst | 11 +++++++++-- docs/source/cli/helper_docs.rst | 4 ++-- src/firewheel/cli/helpers/repository/install | 4 ++-- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/docs/source/cli/commands.rst b/docs/source/cli/commands.rst index 0f67bb5a..56e159f5 100644 --- a/docs/source/cli/commands.rst +++ b/docs/source/cli/commands.rst @@ -42,8 +42,7 @@ config edit usage: firewheel config edit [-e EDITOR] -Edit the FIREWHEEL configuration with a text editor. The user must set either the VISUAL or EDITOR -environment variable or use the provided flag to override these environment variables. +Edit the FIREWHEEL configuration with a text editor. The user must set either the VISUAL or EDITOR environment variable or use the provided flag to override these environment variables. options: @@ -73,6 +72,14 @@ options: +.. _command_config_path: + +config path +^^^^^^^^^^^ + +Prints the path to the current FIREWHEEL configuration. + + .. _command_config_reset: config reset diff --git a/docs/source/cli/helper_docs.rst b/docs/source/cli/helper_docs.rst index 9552d176..68bcb6b2 100644 --- a/docs/source/cli/helper_docs.rst +++ b/docs/source/cli/helper_docs.rst @@ -725,8 +725,8 @@ The repository should be an existing directory on the filesystem. The path may be specified absolute or relative. If the path does not exist, an error message is printed. -Some Model Components may provide an additional installation details which can be executed to perform other setup steps (e.g. installing an extra python package or downloading an external VM resource). -This takes the form of either a ``INSTALL`` directory with a ``vars.yml`` and a ``tasks.yml`` that are Ansible tasks which can be executed. +Some Model Components may provide additional installation details which can be executed to perform other setup steps (e.g., installing an extra Python package or downloading an external VM resource). +This takes the form of either an ``INSTALL`` directory with a ``vars.yml`` and a ``tasks.yml`` that are Ansible tasks which can be executed. Alternatively, it can be a single ``INSTALL`` script that can be can be any executable file type as defined by a `shebang `_ line. .. warning:: diff --git a/src/firewheel/cli/helpers/repository/install b/src/firewheel/cli/helpers/repository/install index 42cef6fe..f8f80518 100644 --- a/src/firewheel/cli/helpers/repository/install +++ b/src/firewheel/cli/helpers/repository/install @@ -7,8 +7,8 @@ The repository should be an existing directory on the filesystem. The path may be specified absolute or relative. If the path does not exist, an error message is printed. -Some Model Components may provide an additional installation details which can be executed to perform other setup steps (e.g. installing an extra python package or downloading an external VM resource). -This takes the form of either a ``INSTALL`` directory with a ``vars.yml`` and a ``tasks.yml`` that are Ansible tasks which can be executed. +Some Model Components may provide additional installation details which can be executed to perform other setup steps (e.g., installing an extra Python package or downloading an external VM resource). +This takes the form of either an ``INSTALL`` directory with a ``vars.yml`` and a ``tasks.yml`` that are Ansible tasks which can be executed. Alternatively, it can be a single ``INSTALL`` script that can be can be any executable file type as defined by a `shebang `_ line. .. warning:: From 40e903f250e15c397115416581bf6e26ffefb351 Mon Sep 17 00:00:00 2001 From: Steven Elliott <6461548+sdelliot@users.noreply.github.com> Date: Tue, 11 Nov 2025 09:07:35 -0500 Subject: [PATCH 3/5] Update src/firewheel/cli/configure_firewheel.py Co-authored-by: Mitch Negus <21086604+mitchnegus@users.noreply.github.com> Signed-off-by: Steven Elliott <6461548+sdelliot@users.noreply.github.com> --- src/firewheel/cli/configure_firewheel.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/firewheel/cli/configure_firewheel.py b/src/firewheel/cli/configure_firewheel.py index dfbf55bb..a1fad7e9 100644 --- a/src/firewheel/cli/configure_firewheel.py +++ b/src/firewheel/cli/configure_firewheel.py @@ -213,7 +213,7 @@ def define_set_parser(self) -> argparse.ArgumentParser: def _update_nested_dict(self, original: dict, updates: dict) -> dict: """ This function recursively updates the original dictionary with values - from the updates dictionary. If a key in the updates dictionary is a + from the updates dictionary. If a key in the ``updates`` dictionary is a nested dictionary, it will update the corresponding key in the original dictionary without removing any existing keys that are not specified in updates. From 48afb0ba396a96b3835d91683efe2f69ce7638e1 Mon Sep 17 00:00:00 2001 From: Steven Elliott Date: Tue, 11 Nov 2025 14:22:40 +0000 Subject: [PATCH 4/5] Removing the string replacement --- src/firewheel/cli/configure_firewheel.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/firewheel/cli/configure_firewheel.py b/src/firewheel/cli/configure_firewheel.py index a1fad7e9..6e138c06 100644 --- a/src/firewheel/cli/configure_firewheel.py +++ b/src/firewheel/cli/configure_firewheel.py @@ -158,7 +158,7 @@ def _argparse_check_json_type(self, json_string): argparse.ArgumentTypeError: If the input string is not a valid JSON. """ try: - return json.loads(json_string.replace("'", "")) + return json.loads(json_string) except json.decoder.JSONDecodeError as exc: raise argparse.ArgumentTypeError(f"Invalid JSON string: {json_string}\n\n") from exc From 8d600077a3000fa8015d6bcd817e18ddf09a93b8 Mon Sep 17 00:00:00 2001 From: Steven Elliott Date: Tue, 11 Nov 2025 11:43:20 -0500 Subject: [PATCH 5/5] Adding unittests for new json functionality --- src/firewheel/cli/configure_firewheel.py | 9 +- .../tests/unit/cli/test_cli_configure.py | 124 +++++++++++++++++- 2 files changed, 129 insertions(+), 4 deletions(-) diff --git a/src/firewheel/cli/configure_firewheel.py b/src/firewheel/cli/configure_firewheel.py index 6e138c06..3eb1ffd3 100644 --- a/src/firewheel/cli/configure_firewheel.py +++ b/src/firewheel/cli/configure_firewheel.py @@ -225,9 +225,12 @@ def _update_nested_dict(self, original: dict, updates: dict) -> dict: dict: The updated original dictionary. """ for key, value in updates.items(): - if isinstance(value, dict) and key in original: - # If the key exists and the value is a dictionary, recurse - self._update_nested_dict(original[key], value) + if isinstance(value, dict): + if key in original and isinstance(original[key], dict): + # If the key exists and both values are dictionaries, recurse + self._update_nested_dict(original[key], value) + else: + original[key] = value else: # Update or add the key in the original dictionary original[key] = value diff --git a/src/firewheel/tests/unit/cli/test_cli_configure.py b/src/firewheel/tests/unit/cli/test_cli_configure.py index a1c28961..3baba260 100644 --- a/src/firewheel/tests/unit/cli/test_cli_configure.py +++ b/src/firewheel/tests/unit/cli/test_cli_configure.py @@ -1,6 +1,7 @@ import io import os import copy +import argparse import tempfile import unittest import unittest.mock @@ -190,7 +191,7 @@ def test_do_edit_param_invalid(self, mock_stdout): msg = "Error: Failed to open FIREWHEEL configuration with" self.assertIn(msg, mock_stdout.getvalue()) - @unittest.mock.patch.dict(os.environ, {'EDITOR': '', 'VISUAL': ''}) + @unittest.mock.patch.dict(os.environ, {"EDITOR": "", "VISUAL": ""}) @unittest.mock.patch("sys.stdout", new_callable=io.StringIO) def test_do_edit_none(self, mock_stdout): args = "" @@ -245,3 +246,124 @@ def test_help_help(self, mock_stdout): msg = "Prints help for the different sub-commands" self.assertIn(msg, mock_stdout.getvalue()) + + def test_valid_json(self): + json_string = '{"key": "value", "number": 123}' + expected = {"key": "value", "number": 123} + result = self.cli._argparse_check_json_type(json_string) + self.assertEqual(result, expected) + + def test_empty_json(self): + json_string = "{}" + expected = {} + result = self.cli._argparse_check_json_type(json_string) + self.assertEqual(result, expected) + + def test_invalid_json(self): + invalid_json_cases = [ + '{"key": "value", "number": 123', # Missing brace + '{"key": "value", "number": "123" "extra": "value"}', # Missing comma + '{"key": "value", "number": [1, 2, 3,}', # Trailing comma + '{key: "value"}', # Missing quotes + '["value1", "value2",]', # Trailing comma + "{'key': 'value'}", # Single quotes + ] + + for json_string in invalid_json_cases: + with self.subTest(json_string=json_string): + with self.assertRaises(argparse.ArgumentTypeError) as context: + self.cli._argparse_check_json_type(json_string) + self.assertIn("Invalid JSON string", str(context.exception)) + + def test_json_with_nested_structure(self): + json_string = '{"outer": {"inner": "value"}}' + expected = {"outer": {"inner": "value"}} + result = self.cli._argparse_check_json_type(json_string) + self.assertEqual(result, expected) + + def test_json_with_array(self): + json_string = '{"array": [1, 2, 3]}' + expected = {"array": [1, 2, 3]} + result = self.cli._argparse_check_json_type(json_string) + self.assertEqual(result, expected) + + @unittest.mock.patch("firewheel.cli.configure_firewheel.Config") + def test_do_set_with_json(self, mock_config_cls): + mock_config_cls().get_config.return_value = {"key": "value"} + mock_config_cls().set_config = unittest.mock.Mock() + mock_config_cls().write = unittest.mock.Mock() + + json_input = '{"key": "new_value", "nested": {"inner_key": "inner_value"}}' + args = f"--json '{json_input}'" + + self.cli.do_set(args) + + expected_config = {"key": "new_value", "nested": {"inner_key": "inner_value"}} + mock_config_cls().set_config.assert_called_once_with(expected_config) + mock_config_cls().write.assert_called_once() + + @unittest.mock.patch("firewheel.cli.configure_firewheel.Config") + def test_do_set_with_invalid_json(self, mock_config_cls): + json_input = '{"key": "value", "nested": {"inner_key": "inner_value"' + args = f'--json "{json_input}"' + + with self.assertRaises(SystemExit): + self.cli.do_set(args) + + @unittest.mock.patch("firewheel.cli.configure_firewheel.Config") + def test_do_set_with_empty_json(self, mock_config_cls): + mock_config_cls().get_config.return_value = {"key": "value"} + mock_config_cls().set_config = unittest.mock.Mock() + mock_config_cls().write = unittest.mock.Mock() + + json_input = "{}" + args = f'--json "{json_input}"' # Create a string that simulates command-line input + + self.cli.do_set(args) # Pass the string instead of a Mock + + expected_config = {"key": "value"} # No changes should be made + mock_config_cls().set_config.assert_called_once_with(expected_config) + mock_config_cls().write.assert_called_once() + + def test_update_existing_key(self): + original = {"key": "value", "nested": {"inner_key": "inner_value"}} + updates = {"key": "new_value"} + expected = {"key": "new_value", "nested": {"inner_key": "inner_value"}} + result = self.cli._update_nested_dict(original, updates) + self.assertEqual(result, expected) + + def test_update_nested_key(self): + original = {"nested": {"inner_key": "inner_value"}} + updates = {"nested": {"inner_key": "new_inner_value"}} + expected = {"nested": {"inner_key": "new_inner_value"}} + result = self.cli._update_nested_dict(original, updates) + self.assertEqual(result, expected) + + def test_add_new_key(self): + original = {"key": "value"} + updates = {"new_key": "new_value"} + expected = {"key": "value", "new_key": "new_value"} + result = self.cli._update_nested_dict(original, updates) + self.assertEqual(result, expected) + + def test_update_with_non_nested_key(self): + original = {"key": "value"} + updates = {"key": {"sub_key": "sub_value"}} + expected = {"key": {"sub_key": "sub_value"}} + result = self.cli._update_nested_dict(original, updates) + self.assertEqual(result, expected) + + def test_update_with_nested_and_non_nested_keys(self): + original = {"key": "value", "nested": {"inner_key": "inner_value"}} + updates = { + "key": "new_value", + "nested": {"inner_key": "new_inner_value"}, + "new_key": "new_value", + } + expected = { + "key": "new_value", + "nested": {"inner_key": "new_inner_value"}, + "new_key": "new_value", + } + result = self.cli._update_nested_dict(original, updates) + self.assertEqual(result, expected)