Improve overall commands handling of envconfig (#181)

tomschr · web-flow · commit 3c41cee2c24e · 2026-02-17T08:44:22.000+01:00
* Adjust commands due to Pydantic validation
* As Pydantic takes care of validation the envconfig,
  we don't have to check that anymore
* 
* Remove any checking of if context.envconfig is None
* Adjust ManagedGitRepo to allow string or Repo as remote_url
  This makes it easier
* Fix tests
  Most tests had a dict instead of a EnvConfig object. This fix
  introduces a fake EnvConfig.
* Change DocBuildContext.envconfig to be a EnvConfig object and not a dict.
* Remove obsolete tests for checking None envconfig
   We have already Pydantic validation, no need to check.
* Test ManagedGitRepo with str and Repo
* Add news fragment
diff --git a/changelog.d/181.doc.rst b/changelog.d/181.doc.rst
@@ -0,0 +1,4 @@
+Streamlined CLI subcommands by removing redundant environment configuration
+checks and implementing strict typing with EnvConfig models.
+Updated :class:`~docbuild.utils.git.ManagedGitRepo` to support both string
+and :class:`~docbuild.models.repo.Repo` model initialization.
diff --git a/docs/source/reference/_autoapi/docbuild/utils/git/ManagedGitRepo.rst b/docs/source/reference/_autoapi/docbuild/utils/git/ManagedGitRepo.rst
@@ -1,7 +1,7 @@
 docbuild.utils.git.ManagedGitRepo
 =================================
 
-.. py:class:: docbuild.utils.git.ManagedGitRepo(remote_url: str, rootdir: pathlib.Path, gitconfig: pathlib.Path | None = None)
+.. py:class:: docbuild.utils.git.ManagedGitRepo(repo: str | docbuild.models.repo.Repo, rootdir: pathlib.Path, gitconfig: pathlib.Path | None = None)
 
    Manages a bare repository and its temporary worktrees.
 
diff --git a/src/docbuild/cli/cmd_build/__init__.py b/src/docbuild/cli/cmd_build/__init__.py
@@ -58,6 +58,7 @@ def build(ctx: click.Context, doctypes: tuple[Doctype]) -> None:
     """
     ctx.ensure_object(DocBuildContext)
     context: DocBuildContext = ctx.obj
+    # env = context.envconfig
 
     click.echo(f"[BUILD] Verbosity: {context.verbose}")
     click.echo(f"{context=}")
diff --git a/src/docbuild/cli/cmd_c14n/__init__.py b/src/docbuild/cli/cmd_c14n/__init__.py
@@ -10,4 +10,5 @@ def c14n(ctx: click.Context) -> None:
 
     :param ctx: The Click context object.
     """
+    # env = context.envconfig
     click.echo(f"[C17N] Verbosity: {ctx.obj.verbose}")
diff --git a/src/docbuild/cli/cmd_check/process.py b/src/docbuild/cli/cmd_check/process.py
@@ -3,13 +3,11 @@
 from collections.abc import Sequence
 import logging
 from pathlib import Path
-from typing import cast
 
 from docbuild.cli.cmd_metadata.metaprocess import get_deliverable_from_doctype
 from docbuild.cli.context import DocBuildContext
 from docbuild.config.xml.stitch import create_stitchfile
 from docbuild.constants import DEFAULT_DELIVERABLES
-from docbuild.models.config.env import EnvConfig
 from docbuild.models.deliverable import Deliverable
 from docbuild.models.doctype import Doctype
 from docbuild.utils.git import ManagedGitRepo
@@ -56,7 +54,7 @@ async def process_check_files(
     """Verify DC file existence using official Deliverable models."""
     log.info("Starting DC file availability check...")
 
-    env_config = cast(EnvConfig, ctx.envconfig)
+    env_config = ctx.envconfig
     config_dir = env_config.paths.config_dir.expanduser()
     repo_root = env_config.paths.repo_dir.expanduser()
 
diff --git a/src/docbuild/cli/cmd_metadata/metaprocess.py b/src/docbuild/cli/cmd_metadata/metaprocess.py
@@ -407,12 +407,13 @@ async def process(
         configured correctly.
     :return: 0 if all files passed validation, 1 if any failures occurred.
     """
-    configdir = Path(context.envconfig.paths.config_dir).expanduser()
+    env = context.envconfig
+    configdir = Path(env.paths.config_dir).expanduser()
     stdout.print(f"Config path: {configdir}")
     xmlconfigs = tuple(configdir.rglob("[a-z]*.xml"))
     stitchnode: etree._ElementTree = await create_stitchfile(xmlconfigs)
 
-    tmp_metadata_dir = context.envconfig.paths.tmp.tmp_metadata_dir
+    tmp_metadata_dir = env.paths.tmp.tmp_metadata_dir
     # TODO: Is this necessary here?
     tmp_metadata_dir.mkdir(parents=True, exist_ok=True)
 
diff --git a/src/docbuild/cli/cmd_repo/cmd_clone.py b/src/docbuild/cli/cmd_repo/cmd_clone.py
@@ -35,9 +35,6 @@ def clone(ctx: click.Context, repos: tuple[str, ...]) -> None:
     :param ctx: The Click context object.
     """
     context: DocBuildContext = ctx.obj
-    if context.envconfig is None:
-        raise ValueError("No envconfig found in context.")
-
     result = asyncio.run(process(context, repos))
     log.info(f"Clone process completed with exit code: {result}")
     ctx.exit(result)
diff --git a/src/docbuild/cli/cmd_repo/cmd_dir.py b/src/docbuild/cli/cmd_repo/cmd_dir.py
@@ -1,5 +1,6 @@
 """Show the directory path for permanent repositories."""
 
+
 import click
 
 from ...cli.context import DocBuildContext
@@ -16,9 +17,7 @@ def cmd_dir(ctx: click.Context) -> None:
     :param ctx: The Click context object.
     """
     context: DocBuildContext = ctx.obj
-    if context.envconfig is None:
-        raise ValueError("No envconfig found in context.")
-
-    repo_dir = context.envconfig.get("paths", {}).get("repo_dir", None)
+    env = context.envconfig
+    repo_dir = env.paths.repo_dir
     print(repo_dir)
     ctx.exit(0)
diff --git a/src/docbuild/cli/cmd_repo/cmd_list.py b/src/docbuild/cli/cmd_repo/cmd_list.py
@@ -24,15 +24,9 @@ def cmd_list(ctx: click.Context) -> None:
     :param ctx: The Click context object.
     """
     context: DocBuildContext = ctx.obj
-    if context.envconfig is None:
-        raise ValueError("No envconfig found in context.")
-
-    repo_dir = context.envconfig.get("paths", {}).get("repo_dir", None)
-    if repo_dir is None:
-        raise ValueError(
-            "No permanent repositories defined, neither with "
-            "--env-config nor as default."
-        )
+    env = context.envconfig
+
+    repo_dir = env.paths.repo_dir
     repo_dir = Path(repo_dir).resolve()
     if not repo_dir.exists():
         console_err.print(
diff --git a/src/docbuild/cli/cmd_repo/process.py b/src/docbuild/cli/cmd_repo/process.py
@@ -23,14 +23,9 @@ async def process(context: DocBuildContext, repos: tuple[str, ...]) -> int:
     :raises ValueError: If configuration paths are missing.
     """
     # The calling command function is expected to have checked context.envconfig.
-    paths = context.envconfig.get("paths", {})
-    config_dir_str = paths.get("config_dir")
-    repo_dir_str = paths.get("repo_dir")
-
-    if not config_dir_str:
-        raise ValueError("Could not get a value from envconfig.paths.config_dir")
-    if not repo_dir_str:
-        raise ValueError("Could not get a value from envconfig.paths.repo_dir")
+    envcfg = context.envconfig
+    config_dir_str = envcfg.paths.config_dir
+    repo_dir_str = envcfg.paths.repo_dir
 
     configdir = Path(config_dir_str).expanduser()
     repo_dir = Path(repo_dir_str).expanduser()
@@ -52,6 +47,7 @@ async def process(context: DocBuildContext, repos: tuple[str, ...]) -> int:
     else:
         # Create a unique list from user input, preserving order
         unique_git_repos = list(dict.fromkeys(Repo(r) for r in repos))
+        log.debug("User-specified repositories: %s", unique_git_repos)
 
     if not unique_git_repos:
         log.info("No repositories found to clone.")
diff --git a/src/docbuild/cli/cmd_validate/__init__.py b/src/docbuild/cli/cmd_validate/__init__.py
@@ -36,20 +36,12 @@ def validate(
     :param validation_method: Validation method to use, 'jing' or 'lxml'.
     """
     context: DocBuildContext = ctx.obj
+    env = context.envconfig
 
     # Set the chosen validation method in the context for downstream use
     context.validation_method = validation_method.lower()
 
-    if context.envconfig is None:
-        raise ValueError("No envconfig found in context.")
-
-    if (paths := ctx.obj.envconfig.get("paths")) is None:
-        raise ValueError("No paths found in envconfig.")
-
-    configdir = paths.get("config_dir", None)
-    if configdir is None:
-        raise ValueError("Could not get a value from envconfig.paths.config_dir")
-
+    configdir = env.paths.config_dir
     configdir_path = Path(configdir).expanduser()
 
     if not xmlfiles:
diff --git a/src/docbuild/cli/context.py b/src/docbuild/cli/context.py
@@ -4,6 +4,7 @@
 from pathlib import Path
 from typing import Any
 
+from ..models.config.env import EnvConfig
 from ..models.doctype import Doctype
 
 
@@ -32,7 +33,7 @@ class DocBuildContext:
     envconfig_from_defaults: bool = False
     """Internal flag to indicate if the env's config was loaded from defaults"""
 
-    envconfig: dict[str, Any] | None = None
+    envconfig: EnvConfig | None = None
     """The accumulated content of all env config files"""
 
     doctypes: list[Doctype] | None = None
diff --git a/src/docbuild/utils/git.py b/src/docbuild/utils/git.py
@@ -32,16 +32,24 @@ def clear_cache(cls) -> None:
         cls._is_updated.clear()
 
     def __init__(
-        self: Self, remote_url: str, rootdir: Path, gitconfig: Path | None = None
+        self: Self, repo: str | Repo, rootdir: Path, gitconfig: Path | None = None
     ) -> None:
         """Initialize the managed repository.
 
-        :param remote_url: The remote URL of the repository.
+        :param repo: The remote URL or :class:`~docbuild.models.repo.Repo` instance of the repository to manage.
+        Repo instance of the repository.
         :param permanent_root: The root directory for storing permanent bare clones.
         :param gitconfig: The path to a separate Git configuration file
            (=None, use the default config from etc/gitconfig)
         """
-        self._repo_model = Repo(remote_url)
+        if isinstance(repo, str):
+            self._repo_model = Repo(repo)
+        elif isinstance(repo, Repo):
+            self._repo_model = repo
+        else:
+            raise TypeError(
+                f"remote_url must be a string or Repo instance, got {type(repo)}"
+            )
         self._permanent_root = rootdir
         # The Repo model handles the "sluggification" of the URL
         self.bare_repo_path = self._permanent_root / self._repo_model.slug
diff --git a/tests/cli/cmd_repo/test_cmd_clone.py b/tests/cli/cmd_repo/test_cmd_clone.py
@@ -9,9 +9,32 @@
 from docbuild.cli.cmd_repo.cmd_clone import clone
 import docbuild.cli.cmd_repo.process as mod_process
 from docbuild.cli.context import DocBuildContext
+from docbuild.utils import shell as shell_module
 
 log = logging.getLogger(__name__)
 
+
+class _DummyPaths:
+    """Lightweight stand-in for EnvPathsConfig used in tests.
+
+    Only the attributes accessed by cmd_repo.process are provided.
+    """
+
+    def __init__(self, *, config_dir: str, repo_dir: str) -> None:
+        self.config_dir = config_dir
+        self.repo_dir = repo_dir
+
+
+class _DummyEnv:
+    """Minimal envconfig replacement exposing ``paths`` for tests.
+
+    This avoids constructing a full EnvConfig instance while still matching
+    the runtime behaviour expected by the cloning logic.
+    """
+
+    def __init__(self, *, config_dir: str, repo_dir: str) -> None:
+        self.paths = _DummyPaths(config_dir=config_dir, repo_dir=repo_dir)
+
 #
 # @pytest.fixture
 # def process_mock() -> AsyncMock:
@@ -29,8 +52,10 @@ def mock_subprocess(monkeypatch) -> AsyncMock:
     process_mock.communicate.return_value = (b"stdout", b"stderr")
     process_mock.returncode = 0
     mock_create_subprocess = AsyncMock(return_value=process_mock)
+    # Git commands go through shell.run_command → asyncio.create_subprocess_exec
+    # in the shell utility module.
     monkeypatch.setattr(
-        mod_process.asyncio, "create_subprocess_exec", mock_create_subprocess
+        shell_module.asyncio, "create_subprocess_exec", mock_create_subprocess
     )
     return mock_create_subprocess
 
@@ -63,9 +88,10 @@ def test_clone_from_xml_config(runner, tmp_path, mock_subprocess, caplog):
     (config_dir / "sles.xml").write_text(xml_content)
 
     context = DocBuildContext(
-        envconfig={
-            "paths": {"repo_dir": str(repo_dir), "config_dir": str(config_dir)},
-        },
+        envconfig=_DummyEnv(
+            config_dir=str(config_dir),
+            repo_dir=str(repo_dir),
+        ),
     )
 
     runner.invoke(clone, [], obj=context)
@@ -78,35 +104,17 @@ def test_clone_from_xml_config(runner, tmp_path, mock_subprocess, caplog):
     assert "https://github.com/test/two.git" in cloned_repos
 
 
-def test_clone_invalid_envconfig(runner):
-    """Test that an error is raised if the environment configuration is invalid."""
-    context = DocBuildContext(envconfig=None)
-
-    result = runner.invoke(
-        clone,
-        ["org/repo"],
-        obj=context,
-        # catch_exceptions=True,
-    )
-
-    assert result.exit_code != 0
-    assert isinstance(result.exception, ValueError)
-    assert "No envconfig found in context" in str(result.exception)
-
-
 # @pytest.mark.asyncio
 async def test_process_stitchnode_none(monkeypatch, tmp_path):
     """Test that process raises ValueError if create_stitchfile returns None."""
     # Patch create_stitchfile to return None
     monkeypatch.setattr(mod_process, "create_stitchfile", AsyncMock(return_value=None))
 
     context = DocBuildContext(
-        envconfig={
-            "paths": {
-                "repo_dir": str(tmp_path / "repos"),
-                "config_dir": str(tmp_path / "config"),
-            }
-        }
+        envconfig=_DummyEnv(
+            config_dir=str(tmp_path / "config"),
+            repo_dir=str(tmp_path / "repos"),
+        )
     )
 
     # The config_dir must exist, even if empty
@@ -119,18 +127,28 @@ async def test_process_stitchnode_none(monkeypatch, tmp_path):
 
 
 async def test_process_configdir_none():
-    context = DocBuildContext(envconfig={"paths": {}})
-    with pytest.raises(
-        ValueError,
-        match=re.escape("Could not get a value from envconfig.paths.config_dir"),
-    ):
-        await mod_process.process(context, repos=())
+    """This scenario is no longer reachable with validated EnvConfig.
+
+    The higher-level CLI now ensures ``envconfig`` is a fully validated
+    environment configuration. Retain this test as a smoke check that
+    calling ``process`` with a dummy, but structurally valid, envconfig
+    does not raise and returns an integer exit code.
+    """
+
+    context = DocBuildContext(
+        envconfig=_DummyEnv(config_dir="/non/existent/config", repo_dir="/tmp/repos"),
+    )
+
+    result = await mod_process.process(context, repos=())
+    assert isinstance(result, int)
 
 
 async def test_process_repodir_none():
-    context = DocBuildContext(envconfig={"paths": {"config_dir": "/dummy/config"}})
-    with pytest.raises(
-        ValueError,
-        match=re.escape("Could not get a value from envconfig.paths.repo_dir"),
-    ):
-        await mod_process.process(context, repos=())
+    """See docstring of test_process_configdir_none for rationale."""
+
+    context = DocBuildContext(
+        envconfig=_DummyEnv(config_dir="/tmp/config", repo_dir="/non/existent/repos"),
+    )
+
+    result = await mod_process.process(context, repos=())
+    assert isinstance(result, int)
diff --git a/tests/cli/cmd_repo/test_cmd_dir.py b/tests/cli/cmd_repo/test_cmd_dir.py
@@ -1,30 +1,30 @@
 from docbuild.cli.cmd_repo.cmd_dir import cmd_dir
 
 
-def test_cmd_dir_prints_repo_dir(runner, monkeypatch):
+class _DummyPaths:
+    """Minimal paths holder exposing ``repo_dir`` only."""
+
+    def __init__(self, repo_dir: str) -> None:
+        self.repo_dir = repo_dir
+
+
+class _DummyEnv:
+    """Fake EnvConfig-like object with a ``paths`` attribute."""
+
+    def __init__(self, repo_dir: str) -> None:
+        self.paths = _DummyPaths(repo_dir)
+
+
+def test_cmd_dir_prints_repo_dir(runner):
     dummy_repo_dir = "/tmp/myrepo"
 
     class DummyContext:
-        def __init__(self, repo_dir):
-            self.envconfig = {"paths": {"repo_dir": repo_dir}}
+        def __init__(self, repo_dir: str) -> None:
+            self.envconfig = _DummyEnv(repo_dir)
 
     ctx_obj = DummyContext(dummy_repo_dir)
 
     result = runner.invoke(cmd_dir, obj=ctx_obj)
 
     assert result.exit_code == 0
     assert dummy_repo_dir in result.output
-
-
-def test_cmd_dir_no_envconfig(runner, capsys):
-    class DummyContextNoEnv:
-        envconfig = None
-
-    result = runner.invoke(cmd_dir, obj=DummyContextNoEnv())
-
-    captured = capsys.readouterr()
-
-    assert captured.out == ""
-    assert result.exit_code != 0
-    assert isinstance(result.exception, ValueError)
-    assert "No envconfig found in context." in str(result.exception)
diff --git a/tests/cli/cmd_repo/test_cmd_list.py b/tests/cli/cmd_repo/test_cmd_list.py
diff --git a/tests/cli/cmd_repo/test_process.py b/tests/cli/cmd_repo/test_process.py
diff --git a/tests/cli/cmd_validate/test_process.py b/tests/cli/cmd_validate/test_process.py
diff --git a/tests/cli/cmd_validate/validate/test_cmd_validate.py b/tests/cli/cmd_validate/validate/test_cmd_validate.py
diff --git a/tests/utils/test_git.py b/tests/utils/test_git.py
