From 7d42526f9ae2bbf2999b73ca382ddae6a45be6c8 Mon Sep 17 00:00:00 2001 From: Alexandra Bara Date: Thu, 16 Apr 2026 09:30:04 -0500 Subject: [PATCH 1/3] added new API --- nodescraper/cli/__init__.py | 6 ++- nodescraper/cli/cli.py | 36 +++++++++++++++++ nodescraper/cli/embed.py | 39 ++++++++++++++++-- test/unit/cli/test_cli_embed_api.py | 63 +++++++++++++++++++++++++++++ 4 files changed, 140 insertions(+), 4 deletions(-) create mode 100644 test/unit/cli/test_cli_embed_api.py diff --git a/nodescraper/cli/__init__.py b/nodescraper/cli/__init__.py index f5e396b2..44e9b02b 100644 --- a/nodescraper/cli/__init__.py +++ b/nodescraper/cli/__init__.py @@ -24,8 +24,9 @@ # ############################################################################### +from .cli import get_cli_top_level_subcommands from .cli import main as cli_entry -from .embed import run_main_return_code +from .embed import CLI_TOP_LEVEL_SUBCOMMANDS, run_cli_return_code, run_main_return_code from .invocation import ( PluginRunInvocation, get_plugin_run_invocation, @@ -34,7 +35,10 @@ ) __all__ = [ + "CLI_TOP_LEVEL_SUBCOMMANDS", "cli_entry", + "get_cli_top_level_subcommands", + "run_cli_return_code", "run_main_return_code", "PluginRunInvocation", "get_plugin_run_invocation", diff --git a/nodescraper/cli/cli.py b/nodescraper/cli/cli.py index 7447e02a..f3f754d9 100644 --- a/nodescraper/cli/cli.py +++ b/nodescraper/cli/cli.py @@ -25,6 +25,7 @@ ############################################################################### import argparse import datetime +import functools import json import logging import os @@ -334,6 +335,41 @@ def build_parser( return parser, plugin_subparser_map +def _top_level_subcommand_names(root: argparse.ArgumentParser) -> tuple[str, ...]: + """Return ``dest=subcmd`` subparser names from the root CLI parser. + + Args: + root: Parser returned by :func:`build_parser`. + + Returns: + Tuple of top-level subcommand strings. + """ + for action in root._actions: + if isinstance(action, argparse._SubParsersAction) and action.dest == "subcmd": + return tuple(action.choices.keys()) + raise RuntimeError("nodescraper CLI root parser has no subcmd subparsers") + + +@functools.lru_cache(maxsize=1) +def get_cli_top_level_subcommands() -> tuple[str, ...]: + """Return top-level subcommand names from a parser built like :func:`main` (cached). + + Returns: + Tuple of ``subcmd`` subparser names; call ``cache_clear()`` if registries change in-process. + """ + plugin_reg = PluginRegistry() + config_reg = ConfigRegistry() + config_reg.configs["AllPlugins"] = PluginConfig( + name="AllPlugins", + desc="Run all registered plugins with default arguments", + global_args={}, + plugins={name: {} for name in plugin_reg.plugins}, + result_collators={}, + ) + parser, _plugin_subparser_map = build_parser(plugin_reg, config_reg) + return _top_level_subcommand_names(parser) + + def setup_logger( log_level: str = "INFO", log_path: Optional[str] = None, diff --git a/nodescraper/cli/embed.py b/nodescraper/cli/embed.py index aa5ad082..60d94515 100644 --- a/nodescraper/cli/embed.py +++ b/nodescraper/cli/embed.py @@ -23,14 +23,39 @@ # SOFTWARE. # ############################################################################### -"""In-process CLI entry without adding new argparse flags.""" from __future__ import annotations import argparse from typing import Optional -__all__ = ["run_main_return_code"] +from nodescraper.cli.cli import get_cli_top_level_subcommands + +CLI_TOP_LEVEL_SUBCOMMANDS = get_cli_top_level_subcommands() + +__all__ = [ + "CLI_TOP_LEVEL_SUBCOMMANDS", + "get_cli_top_level_subcommands", + "run_cli_return_code", + "run_main_return_code", +] + + +def run_cli_return_code( + argv: list[str], + *, + host_cli_args: Optional[argparse.Namespace] = None, +) -> int: + """Run nodescraper in-process; same behavior as :func:`run_main_return_code`. + + Args: + argv: Tokens after the program name. + host_cli_args: Optional host namespace forwarded to :func:`nodescraper.cli.cli.main`. + + Returns: + Integer exit code (``SystemExit`` is mapped, not raised). + """ + return run_main_return_code(argv, host_cli_args=host_cli_args) def run_main_return_code( @@ -38,7 +63,15 @@ def run_main_return_code( *, host_cli_args: Optional[argparse.Namespace] = None, ) -> int: - """Runs the nodescraper main entrypoint and maps SystemExit to an integer return code.""" + """Run :func:`nodescraper.cli.cli.main` and map ``SystemExit`` to an exit code. + + Args: + arg_input: Tokens after the program name. + host_cli_args: Optional host namespace for embedded runs. + + Returns: + Integer exit code. + """ from nodescraper.cli.cli import main try: diff --git a/test/unit/cli/test_cli_embed_api.py b/test/unit/cli/test_cli_embed_api.py new file mode 100644 index 00000000..db44f6cf --- /dev/null +++ b/test/unit/cli/test_cli_embed_api.py @@ -0,0 +1,63 @@ +############################################################################### +# +# MIT License +# +# Copyright (C) 2026 Advanced Micro Devices, Inc. +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in all +# copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. +# +############################################################################### + +from __future__ import annotations + +import pytest + +from nodescraper.cli.cli import get_cli_top_level_subcommands +from nodescraper.cli.embed import ( + CLI_TOP_LEVEL_SUBCOMMANDS, + run_cli_return_code, + run_main_return_code, +) + + +def test_get_cli_top_level_subcommands_matches_argparse_subparsers() -> None: + subs = get_cli_top_level_subcommands() + assert isinstance(subs, tuple) + assert "run-plugins" in subs + assert "summary" in subs + assert all(isinstance(s, str) for s in subs) + + +def test_cli_top_level_subcommands_lazy_alias_matches_getter() -> None: + assert CLI_TOP_LEVEL_SUBCOMMANDS == get_cli_top_level_subcommands() + + +def test_run_cli_return_code_and_run_main_return_code_delegate( + monkeypatch: pytest.MonkeyPatch, +) -> None: + calls: list[list[str]] = [] + + def fake_main(arg_input: list[str], *, host_cli_args=None) -> None: + calls.append(list(arg_input)) + raise SystemExit(7) + + monkeypatch.setattr("nodescraper.cli.cli.main", fake_main) + assert run_cli_return_code(["describe", "plugin", "X"]) == 7 + assert run_main_return_code(["a", "b"]) == 7 + assert calls == [["describe", "plugin", "X"], ["a", "b"]] From 4117eadda5a3e0f3d14c0376a5cca003979bfb85 Mon Sep 17 00:00:00 2001 From: Alexandra Bara Date: Thu, 16 Apr 2026 11:39:06 -0500 Subject: [PATCH 2/3] adding --plugin-config= opt --- README.md | 22 ++++----- nodescraper/cli/cli.py | 16 +++++-- test/functional/test_fabrics_plugin.py | 8 ++-- test/functional/test_network_plugin.py | 11 ++--- test/functional/test_nic_plugin.py | 8 ++-- test/functional/test_pcie_plugin.py | 14 ++---- test/functional/test_plugin_configs.py | 48 ++++++++----------- test/functional/test_rdma_plugin.py | 8 ++-- .../test_reference_config_workflow.py | 4 +- test/functional/test_sys_settings_plugin.py | 6 +-- test/unit/cli/test_cli_no_console_stdout.py | 2 +- 11 files changed, 67 insertions(+), 80 deletions(-) diff --git a/README.md b/README.md index eda8dea4..cf4647a7 100644 --- a/README.md +++ b/README.md @@ -78,7 +78,7 @@ usage: cli.py [-h] [--version] [--sys-name STRING] [--sys-location {LOCAL,REMOTE}] [--sys-interaction-level {PASSIVE,INTERACTIVE,DISRUPTIVE}] [--sys-sku STRING] [--sys-platform STRING] - [--plugin-configs [STRING ...]] [--system-config STRING] + [--plugin-configs LIST] [--system-config STRING] [--connection-config STRING] [--log-path STRING] [--log-level {CRITICAL,FATAL,ERROR,WARN,WARNING,INFO,DEBUG,NOTSET}] [--no-console-log] [--gen-reference-config] [--skip-sudo] @@ -112,10 +112,9 @@ options: --sys-sku STRING Manually specify SKU of system (default: None) --sys-platform STRING Specify system platform (default: None) - --plugin-configs [STRING ...] - built-in config names or paths to plugin config JSONs. - Available built-in configs: NodeStatus, AllPlugins - (default: None) + --plugin-configs LIST + Comma-separated built-in names and/or plugin config JSON + paths. Built-in: NodeStatus, AllPlugins (default: None) --system-config STRING Path to system config json (default: None) --connection-config STRING @@ -348,7 +347,7 @@ You can extend the built-in error detection with custom regex patterns. Create a Save this to `dmesg_custom_config.json` and run: ```sh -node-scraper --plugin-configs dmesg_custom_config.json run-plugins DmesgPlugin +node-scraper --plugin-configs=dmesg_custom_config.json run-plugins DmesgPlugin ``` #### **'compare-runs' subcommand** @@ -539,8 +538,9 @@ Built-in configs include **NodeStatus** (a subset of plugins) and **AllPlugins** registered plugin with default arguments—useful for generating a reference config from the full system). **NodeStatus plus additional plugins** — built-in configs merge with plugins named after `run-plugins`. -Use **`--plugin-configs=`** (equals form): with a space -after `--plugin-configs`. See below for examples: +Values are comma-separated; pass as **`--plugin-configs=…`** or **`--plugin-configs` …** (same as other +optional flags), e.g. `--plugin-configs=NodeStatus,/path/extra.json`. +Examples: ```sh node-scraper --plugin-configs=NodeStatus run-plugins PciePlugin ``` @@ -551,7 +551,7 @@ node-scraper --log-path ./logs --plugin-configs=NodeStatus run-plugins PciePlugi Using a JSON file: ```sh -node-scraper --plugin-configs plugin_config.json +node-scraper --plugin-configs=plugin_config.json ``` Here is an example of a comprehensive plugin config that specifies analyzer args for each plugin: ```json @@ -613,7 +613,7 @@ data. **Run all registered plugins (AllPlugins config):** ```sh -node-scraper --plugin-config AllPlugins +node-scraper --plugin-configs=AllPlugins ``` @@ -647,7 +647,7 @@ This will generate the following config: ``` This config can later be used on a different platform for comparison, using the steps at #2: ```sh -node-scraper --plugin-configs reference_config.json +node-scraper --plugin-configs=reference_config.json ``` diff --git a/nodescraper/cli/cli.py b/nodescraper/cli/cli.py index f3f754d9..26a30aa3 100644 --- a/nodescraper/cli/cli.py +++ b/nodescraper/cli/cli.py @@ -64,6 +64,11 @@ from nodescraper.pluginregistry import PluginRegistry +def _parse_plugin_configs_csv(value: str) -> list[str]: + """Split a comma-separated ``--plugin-configs`` value into names/paths.""" + return [p.strip() for p in value.split(",") if p.strip()] + + def build_parser( plugin_reg: PluginRegistry, config_reg: ConfigRegistry, @@ -126,10 +131,13 @@ def build_parser( parser.add_argument( "--plugin-configs", - type=str, - nargs="*", - help=f"built-in config names or paths to plugin config JSONs.\nAvailable built-in configs: {', '.join(config_reg.configs.keys())}", - metavar=META_VAR_MAP[str], + type=_parse_plugin_configs_csv, + default=None, + help=( + "Comma-separated built-in names and/or plugin config JSON paths " + f"(e.g. --plugin-configs=NodeStatus,/path/c.json). Built-ins: {', '.join(config_reg.configs.keys())}" + ), + metavar="LIST", ) parser.add_argument( diff --git a/test/functional/test_fabrics_plugin.py b/test/functional/test_fabrics_plugin.py index a8f0cd62..a334d966 100644 --- a/test/functional/test_fabrics_plugin.py +++ b/test/functional/test_fabrics_plugin.py @@ -48,7 +48,7 @@ def test_fabrics_plugin_with_basic_config(run_cli_command, fabrics_config_file, log_path = str(tmp_path / "logs_fabrics_basic") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(fabrics_config_file)], check=False + ["--log-path", log_path, f"--plugin-configs={fabrics_config_file}"], check=False ) assert result.returncode in [0, 1, 2] @@ -76,8 +76,7 @@ def test_fabrics_plugin_with_passive_interaction(run_cli_command, fabrics_config log_path, "--sys-interaction-level", "PASSIVE", - "--plugin-configs", - str(fabrics_config_file), + f"--plugin-configs={fabrics_config_file}", ], check=False, ) @@ -95,8 +94,7 @@ def test_fabrics_plugin_skip_sudo(run_cli_command, fabrics_config_file, tmp_path "--log-path", log_path, "--skip-sudo", - "--plugin-configs", - str(fabrics_config_file), + f"--plugin-configs={fabrics_config_file}", ], check=False, ) diff --git a/test/functional/test_network_plugin.py b/test/functional/test_network_plugin.py index 5759ad3b..d4ff71fc 100644 --- a/test/functional/test_network_plugin.py +++ b/test/functional/test_network_plugin.py @@ -48,7 +48,7 @@ def test_network_plugin_with_basic_config(run_cli_command, network_config_file, log_path = str(tmp_path / "logs_network_basic") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(network_config_file)], check=False + ["--log-path", log_path, f"--plugin-configs={network_config_file}"], check=False ) assert result.returncode in [0, 1, 2] @@ -76,8 +76,7 @@ def test_network_plugin_with_passive_interaction(run_cli_command, network_config log_path, "--sys-interaction-level", "PASSIVE", - "--plugin-configs", - str(network_config_file), + f"--plugin-configs={network_config_file}", ], check=False, ) @@ -95,8 +94,7 @@ def test_network_plugin_skip_sudo(run_cli_command, network_config_file, tmp_path "--log-path", log_path, "--skip-sudo", - "--plugin-configs", - str(network_config_file), + f"--plugin-configs={network_config_file}", ], check=False, ) @@ -113,8 +111,7 @@ def test_network_plugin_with_url(run_cli_command, network_config_file, tmp_path) [ "--log-path", log_path, - "--plugin-configs", - str(network_config_file), + f"--plugin-configs={network_config_file}", ], check=False, ) diff --git a/test/functional/test_nic_plugin.py b/test/functional/test_nic_plugin.py index ed9d28f2..b862845e 100644 --- a/test/functional/test_nic_plugin.py +++ b/test/functional/test_nic_plugin.py @@ -60,8 +60,7 @@ def test_nic_plugin_with_full_analyzer_args_config( [ "--log-path", log_path, - "--plugin-configs", - str(nic_plugin_config_full_analyzer_args), + f"--plugin-configs={nic_plugin_config_full_analyzer_args}", ], check=False, ) @@ -82,7 +81,7 @@ def test_nic_plugin_with_minimal_config(run_cli_command, nic_plugin_config_minim log_path = str(tmp_path / "logs_nic_minimal") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(nic_plugin_config_minimal)], + ["--log-path", log_path, f"--plugin-configs={nic_plugin_config_minimal}"], check=False, ) @@ -122,8 +121,7 @@ def test_nic_plugin_full_config_validates_analysis_args( [ "--log-path", log_path, - "--plugin-configs", - str(nic_plugin_config_full_analyzer_args), + f"--plugin-configs={nic_plugin_config_full_analyzer_args}", ], check=False, ) diff --git a/test/functional/test_pcie_plugin.py b/test/functional/test_pcie_plugin.py index 9d6c70c9..63bc21cf 100644 --- a/test/functional/test_pcie_plugin.py +++ b/test/functional/test_pcie_plugin.py @@ -54,7 +54,7 @@ def test_pcie_plugin_with_basic_config(run_cli_command, pcie_config_file, tmp_pa log_path = str(tmp_path / "logs_pcie_basic") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(pcie_config_file)], check=False + ["--log-path", log_path, f"--plugin-configs={pcie_config_file}"], check=False ) assert result.returncode in [0, 1, 2] @@ -69,7 +69,7 @@ def test_pcie_plugin_with_advanced_config(run_cli_command, pcie_advanced_config_ log_path = str(tmp_path / "logs_pcie_advanced") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(pcie_advanced_config_file)], + ["--log-path", log_path, f"--plugin-configs={pcie_advanced_config_file}"], check=False, ) @@ -97,8 +97,7 @@ def test_pcie_plugin_with_passive_interaction(run_cli_command, pcie_config_file, log_path, "--sys-interaction-level", "PASSIVE", - "--plugin-configs", - str(pcie_config_file), + f"--plugin-configs={pcie_config_file}", ], check=False, ) @@ -116,8 +115,7 @@ def test_pcie_plugin_skip_sudo(run_cli_command, pcie_config_file, tmp_path): "--log-path", log_path, "--skip-sudo", - "--plugin-configs", - str(pcie_config_file), + f"--plugin-configs={pcie_config_file}", ], check=False, ) @@ -136,9 +134,7 @@ def test_pcie_plugin_combined_configs( [ "--log-path", log_path, - "--plugin-configs", - str(pcie_config_file), - str(pcie_advanced_config_file), + f"--plugin-configs={pcie_config_file},{pcie_advanced_config_file}", ], check=False, ) diff --git a/test/functional/test_plugin_configs.py b/test/functional/test_plugin_configs.py index 768e9e6e..90c68218 100644 --- a/test/functional/test_plugin_configs.py +++ b/test/functional/test_plugin_configs.py @@ -95,9 +95,7 @@ def invalid_plugin_config(tmp_path): def test_plugin_config_with_builtin_config(run_cli_command, tmp_path): """Test using a built-in config name.""" log_path = str(tmp_path / "logs_builtin") - result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", "NodeStatus"], check=False - ) + result = run_cli_command(["--log-path", log_path, "--plugin-configs=NodeStatus"], check=False) assert result.returncode in [0, 1, 2] output = result.stdout + result.stderr @@ -139,7 +137,7 @@ def test_individual_plugin_with_config_file( log_path = str(tmp_path / f"logs_{plugin_name.lower()}") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(config_file)], check=False + ["--log-path", log_path, f"--plugin-configs={config_file}"], check=False ) assert result.returncode in [0, 1, 2] @@ -153,7 +151,7 @@ def test_plugin_config_with_custom_json_file(run_cli_command, sample_plugin_conf """Test using a custom JSON config file path.""" log_path = str(tmp_path / "logs_custom") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", sample_plugin_config], check=False + ["--log-path", log_path, f"--plugin-configs={sample_plugin_config}"], check=False ) assert result.returncode in [0, 1, 2] @@ -168,13 +166,7 @@ def test_plugin_config_with_multiple_configs(run_cli_command, plugin_config_file os_config = str(plugin_config_files["OsPlugin"]) result = run_cli_command( - [ - "--log-path", - log_path, - "--plugin-configs", - bios_config, - os_config, - ], + ["--log-path", log_path, f"--plugin-configs={bios_config},{os_config}"], check=False, ) @@ -186,7 +178,7 @@ def test_plugin_config_with_multiple_configs(run_cli_command, plugin_config_file def test_plugin_config_with_nonexistent_file(run_cli_command, tmp_path): """Test that a nonexistent config file path fails gracefully.""" nonexistent_path = str(tmp_path / "nonexistent_config.json") - result = run_cli_command(["--plugin-configs", nonexistent_path], check=False) + result = run_cli_command([f"--plugin-configs={nonexistent_path}"], check=False) assert result.returncode != 0 output = (result.stdout + result.stderr).lower() @@ -195,7 +187,7 @@ def test_plugin_config_with_nonexistent_file(run_cli_command, tmp_path): def test_plugin_config_with_invalid_builtin_name(run_cli_command): """Test that an invalid built-in config name fails gracefully.""" - result = run_cli_command(["--plugin-configs", "NonExistentConfig"], check=False) + result = run_cli_command(["--plugin-configs=NonExistentConfig"], check=False) assert result.returncode != 0 output = (result.stdout + result.stderr).lower() @@ -204,7 +196,7 @@ def test_plugin_config_with_invalid_builtin_name(run_cli_command): def test_plugin_config_with_invalid_json(run_cli_command, invalid_plugin_config): """Test that an invalid JSON file fails gracefully.""" - result = run_cli_command(["--plugin-configs", invalid_plugin_config], check=False) + result = run_cli_command([f"--plugin-configs={invalid_plugin_config}"], check=False) assert result.returncode != 0 output = (result.stdout + result.stderr).lower() @@ -212,9 +204,9 @@ def test_plugin_config_with_invalid_json(run_cli_command, invalid_plugin_config) def test_plugin_config_empty_list(run_cli_command, tmp_path): - """Test --plugin-configs with no arguments (uses default config).""" + """Test omitting --plugin-configs (uses default config).""" log_path = str(tmp_path / "logs_empty") - result = run_cli_command(["--log-path", log_path, "--plugin-configs"], check=False) + result = run_cli_command(["--log-path", log_path], check=False) assert result.returncode in [0, 1, 2] output = result.stdout + result.stderr @@ -234,8 +226,7 @@ def test_plugin_config_with_system_interaction_level( log_path, "--sys-interaction-level", "PASSIVE", - "--plugin-configs", - config_file, + f"--plugin-configs={config_file}", ], check=False, ) @@ -254,8 +245,7 @@ def test_plugin_config_combined_with_run_plugins(run_cli_command, plugin_config_ [ "--log-path", log_path, - "--plugin-configs", - config_file, + f"--plugin-configs={config_file}", "run-plugins", "UptimePlugin", ], @@ -272,7 +262,9 @@ def test_plugin_config_verify_log_output(run_cli_command, plugin_config_files, t log_path = str(tmp_path / "logs_verify") config_file = str(plugin_config_files["OsPlugin"]) - result = run_cli_command(["--log-path", log_path, "--plugin-configs", config_file], check=False) + result = run_cli_command( + ["--log-path", log_path, f"--plugin-configs={config_file}"], check=False + ) log_dirs = [d for d in os.listdir(tmp_path) if d.startswith("logs_verify")] if result.returncode in [0, 1]: @@ -304,7 +296,7 @@ def test_dmesg_plugin_log_dmesg_data_false(run_cli_command, tmp_path): log_path = str(tmp_path / "logs_dmesg_no_log") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(config_file)], check=False + ["--log-path", log_path, f"--plugin-configs={config_file}"], check=False ) assert result.returncode in [0, 1, 2] @@ -331,7 +323,7 @@ def test_dmesg_plugin_log_dmesg_data_true(run_cli_command, tmp_path): log_path = str(tmp_path / "logs_dmesg_with_log") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(config_file)], check=False + ["--log-path", log_path, f"--plugin-configs={config_file}"], check=False ) if result.returncode in [0, 1]: @@ -388,7 +380,7 @@ def test_dmesg_plugin_with_custom_regex_in_config(run_cli_command, tmp_path): log_path = str(tmp_path / "logs_custom_regex") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(config_file)], check=False + ["--log-path", log_path, f"--plugin-configs={config_file}"], check=False ) # Check that command ran successfully @@ -448,7 +440,7 @@ def test_dmesg_plugin_with_event_collapsing_config(run_cli_command, tmp_path): log_path = str(tmp_path / "logs_collapse") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(config_file)], check=False + ["--log-path", log_path, f"--plugin-configs={config_file}"], check=False ) assert result.returncode in [0, 1, 2] @@ -521,7 +513,7 @@ def test_dmesg_plugin_with_custom_regex_and_collapsing(run_cli_command, tmp_path log_path = str(tmp_path / "logs_custom_collapse") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(config_file)], check=False + ["--log-path", log_path, f"--plugin-configs={config_file}"], check=False ) assert result.returncode in [0, 1, 2] @@ -579,7 +571,7 @@ def test_dmesg_plugin_different_collapse_intervals(run_cli_command, tmp_path): log_path_small = str(tmp_path / "logs_small_interval") result = run_cli_command( - ["--log-path", log_path_small, "--plugin-configs", str(config_file_small)], check=False + ["--log-path", log_path_small, f"--plugin-configs={config_file_small}"], check=False ) assert result.returncode in [0, 1, 2] diff --git a/test/functional/test_rdma_plugin.py b/test/functional/test_rdma_plugin.py index 862de3b8..31de828f 100644 --- a/test/functional/test_rdma_plugin.py +++ b/test/functional/test_rdma_plugin.py @@ -48,7 +48,7 @@ def test_rdma_plugin_with_basic_config(run_cli_command, rdma_config_file, tmp_pa log_path = str(tmp_path / "logs_rdma_basic") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(rdma_config_file)], check=False + ["--log-path", log_path, f"--plugin-configs={rdma_config_file}"], check=False ) assert result.returncode in [0, 1, 2] @@ -76,8 +76,7 @@ def test_rdma_plugin_with_passive_interaction(run_cli_command, rdma_config_file, log_path, "--sys-interaction-level", "PASSIVE", - "--plugin-configs", - str(rdma_config_file), + f"--plugin-configs={rdma_config_file}", ], check=False, ) @@ -95,8 +94,7 @@ def test_rdma_plugin_skip_sudo(run_cli_command, rdma_config_file, tmp_path): "--log-path", log_path, "--skip-sudo", - "--plugin-configs", - str(rdma_config_file), + f"--plugin-configs={rdma_config_file}", ], check=False, ) diff --git a/test/functional/test_reference_config_workflow.py b/test/functional/test_reference_config_workflow.py index 7b929d38..b9f1648d 100644 --- a/test/functional/test_reference_config_workflow.py +++ b/test/functional/test_reference_config_workflow.py @@ -155,7 +155,7 @@ def test_use_generated_reference_config(run_cli_command, tmp_path): assert reference_config_path.exists() use_result = run_cli_command( - ["--log-path", use_log_path, "--plugin-configs", str(reference_config_path)], + ["--log-path", use_log_path, f"--plugin-configs={reference_config_path}"], check=False, ) @@ -209,7 +209,7 @@ def test_full_workflow_all_plugins(run_cli_command, tmp_path, all_plugin_names): assert isinstance(plugin_config["analysis_args"], dict) use_result = run_cli_command( - ["--log-path", use_log_path, "--plugin-configs", str(reference_config_path)], + ["--log-path", use_log_path, f"--plugin-configs={reference_config_path}"], check=False, ) diff --git a/test/functional/test_sys_settings_plugin.py b/test/functional/test_sys_settings_plugin.py index 85b1bc03..03869184 100644 --- a/test/functional/test_sys_settings_plugin.py +++ b/test/functional/test_sys_settings_plugin.py @@ -54,7 +54,7 @@ def test_sys_settings_plugin_with_config_file(run_cli_command, sys_settings_conf log_path = str(tmp_path / "logs_sys_settings") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(sys_settings_config_file)], check=False + ["--log-path", log_path, f"--plugin-configs={sys_settings_config_file}"], check=False ) assert result.returncode in [0, 1, 2] @@ -85,7 +85,7 @@ def test_sys_settings_plugin_output_contains_plugin_result( log_path = str(tmp_path / "logs_sys_settings_result") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(sys_settings_config_file)], check=False + ["--log-path", log_path, f"--plugin-configs={sys_settings_config_file}"], check=False ) output = result.stdout + result.stderr @@ -99,7 +99,7 @@ def test_sys_settings_plugin_with_plugin_config_json(run_cli_command, plugin_con log_path = str(tmp_path / "logs_plugin_config") result = run_cli_command( - ["--log-path", log_path, "--plugin-configs", str(plugin_config_json)], check=False + ["--log-path", log_path, f"--plugin-configs={plugin_config_json}"], check=False ) assert result.returncode in [0, 1, 2] diff --git a/test/unit/cli/test_cli_no_console_stdout.py b/test/unit/cli/test_cli_no_console_stdout.py index 775bccdf..afa53d13 100644 --- a/test/unit/cli/test_cli_no_console_stdout.py +++ b/test/unit/cli/test_cli_no_console_stdout.py @@ -109,7 +109,7 @@ def test_run_plugins_empty_config_no_stdout(no_console_base, tmp_path): encoding="utf-8", ) _assert_main_leaves_stdout_empty( - no_console_base + ["run-plugins", "--plugin-configs", str(cfg)], + no_console_base + ["run-plugins", f"--plugin-configs={cfg}"], ) From 89727a4f3009481d8afb4d2e822f0bcda121c202 Mon Sep 17 00:00:00 2001 From: Alexandra Bara Date: Tue, 21 Apr 2026 09:55:26 -0500 Subject: [PATCH 3/3] updates --- nodescraper/cli/cli.py | 93 ++++++++++++------- nodescraper/cli/host_cli_embed.py | 84 +++++++++++++++++ .../cli/test_build_global_argument_parser.py | 18 ++++ test/unit/cli/test_host_cli_embed.py | 71 ++++++++++++++ test/unit/cli/test_plugin_configs_cli.py | 37 ++++++++ 5 files changed, 269 insertions(+), 34 deletions(-) create mode 100644 nodescraper/cli/host_cli_embed.py create mode 100644 test/unit/cli/test_build_global_argument_parser.py create mode 100644 test/unit/cli/test_host_cli_embed.py create mode 100644 test/unit/cli/test_plugin_configs_cli.py diff --git a/nodescraper/cli/cli.py b/nodescraper/cli/cli.py index 26a30aa3..054c2c5b 100644 --- a/nodescraper/cli/cli.py +++ b/nodescraper/cli/cli.py @@ -49,6 +49,10 @@ parse_gen_plugin_config, process_args, ) +from nodescraper.cli.host_cli_embed import ( + apply_host_cli_args_to_parsed_args, + merge_plugin_connection_config_from_host_ns, +) from nodescraper.cli.inputargtypes import ModelArgHandler, json_arg, log_path_arg from nodescraper.cli.invocation import run_plugin_queue_with_invocation from nodescraper.configregistry import ConfigRegistry @@ -69,24 +73,25 @@ def _parse_plugin_configs_csv(value: str) -> list[str]: return [p.strip() for p in value.split(",") if p.strip()] -def build_parser( - plugin_reg: PluginRegistry, - config_reg: ConfigRegistry, -) -> tuple[argparse.ArgumentParser, dict[str, tuple[argparse.ArgumentParser, dict]]]: - """Build an argument parser - - Args: - plugin_reg (PluginRegistry): registry of plugins - - Returns: - tuple[argparse.ArgumentParser, dict[str, tuple[argparse.ArgumentParser, dict]]]: tuple containing main - parser and subparsers for each plugin module - """ - parser = argparse.ArgumentParser( - description="node scraper CLI", - formatter_class=argparse.ArgumentDefaultsHelpFormatter, +def _config_registry_with_all_plugins(plugin_reg: PluginRegistry) -> ConfigRegistry: + """Synthetic ``AllPlugins`` config used for CLI help and :func:`build_global_argument_parser`.""" + config_reg = ConfigRegistry() + config_reg.configs["AllPlugins"] = PluginConfig( + name="AllPlugins", + desc="Run all registered plugins with default arguments", + global_args={}, + plugins={name: {} for name in plugin_reg.plugins}, + result_collators={}, ) + return config_reg + +def _add_cli_root_globals( + parser: argparse.ArgumentParser, + plugin_reg: PluginRegistry, + config_reg: ConfigRegistry, +) -> None: + """Register top-level flags before ``subcmd`` subparsers (shared with :func:`build_global_argument_parser`).""" parser.add_argument( "--version", action="version", @@ -193,6 +198,40 @@ def build_parser( help="Skip plugins that require sudo permissions", ) + +def build_global_argument_parser(*, add_help: bool = True) -> argparse.ArgumentParser: + """Globals only (no subcommands), for host CLIs such as amd-error-scraper ``error-scraper``.""" + plugin_reg = PluginRegistry() + config_reg = _config_registry_with_all_plugins(plugin_reg) + parser = argparse.ArgumentParser( + description="node scraper CLI (global options only)", + formatter_class=argparse.ArgumentDefaultsHelpFormatter, + add_help=add_help, + ) + _add_cli_root_globals(parser, plugin_reg, config_reg) + return parser + + +def build_parser( + plugin_reg: PluginRegistry, + config_reg: ConfigRegistry, +) -> tuple[argparse.ArgumentParser, dict[str, tuple[argparse.ArgumentParser, dict]]]: + """Build an argument parser + + Args: + plugin_reg (PluginRegistry): registry of plugins + + Returns: + tuple[argparse.ArgumentParser, dict[str, tuple[argparse.ArgumentParser, dict]]]: tuple containing main + parser and subparsers for each plugin module + """ + parser = argparse.ArgumentParser( + description="node scraper CLI", + formatter_class=argparse.ArgumentDefaultsHelpFormatter, + ) + + _add_cli_root_globals(parser, plugin_reg, config_reg) + subparsers = parser.add_subparsers(dest="subcmd", help="Subcommands") subparsers.default = "run-plugins" @@ -366,14 +405,7 @@ def get_cli_top_level_subcommands() -> tuple[str, ...]: Tuple of ``subcmd`` subparser names; call ``cache_clear()`` if registries change in-process. """ plugin_reg = PluginRegistry() - config_reg = ConfigRegistry() - config_reg.configs["AllPlugins"] = PluginConfig( - name="AllPlugins", - desc="Run all registered plugins with default arguments", - global_args={}, - plugins={name: {} for name in plugin_reg.plugins}, - result_collators={}, - ) + config_reg = _config_registry_with_all_plugins(plugin_reg) parser, _plugin_subparser_map = build_parser(plugin_reg, config_reg) return _top_level_subcommand_names(parser) @@ -441,16 +473,7 @@ def main( arg_input = sys.argv[1:] plugin_reg = PluginRegistry() - - config_reg = ConfigRegistry() - # Add synthetic "AllPlugins" config that includes every registered plugin - config_reg.configs["AllPlugins"] = PluginConfig( - name="AllPlugins", - desc="Run all registered plugins with default arguments", - global_args={}, - plugins={name: {} for name in plugin_reg.plugins}, - result_collators={}, - ) + config_reg = _config_registry_with_all_plugins(plugin_reg) parser, plugin_subparser_map = build_parser(plugin_reg, config_reg) try: @@ -459,6 +482,8 @@ def main( ) parsed_args = parser.parse_args(top_level_args) + apply_host_cli_args_to_parsed_args(parsed_args, host_cli_args) + merge_plugin_connection_config_from_host_ns(parsed_args, host_cli_args) system_info = get_system_info(parsed_args) sname = system_info.name.lower().replace("-", "_").replace(".", "_") timestamp = datetime.datetime.now().strftime("%Y_%m_%d-%I_%M_%S_%p") diff --git a/nodescraper/cli/host_cli_embed.py b/nodescraper/cli/host_cli_embed.py new file mode 100644 index 00000000..bffeb378 --- /dev/null +++ b/nodescraper/cli/host_cli_embed.py @@ -0,0 +1,84 @@ +############################################################################### +# +# MIT License +# +# Copyright (C) 2026 Advanced Micro Devices, Inc. +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in all +# copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. +# +############################################################################### + +from __future__ import annotations + +import argparse +from typing import Optional + +__all__ = [ + "apply_host_cli_args_to_parsed_args", + "merge_plugin_connection_config_from_host_ns", +] + + +def apply_host_cli_args_to_parsed_args( + parsed_args: argparse.Namespace, + host_ns: Optional[argparse.Namespace], +) -> None: + """Copy host profile fields from an embedding host onto parsed top-level args. + + Used when ``main(..., host_cli_args=...)`` is invoked (e.g. from the + error-scraper wrapper) so ``--connection-config`` profile data loaded by the + host is visible to :func:`get_system_info` and the rest of the CLI. + """ + if host_ns is None: + return + for attr in ( + "sys_name", + "sys_location", + "sys_sku", + "sys_platform", + "sys_interaction_level", + ): + if hasattr(host_ns, attr): + val = getattr(host_ns, attr) + if val is not None: + setattr(parsed_args, attr, val) + + +def merge_plugin_connection_config_from_host_ns( + parsed_args: argparse.Namespace, + host_ns: Optional[argparse.Namespace], +) -> None: + """Merge plugin ``connection_config`` from the host namespace into ``parsed_args``. + + The same key shape as ``--connection-config`` on the node-scraper argv: + connection manager class name (string) → args dict. When both the host + profile and the CLI supply ``connection_config``, **CLI** values win for + duplicate manager keys. + """ + if host_ns is None: + return + from_host = getattr(host_ns, "connection_config", None) + if not from_host: + return + if not isinstance(from_host, dict): + return + from_cli = getattr(parsed_args, "connection_config", None) or {} + if not isinstance(from_cli, dict): + from_cli = {} + parsed_args.connection_config = {**from_host, **from_cli} diff --git a/test/unit/cli/test_build_global_argument_parser.py b/test/unit/cli/test_build_global_argument_parser.py new file mode 100644 index 00000000..33489e9c --- /dev/null +++ b/test/unit/cli/test_build_global_argument_parser.py @@ -0,0 +1,18 @@ +# Copyright (C) 2025 Advanced Micro Devices, Inc. + +from __future__ import annotations + +from nodescraper.cli.cli import build_global_argument_parser + + +def test_build_global_argument_parser_leaves_subcommand_in_unknown() -> None: + p = build_global_argument_parser(add_help=False) + ns, rest = p.parse_known_args(["--sys-name", "sut.example", "run-plugins", "DmesgPlugin"]) + assert ns.sys_name == "sut.example" + assert rest == ["run-plugins", "DmesgPlugin"] + + +def test_build_global_argument_parser_has_no_subcmd_default() -> None: + p = build_global_argument_parser(add_help=False) + ns, _ = p.parse_known_args([]) + assert getattr(ns, "subcmd", None) is None diff --git a/test/unit/cli/test_host_cli_embed.py b/test/unit/cli/test_host_cli_embed.py new file mode 100644 index 00000000..232fc2ee --- /dev/null +++ b/test/unit/cli/test_host_cli_embed.py @@ -0,0 +1,71 @@ +############################################################################### +# +# MIT License +# +# Copyright (C) 2026 Advanced Micro Devices, Inc. +# +############################################################################### + +from __future__ import annotations + +import argparse + +from nodescraper.cli.host_cli_embed import ( + apply_host_cli_args_to_parsed_args, + merge_plugin_connection_config_from_host_ns, +) + + +def test_apply_host_cli_args_to_parsed_args_copies_sys_fields() -> None: + parsed = argparse.Namespace( + sys_name="client", + sys_location="LOCAL", + sys_sku=None, + sys_platform=None, + sys_interaction_level="INTERACTIVE", + ) + host = argparse.Namespace( + sys_name="sut.example.com", + sys_location="REMOTE", + sys_sku="MI450", + sys_platform="whitman", + sys_interaction_level="STANDARD", + ) + apply_host_cli_args_to_parsed_args(parsed, host) + assert parsed.sys_name == "sut.example.com" + assert parsed.sys_location == "REMOTE" + assert parsed.sys_sku == "MI450" + assert parsed.sys_platform == "whitman" + assert parsed.sys_interaction_level == "STANDARD" + + +def test_apply_host_cli_args_to_parsed_args_noop_without_host() -> None: + parsed = argparse.Namespace(sys_name="x") + apply_host_cli_args_to_parsed_args(parsed, None) + assert parsed.sys_name == "x" + + +def test_merge_plugin_connection_config_from_host_ns_host_first_cli_wins() -> None: + parsed = argparse.Namespace( + connection_config={"InBandConnectionManager": {"hostname": "cli-host"}} + ) + host = argparse.Namespace( + connection_config={ + "InBandConnectionManager": {"hostname": "host-host"}, + "RedfishConnectionManager": {"host": "10.0.0.1"}, + } + ) + merge_plugin_connection_config_from_host_ns(parsed, host) + assert parsed.connection_config["InBandConnectionManager"]["hostname"] == "cli-host" + assert parsed.connection_config["RedfishConnectionManager"]["host"] == "10.0.0.1" + + +def test_merge_plugin_connection_config_from_host_ns_host_only() -> None: + parsed = argparse.Namespace(connection_config=None) + host = argparse.Namespace( + connection_config={"InBandConnectionManager": {"hostname": "h", "username": "u"}} + ) + merge_plugin_connection_config_from_host_ns(parsed, host) + assert parsed.connection_config == { + "InBandConnectionManager": {"hostname": "h", "username": "u"} + } diff --git a/test/unit/cli/test_plugin_configs_cli.py b/test/unit/cli/test_plugin_configs_cli.py new file mode 100644 index 00000000..5c4af37b --- /dev/null +++ b/test/unit/cli/test_plugin_configs_cli.py @@ -0,0 +1,37 @@ +############################################################################### +# +# MIT License +# +# Copyright (C) 2026 Advanced Micro Devices, Inc. +# +############################################################################### + +from __future__ import annotations + +from nodescraper.cli.cli import build_parser +from nodescraper.configregistry import ConfigRegistry +from nodescraper.models import PluginConfig +from nodescraper.pluginregistry import PluginRegistry + + +def _parser(): + plugin_reg = PluginRegistry() + config_reg = ConfigRegistry() + config_reg.configs["AllPlugins"] = PluginConfig( + name="AllPlugins", + desc="Run all registered plugins with default arguments", + global_args={}, + plugins={name: {} for name in plugin_reg.plugins}, + result_collators={}, + ) + return build_parser(plugin_reg, config_reg)[0] + + +def test_plugin_configs_equals_form_parses_csv() -> None: + ns = _parser().parse_args(["--plugin-configs=NodeStatus,AllPlugins"]) + assert ns.plugin_configs == ["NodeStatus", "AllPlugins"] + + +def test_plugin_configs_space_separated_parses() -> None: + ns = _parser().parse_args(["--plugin-configs", "NodeStatus,AllPlugins"]) + assert ns.plugin_configs == ["NodeStatus", "AllPlugins"]