Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/112.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Refactored the config CLI into task-oriented 'list' and 'validate' subcommands and fixed a crash that occurred when requesting help while having an invalid configuration file.
19 changes: 13 additions & 6 deletions src/docbuild/cli/cmd_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,13 @@ def cli(
context = ctx.obj
context.verbose, context.dry_run, context.debug = verbose, dry_run, debug

# State tracking for centralized error handling
# --- LAZY FIX ---
# If the user is just asking for help, STOP HERE.
# Click will handle the help display for the subcommands automatically.
if ctx.resilient_parsing or "--help" in sys.argv or "-h" in sys.argv:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really needed to explicitly check for --help/-h? Shouldn't that be the task of Click? This looks like a code smell to me. 😉

return

# 2. Only load config if we aren't displaying help
current_model: type[BaseModel] = AppConfig
current_files: Sequence[Path] | None = None

Expand All @@ -229,11 +235,12 @@ def cli(
current_files = (app_config,) if app_config else None
load_app_config(ctx, app_config, max_workers)

# Setup logging
logging_config = context.appconfig.logging.model_dump(
by_alias=True, exclude_none=True
)
setup_logging(cliverbosity=verbose, user_config={"logging": logging_config})
# Access logging safely
if context.appconfig and context.appconfig.logging:
logging_config = context.appconfig.logging.model_dump(
by_alias=True, exclude_none=True
)
setup_logging(cliverbosity=verbose, user_config={"logging": logging_config})

# --- PHASE 2: Load Environment Config ---
current_model = EnvConfig
Expand Down
16 changes: 8 additions & 8 deletions src/docbuild/cli/cmd_config/__init__.py
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
"""CLI interface to shows config files how docbuild sees it."""
"""CLI interface for docbuild configuration management."""

import click

from .application import app
from .environment import env
from .list import list_config
from .validate import validate_config


@click.group(
name="config",
help=__doc__,
help="CLI interface to manage and verify configuration files.",
)
@click.pass_context
def config(ctx: click.Context) -> None:
"""Subcommand to show the configuration files and their content."""
"""Subcommand to manage docbuild configuration (TOML and XML)."""
pass


# Register the subcommands for the config group
config.add_command(env)
config.add_command(app)
# Register the task-oriented subcommands
config.add_command(list_config)
config.add_command(validate_config)
23 changes: 0 additions & 23 deletions src/docbuild/cli/cmd_config/application.py

This file was deleted.

26 changes: 0 additions & 26 deletions src/docbuild/cli/cmd_config/environment.py

This file was deleted.

62 changes: 62 additions & 0 deletions src/docbuild/cli/cmd_config/list.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
"""CLI interface to list the configuration."""

from typing import Any

import click
from rich import print_json
from rich.console import Console

console = Console()

def flatten_dict(d: dict[str, Any], prefix: str = "") -> dict[str, Any]:
"""Flatten a nested dictionary into dotted keys (e.g., app.logging.level)."""
items: list[tuple[str, Any]] = []
for k, v in d.items():
new_key = f"{prefix}.{k}" if prefix else k
if isinstance(v, dict):
items.extend(flatten_dict(v, new_key).items())
else:
items.append((new_key, v))
return dict(items)
Comment on lines +11 to +20
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function looks quite general. Maybe move this function somewhere in src/docbuild/utils/? Use flatten.py?

Comment on lines +11 to +20
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is converting the list to a dict really necessary? It should be left to the caller.
Additionally, maybe turn the function into a generator. This won't hold the entire list in memory, making it more efficient.

def flatten_dict(d: dict[str, Any], prefix: str = "") -> Generator[tuple[str, Any], None, None]:
    """
    A generator that yields flattened key-value pairs from a nested dictionary.
    
    :param prefix: The accumulated path of keys from higher levels of 
       nesting. This is used to build the dotted string (e.g., "parent.child").
       Defaults to an empty string for the root level.
    :yields: A tuple containing the (dotted_key, value) pair.
    """
    for k, v in d.items():
        # Construct the new key based on the current nesting level
        new_key = f"{prefix}.{k}" if prefix else k
        
        if isinstance(v, dict):
            # yield from recursively calls the generator and yields its results
            yield from flatten_dict(v, new_key)
        else:
            # Yield the final flattened key and its non-dict value
            yield new_key, v


def _print_section(title: str, data: dict[str, Any], prefix: str, flat: bool, color: str) -> None:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the underscore here as it's not really a very secret function. I would also introduce an additional line break as it's visually more readable (I think it should pass the ruff check).

Suggested change
def _print_section(title: str, data: dict[str, Any], prefix: str, flat: bool, color: str) -> None:
def print_section(title: str, data: dict[str, Any], prefix: str, flat: bool, color: str) -> None:

"""Print the Application and Environment configuration sections."""
if flat:
for k, v in flatten_dict(data, prefix).items():
console.print(f"[bold {color}]{k}[/bold {color}] = [green]{v}[/green]")
else:
console.print(f"\n# {title}", style="blue")
print_json(data=data)

def _print_portal(doctypes: list[Any], flat: bool) -> None:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here:

Suggested change
def _print_portal(doctypes: list[Any], flat: bool) -> None:
def print_portal(doctypes: list[Any], flat: bool) -> None:

"""Print the Portal and Doctype metadata section."""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you document the arguments in the docstring?

if not flat:
console.print("\n# Portal/Doctype Metadata", style="blue")

for doctype in doctypes:
name = getattr(doctype, "name", "Unknown")
path = str(getattr(doctype, "path", "N/A"))
if flat:
console.print(f"[bold magenta]portal.{name}[/bold magenta] = [green]{path}[/green]")
else:
console.print(f" - [bold]{name}[/bold]: {path}")

@click.command(name="list")
@click.option("--app", is_flag=True, help="Show only application configuration")
@click.option("--env", is_flag=True, help="Show only environment configuration")
@click.option("--portal", is_flag=True, help="Show only portal/doctype metadata")
@click.option("--flat", is_flag=True, help="Output in flat dotted format (git-style)")
@click.pass_context
def list_config(ctx: click.Context, app: bool, env: bool, portal: bool, flat: bool) -> None:
"""List the configuration as JSON or flat text."""
context = ctx.obj
show_all = not (app or env or portal)

if (app or show_all) and context.appconfig:
_print_section("Application Configuration", context.appconfig.model_dump(), "app", flat, "cyan")

if (env or show_all) and context.envconfig:
_print_section("Environment Configuration", context.envconfig.model_dump(), "env", flat, "yellow")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why, but when I use my env.devel.toml file, I get a traceback:

Traceback of docbuild config list --env
$ docbuild --env-config=env.devel.toml config list --env

# Environment Configuration
Traceback (most recent call last):
  File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/bin/docbuild", line 10, in <module>
    sys.exit(cli())
             ~~~^^
  File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/click/core.py", line 1514, in __call__
    return self.main(*args, **kwargs)
           ~~~~~~~~~^^^^^^^^^^^^^^^^^
  File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/click/core.py", line 1435, in main
    rv = self.invoke(ctx)
  File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/click/core.py", line 1902, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^
  File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/click/core.py", line 1902, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^
  File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/click/core.py", line 1298, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/click/core.py", line 853, in invoke
    return callback(*args, **kwargs)
  File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/click/decorators.py", line 34, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/home/toms/repos/GH/opensuse/docbuild.git/main/src/docbuild/cli/cmd_config/list.py", line 59, in list_config
    _print_section("Environment Configuration", context.envconfig.model_dump(), "env", flat, "yellow")
    ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/toms/repos/GH/opensuse/docbuild.git/main/src/docbuild/cli/cmd_config/list.py", line 29, in _print_section
    print_json(data=data)
    ~~~~~~~~~~^^^^^^^^^^^
  File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/rich/__init__.py", line 106, in print_json
    get_console().print_json(
    ~~~~~~~~~~~~~~~~~~~~~~~~^
        json,
        ^^^^^
    ...<8 lines>...
        sort_keys=sort_keys,
        ^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/rich/console.py", line 1790, in print_json
    json_renderable = JSON.from_data(
        data,
    ...<7 lines>...
        sort_keys=sort_keys,
    )
  File "/home/toms/repos/GH/opensuse/docbuild.git/main/.venv/lib/python3.14/site-packages/rich/json.py", line 85, in from_data
    json = dumps(
        data,
    ...<6 lines>...
        sort_keys=sort_keys,
    )
  File "/home/toms/.local/share/uv/python/cpython-3.14.3-linux-x86_64-gnu/lib/python3.14/json/__init__.py", line 242, in dumps
    **kw).encode(obj)
          ~~~~~~^^^^^
  File "/home/toms/.local/share/uv/python/cpython-3.14.3-linux-x86_64-gnu/lib/python3.14/json/encoder.py", line 202, in encode
    chunks = self.iterencode(o, _one_shot=True)
  File "/home/toms/.local/share/uv/python/cpython-3.14.3-linux-x86_64-gnu/lib/python3.14/json/encoder.py", line 263, in iterencode
    return _iterencode(o, 0)
  File "/home/toms/.local/share/uv/python/cpython-3.14.3-linux-x86_64-gnu/lib/python3.14/json/encoder.py", line 182, in default
    raise TypeError(f'Object of type {o.__class__.__name__} '
                    f'is not JSON serializable')
TypeError: Object of type IPv4Address is not JSON serializable
when serializing dict item 'host'
when serializing dict item 'server'

It looks like it's a problem with our models?


if (portal or show_all) and context.doctypes:
_print_portal(context.doctypes, flat)
Comment on lines +56 to +62
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjust the function names accordingly:

Suggested change
_print_section("Application Configuration", context.appconfig.model_dump(), "app", flat, "cyan")
if (env or show_all) and context.envconfig:
_print_section("Environment Configuration", context.envconfig.model_dump(), "env", flat, "yellow")
if (portal or show_all) and context.doctypes:
_print_portal(context.doctypes, flat)
print_section("Application Configuration", context.appconfig.model_dump(), "app", flat, "cyan")
if (env or show_all) and context.envconfig:
print_section("Environment Configuration", context.envconfig.model_dump(), "env", flat, "yellow")
if (portal or show_all) and context.doctypes:
print_portal(context.doctypes, flat)

57 changes: 57 additions & 0 deletions src/docbuild/cli/cmd_config/validate.py
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this needs to be broken down even further. Why not create a package like this:

validate
├── __init__.py
├── app.py
├── env.py
└── portal.py

The __init__.py would be the "orchestrator" and delegate it to the respective configs. Especially the portal.py will become quite big. This is not driven by Pydantic and needs a specific validation logic (using RELAX NG and some Python logic).

Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
"""CLI interface to validate the configuration files."""

import click
from rich.console import Console
from rich.panel import Panel


@click.command(name="validate")
@click.pass_context
def validate_config(ctx: click.Context) -> None:
Comment on lines +8 to +10
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will validate all config files. I think, that's unfortunate; maybe you only want to validate the XML? Or the env config?

I would introduce the options --portal, --env, and --app to restrict it to the specific config only. Additionally, the code below also introduces an --all option. The idea would be that --all and the other three options are mutually exclusive.

This how you could add more arguments with an opt-in logic:

Suggested change
@click.command(name="validate")
@click.pass_context
def validate_config(ctx: click.Context) -> None:
@click.command(name="validate")
@click.option('portal_flag',
'--portal/--no-portal',
default=False,
help="Validate the portal settings"
)
@click.option('env_flag',
'--env/--no-env',
default=False,
help="Validate the environment settings"
)
@click.option('app_flag',
'--app/--no-app',
default=False,
help="Validate the app settings"
)
@click.option('all_flag',
'--all',
is_flag=True,
default=True,
help="Validate everything (Exclusive)."
)
@click.pass_context
def validate_config(ctx: click.Context, portal_flag, env_flag, app_flag, all_flag) -> None:

With that options, you would have

"""Validate all configuration files (TOML and XML).

This command performs a full check of the application settings,
environment overrides, and portal doctypes.
"""
context = ctx.obj
console = Console()

# Since this command is reached only if the main CLI loader
# didn't exit with an error, we know the TOML files are
# syntactically correct and match the Pydantic models.

console.print("[bold blue]Running Configuration Validation...[/bold blue]\n")

# 1. App Config Status
if context.appconfig:
console.print("✅ [bold]Application Configuration:[/bold] Valid")
if context.appconfigfiles:
for f in context.appconfigfiles:
console.print(f" [dim]- {f}[/dim]")

# 2. Env Config Status
if context.envconfig:
console.print("\n✅ [bold]Environment Configuration:[/bold] Valid")
if context.envconfigfiles:
for f in context.envconfigfiles:
console.print(f" [dim]- {f}[/dim]")
elif context.envconfig_from_defaults:
console.print(" [dim]- Using internal defaults[/dim]")

# 3. Portal/Doctype Status
if context.doctypes:
console.print(f"\n✅ [bold]Portals/Doctypes:[/bold] {len(context.doctypes)} discovered")
for doctype in context.doctypes:
name = getattr(doctype, "name", "Unknown")
console.print(f" [dim]- {name}[/dim]")
Comment on lines +42 to +46
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not validation. Validation is much more.😉

After looking at your code, I remembered that I had done something similar for the Docserv/Portal validation. I introduced a docbuild validate subcommand back then.
See src/docbuild/cli/cmd_validate/process.py.

Sorry that I didn't tell you first. I guess we should remove the docbuild validate subcommand. Perhaps you should also move these functions and move it here.

else:
console.print("\n⚠️ [bold yellow]Portals/Doctypes:[/bold yellow] None discovered")

console.print(
Panel(
"[bold green]Configuration is valid![/bold green]\n"
"All TOML files match the required schema and portals are reachable.",
border_style="green",
expand=False
)
)
58 changes: 0 additions & 58 deletions tests/cli/cmd_config/test_application.py

This file was deleted.

8 changes: 0 additions & 8 deletions tests/cli/cmd_config/test_config.py

This file was deleted.

40 changes: 40 additions & 0 deletions tests/cli/cmd_config/test_list.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
from pathlib import Path
from unittest.mock import Mock

import pytest

from docbuild.cli.cmd_cli import cli
from docbuild.models.config.app import AppConfig
from docbuild.models.config.env import EnvConfig


@pytest.fixture
def mock_models(monkeypatch):
"""Local fixture to mock config models for list command tests."""
mock_app = Mock(spec=AppConfig)
# 1. Provide the logging attribute the CLI expects
mock_app.logging = Mock()
mock_app.logging.model_dump.return_value = {"version": 1}
# 2. Provide the data for our 'list' command
mock_app.model_dump.return_value = {"key": "value", "logging": {"level": "info"}}

mock_env = Mock(spec=EnvConfig)
mock_env.model_dump.return_value = {"env_key": "env_val"}

monkeypatch.setattr(AppConfig, "from_dict", Mock(return_value=mock_app))
monkeypatch.setattr(EnvConfig, "from_dict", Mock(return_value=mock_env))
return mock_app

def test_config_list_json(runner, mock_models, fake_handle_config):
"""Test that config list shows the expected JSON output."""
fake_handle_config(lambda *a, **k: ((Path("test.toml"),), {"key": "value"}, False))
result = runner.invoke(cli, ["config", "list"])
assert result.exit_code == 0
assert '"key": "value"' in result.output

def test_config_list_flat(runner, mock_models, fake_handle_config):
"""Test that config list with --flat shows flattened keys."""
fake_handle_config(lambda *a, **k: ((Path("test.toml"),), {"logging": {"level": "info"}}, False))
result = runner.invoke(cli, ["config", "list", "--flat"])
assert result.exit_code == 0
assert "app.logging.level = info" in result.output
44 changes: 44 additions & 0 deletions tests/cli/cmd_config/test_validate.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
from pathlib import Path
from unittest.mock import MagicMock, Mock

import pytest

from docbuild.cli.cmd_cli import cli
from docbuild.cli.context import DocBuildContext
from docbuild.models.config.app import AppConfig
from docbuild.models.config.env import EnvConfig


@pytest.fixture
def mock_models(monkeypatch):
"""Local fixture to mock config models for validate command tests."""
mock_app = Mock(spec=AppConfig)
mock_app.logging = Mock()
mock_app.logging.model_dump.return_value = {"version": 1}

mock_env = Mock(spec=EnvConfig)

monkeypatch.setattr(AppConfig, "from_dict", Mock(return_value=mock_app))
monkeypatch.setattr(EnvConfig, "from_dict", Mock(return_value=mock_env))
return mock_app

def test_config_validate_success(runner, mock_models, fake_handle_config):
"""Test that config validate succeeds with valid configuration."""
# Setup context attributes so they don't default to Mocks
# Manually create the context with real lists/tuples to avoid Mock iteration errors
ctx_obj = DocBuildContext()
ctx_obj.appconfig = mock_models
ctx_obj.appconfigfiles = (Path("app.toml"),)
ctx_obj.envconfig = MagicMock() # Mock for EnvConfig
ctx_obj.envconfigfiles = (Path("env.toml"),)
ctx_obj.doctypes = [] # Explicitly empty list

fake_handle_config(lambda *a, **k: ((Path("app.toml"),), {"key": "val"}, False))

# We use 'standalone_mode=False' so we can see the actual error if it crashes
result = runner.invoke(cli, ["config", "validate"], obj=ctx_obj)

assert result.exit_code == 0
# Use 'in' to check for text without worrying about exact formatting/colors
assert "Configuration is valid" in result.output
assert "Application Configuration" in result.output
Loading
Loading