From e67205679a1cabd4a803a2595ef9f41de07024e4 Mon Sep 17 00:00:00 2001 From: Simon Maillard Date: Fri, 3 Apr 2026 20:07:27 +0000 Subject: [PATCH 1/2] feat(facts): add requires_command guard sentinel and check_preconditions() hook --- CHANGELOG.md | 13 +++ docs/api/facts.md | 77 ++++++++++++- src/pyinfra/api/__init__.py | 3 + src/pyinfra/api/exceptions.py | 32 ++++++ src/pyinfra/api/facts.py | 105 +++++++++++++++--- .../test_api_facts_missing_command.py | 71 ++++++++++++ .../test_api/test_api_facts_preconditions.py | 69 ++++++++++++ 7 files changed, 353 insertions(+), 17 deletions(-) create mode 100644 tests/test_api/test_api_facts_missing_command.py create mode 100644 tests/test_api/test_api_facts_preconditions.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 905e29596..dcb315550 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,16 @@ +# Unreleased + +Core: + +- api.facts: `requires_command` now uses an if/then/else shell guard with a sentinel marker + (`##PYINFRA_NOCMD##`) so that "binary absent" can be distinguished from "binary returned no data" +- api.facts: add `check_preconditions(state, host)` hook on `FactBase` for runtime prerequisite checks + (e.g. kernel module loaded, service running) — return a reason string or `None` +- api.exceptions: add `FactNotCollected`, `MissingCommandError`, `FactPreconditionError` exception + hierarchy; both exceptions are phase-aware (silent during prepare, raised during execute) +- facts.zfs: fix `ZfsDatasets.requires_command` returning `"zpool"` instead of `"zfs"`; add + `ZfsPools` fact; add `ZfsDatasets.check_preconditions()` checking kernel module via `server.KernelModules` + # v3.7 Thank you to all contributors - particular shout out to @wowi42 for an incredible run of PRs! diff --git a/docs/api/facts.md b/docs/api/facts.md index 1f0c2d100..40386b257 100644 --- a/docs/api/facts.md +++ b/docs/api/facts.md @@ -5,9 +5,80 @@ and a ``process`` function. The command is executed on the target host and the o passed (as a ``list`` of lines) to the ``process`` handler to generate fact data. Facts can output anything, normally a ``list`` or ``dict``. Fact classes may provide a ``default`` function that takes no arguments (except ``self``). The return value of this function is used if an error -occurs during fact collection. Additionally, a ``requires_command`` variable can be set on the fact that specifies a command that must be available -on the host to collect the fact. If this command is not present on the host, the fact will be set to the default, or empty if no ``default`` function -is available. +occurs during fact collection. + +## Guarding against missing binaries: `requires_command` + +Override `requires_command` to declare a binary that must be present on the remote host before +the fact command is run. When the binary is absent pyinfra emits a unique sentinel instead of +executing the command, then raises `MissingCommandError` internally: + +```py +from pyinfra.api import FactBase + +class ZfsPools(FactBase): + def requires_command(self) -> str: + return "zpool" + + def command(self) -> str: + return "zpool get -H all" +``` + +**Phase-aware behaviour** — The exception is handled differently depending on the deploy phase: + +- **Prepare phase**: the fact returns `default()` silently. The binary may simply not be + installed yet; a later operation will install it. +- **Execute phase** (v3): a warning is logged and `default()` is returned. This preserves + backwards compatibility for deploys that rely on `default()` being returned when a binary + is absent. +- **Execute phase** (v4): `MissingCommandError` will be raised so the developer knows the + deploy is incorrectly ordered (the install step must come first). + +## Checking runtime prerequisites: `check_preconditions()` + +Some facts require more than just a binary — they need a specific runtime state (e.g. a kernel +module loaded, a service running). Override `check_preconditions` to express these checks: + +```py +from pyinfra.api import FactBase + +class ZfsDatasets(FactBase): + def requires_command(self) -> str: + return "zfs" + + def check_preconditions(self, state, host): + from pyinfra.facts.server import KernelModules + modules = host.get_fact(KernelModules) or {} + if "zfs" not in modules: + return "kernel module 'zfs' is not loaded" + + def command(self) -> str: + return "zfs get -H all" +``` + +Return values: + +| Return value | Meaning | +|---|---| +| `None` (or no return) | Prerequisites satisfied — proceed normally | +| `"reason"` | Prerequisite not satisfied with a human-readable explanation | + +The framework raises `FactPreconditionError` automatically and applies the same +phase-aware behaviour as `requires_command`: silent during prepare, raised during execute. +Fact authors never need to import any exception class. + +## Exception hierarchy + +All "fact skipped" situations use a common base class so callers can catch at any level: + +``` +FactError +└── FactNotCollected # base: fact could not be collected + ├── MissingCommandError # requires_command binary absent + └── FactPreconditionError # check_preconditions() not satisfied +``` + +All three are exported from `pyinfra.api`. ## Importing & Using Facts diff --git a/src/pyinfra/api/__init__.py b/src/pyinfra/api/__init__.py index 4ee9d0067..8d7a6f842 100644 --- a/src/pyinfra/api/__init__.py +++ b/src/pyinfra/api/__init__.py @@ -11,11 +11,14 @@ from .deploy import deploy # noqa: F401 # pragma: no cover from .exceptions import ( # noqa: F401 DeployError, # noqa: F401 # pragma: no cover + FactPreconditionError, FactError, + FactNotCollected, FactProcessError, FactTypeError, FactValueError, InventoryError, + MissingCommandError, OperationError, OperationTypeError, OperationValueError, diff --git a/src/pyinfra/api/exceptions.py b/src/pyinfra/api/exceptions.py index 061a57121..1d5fb574d 100644 --- a/src/pyinfra/api/exceptions.py +++ b/src/pyinfra/api/exceptions.py @@ -60,6 +60,38 @@ class NestedOperationError(OperationError): """ +class FactNotCollected(FactError): + """ + Base exception raised when a fact could not be collected (e.g. binary absent + on the remote host, or the fact was skipped by a condition). + """ + + +class MissingCommandError(FactNotCollected): + """ + Exception raised when ``requires_command`` specifies a binary that is + not present on the remote host. The fact returns its ``default()`` value + instead of raising, unless explicitly configured otherwise. + """ + + def __init__(self, command: str) -> None: + self.command = command + super().__init__(f"Command not found on remote host: {command}") + + +class FactPreconditionError(FactNotCollected): + """ + Exception raised when a fact's ``check_preconditions()`` returns a reason string + (e.g. a kernel module is not loaded). Like ``MissingCommandError``, this is + silenced during the prepare phase and re-raised during execute. + """ + + def __init__(self, fact_cls: type, reason: str) -> None: + self.fact_cls = fact_cls + self.reason = reason + super().__init__(f"Fact precondition not satisfied ({fact_cls.__name__}): {reason}") + + class DeployError(PyinfraError): """ User exception for raising in deploys or sub deploys. diff --git a/src/pyinfra/api/facts.py b/src/pyinfra/api/facts.py index eb271908d..d2e7c363c 100644 --- a/src/pyinfra/api/facts.py +++ b/src/pyinfra/api/facts.py @@ -24,7 +24,7 @@ from pyinfra.api.output import format_text from pyinfra.api import StringCommand from pyinfra.api.arguments import all_global_arguments, pop_global_arguments -from pyinfra.api.exceptions import FactProcessError +from pyinfra.api.exceptions import FactPreconditionError, FactProcessError, MissingCommandError from pyinfra.api.util import ( get_kwargs_str, log_error_or_warning, @@ -36,10 +36,14 @@ from pyinfra.progress import progress_spinner from .arguments import CONNECTOR_ARGUMENT_KEYS +from .state import StateStage if TYPE_CHECKING: from pyinfra.api import Host, State +# Sentinel output line emitted when skip_unless_command binary is absent on the remote host. +_MISSING_COMMAND_MARKER = "##PYINFRA_NOCMD##" + SUDO_REGEX = r"^sudo: unknown user" SU_REGEXES = ( r"^su: user .+ does not exist", @@ -60,6 +64,22 @@ class FactBase(Generic[T]): command: Callable[..., str | StringCommand] def requires_command(self, *args, **kwargs) -> str | None: + """Return the binary name that must exist on the remote host for this fact to run. + If the binary is absent the fact returns its ``default()`` value silently. + """ + return None + + def check_preconditions(self, state: "State", host: "Host") -> str | None: + """Check that this fact's prerequisites are satisfied before running. + + Override this method to call ``host.get_fact(...)`` and return: + + - ``None`` (or no return) — all prerequisites satisfied, proceed normally + - ``"reason message"`` — prerequisite not satisfied with explanation + + The framework handles raising ``FactPreconditionError`` and phase-awareness + automatically; fact authors never need to import exception classes. + """ return None @override @@ -186,15 +206,51 @@ def get_fact( apply_failed_hosts=apply_failed_hosts, ) - return _get_fact( - state, - host, - cls, - args, - kwargs, - ensure_hosts, - apply_failed_hosts, - ) + try: + return _get_fact( + state, + host, + cls, + args, + kwargs, + ensure_hosts, + apply_failed_hosts, + ) + except MissingCommandError as e: + # During the prepare phase the binary might not yet be installed (a prior + # operation will install it). Silently return the default so change + # detection can proceed normally. + if state.current_stage != StateStage.Execute: + logger.debug( + "Fact %s skipped on %s during prepare: %s", + cls.__name__, + host.print_prefix, + e, + ) + return cls().default() + # During the execute phase the binary should already be present. If it + # isn't, the deploy is incorrectly ordered (missing an install step?). + # TODO(v4): remove this compat shim and let the exception propagate. + logger.warning( + "Fact %s skipped on %s: command not found: %s (this will raise an exception in v4)", + cls.__name__, + host.print_prefix, + e, + ) + return cls().default() + except FactPreconditionError as e: + # Same phase-aware logic: a precondition not satisfied during prepare + # is normal (e.g. kernel module not yet loaded); during execute it is an + # ordering error in the deploy. + if state.current_stage != StateStage.Execute: + logger.debug( + "Fact %s skipped on %s during prepare: %s", + cls.__name__, + host.print_prefix, + e, + ) + return cls().default() + raise def _get_fact( @@ -229,20 +285,30 @@ def _get_fact( if fact.shell_executable: global_kwargs["_shell_executable"] = fact.shell_executable + # Check preconditions before running this fact's command. + if reason := fact.check_preconditions(state, host): + raise FactPreconditionError(cls, reason) + command = _make_command(fact.command, fact_kwargs) requires_command = _make_command(fact.requires_command, fact_kwargs) if requires_command: command = StringCommand( - # Command doesn't exist, return 0 *or* run & return fact command - "!", + # If binary exists → run the fact command; otherwise emit the sentinel so + # pyinfra can distinguish "binary absent" from "no output". + "if", "command", "-v", requires_command, ">/dev/null", - "||", + "2>&1;", + "then", "(", command, - ")", + ");", + "else", + "echo", + f"'{_MISSING_COMMAND_MARKER}';", + "fi", ) status = False @@ -268,6 +334,17 @@ def _get_fact( stdout_lines, stderr_lines = output.stdout_lines, output.stderr_lines + # Detect the "binary absent" sentinel from the if/then/else shell guard. + if status and stdout_lines == [_MISSING_COMMAND_MARKER]: + cmd_str = str(requires_command) if requires_command else "" + logger.debug( + "Skipping fact %s on %s: command not found: %s", + name, + host.print_prefix, + cmd_str, + ) + raise MissingCommandError(cmd_str) + data = fact.default() if status: diff --git a/tests/test_api/test_api_facts_missing_command.py b/tests/test_api/test_api_facts_missing_command.py new file mode 100644 index 000000000..1a7bde906 --- /dev/null +++ b/tests/test_api/test_api_facts_missing_command.py @@ -0,0 +1,71 @@ +""" +Tests for MissingCommandError / requires_command phase-aware behaviour. + +When a fact declares ``requires_command`` and the requested binary is absent: + +- **Prepare phase** – silent: the binary might not yet be installed. + The fact must return ``default()`` so change-detection can proceed. +- **Execute phase** – compat shim (v3): warning logged, ``default()`` returned. + Will raise ``MissingCommandError`` in v4. +""" + +from unittest import TestCase +from unittest.mock import MagicMock, patch + +from pyinfra.api.exceptions import FactError, FactNotCollected, MissingCommandError +from pyinfra.api.facts import _MISSING_COMMAND_MARKER, get_fact +from pyinfra.api.state import StateStage +from pyinfra.facts.zfs import ZfsPools + + +def _make_state(stage: StateStage) -> MagicMock: + state = MagicMock() + state.current_stage = stage + return state + + +class TestMissingCommandPhaseAwareness(TestCase): + """Phase-aware handling of MissingCommandError in the public get_fact() wrapper.""" + + def test_prepare_phase_returns_default(self): + """Binary absent during prepare → fact returns default() silently.""" + state = _make_state(StateStage.Prepare) + host = MagicMock() + + with patch("pyinfra.api.facts._get_fact", side_effect=MissingCommandError("zpool")): + result = get_fact(state, host, ZfsPools) + + assert result == ZfsPools().default() + + def test_execute_phase_returns_default_with_warning(self): + """Binary absent during execute → compat shim: warning logged, default() returned (v3). + TODO(v4): this should raise MissingCommandError instead.""" + state = _make_state(StateStage.Execute) + host = MagicMock() + + with patch("pyinfra.api.facts._get_fact", side_effect=MissingCommandError("zpool")): + result = get_fact(state, host, ZfsPools) + + assert result == ZfsPools().default() + + def test_missing_command_error_message(self): + """MissingCommandError carries a human-readable message.""" + exc = MissingCommandError("zpool") + assert "zpool" in str(exc) + + def test_missing_command_inherits_fact_not_collected(self): + """MissingCommandError is a FactNotCollected (and a FactError).""" + exc = MissingCommandError("zpool") + assert isinstance(exc, FactNotCollected) + assert isinstance(exc, FactError) + + +class TestMissingCommandMarker(TestCase): + """The sentinel constant is stable – callers may depend on it.""" + + def test_marker_is_unique_string(self): + assert isinstance(_MISSING_COMMAND_MARKER, str) + assert len(_MISSING_COMMAND_MARKER) > 0 + # Must not look like valid shell output from any standard command + assert _MISSING_COMMAND_MARKER.startswith("##") + assert _MISSING_COMMAND_MARKER.endswith("##") diff --git a/tests/test_api/test_api_facts_preconditions.py b/tests/test_api/test_api_facts_preconditions.py new file mode 100644 index 000000000..dc50ba463 --- /dev/null +++ b/tests/test_api/test_api_facts_preconditions.py @@ -0,0 +1,69 @@ +""" +Tests for FactPreconditionError / check_preconditions() phase-aware behaviour. + +When a fact's ``check_preconditions()`` method returns a reason string: + +- **Prepare phase** – silent: the condition might not yet be satisfied (e.g. a + kernel module not yet loaded). The fact must return ``default()``. +- **Execute phase** – loud: ``check_preconditions`` is a new API so no existing + deploys rely on it. The exception propagates so the developer gets a clear + error about incorrect deploy ordering. +""" + +from unittest import TestCase +from unittest.mock import MagicMock, patch + +from pyinfra.api.exceptions import FactError, FactNotCollected, FactPreconditionError +from pyinfra.api.facts import get_fact +from pyinfra.api.state import StateStage +from pyinfra.facts.zfs import ZfsPools + + +def _make_state(stage: StateStage) -> MagicMock: + state = MagicMock() + state.current_stage = stage + return state + + +class TestFactPreconditionError(TestCase): + """Phase-aware handling of FactPreconditionError via check_preconditions() in get_fact().""" + + def test_prepare_phase_returns_default(self): + """Precondition not satisfied during prepare → fact returns default() silently.""" + state = _make_state(StateStage.Prepare) + host = MagicMock() + + with patch( + "pyinfra.api.facts._get_fact", + side_effect=FactPreconditionError(ZfsPools, "zfs module not loaded"), + ): + result = get_fact(state, host, ZfsPools) + + assert result == ZfsPools().default() + + def test_execute_phase_raises(self): + """Precondition not satisfied during execute → FactPreconditionError propagates.""" + state = _make_state(StateStage.Execute) + host = MagicMock() + + with patch( + "pyinfra.api.facts._get_fact", + side_effect=FactPreconditionError(ZfsPools, "zfs module not loaded"), + ): + with self.assertRaises(FactPreconditionError) as ctx: + get_fact(state, host, ZfsPools) + + assert ctx.exception.fact_cls is ZfsPools + assert "zfs module not loaded" in ctx.exception.reason + + def test_precondition_error_message(self): + """FactPreconditionError carries a human-readable message.""" + exc = FactPreconditionError(ZfsPools, "module not loaded") + assert "ZfsPools" in str(exc) + assert "module not loaded" in str(exc) + + def test_precondition_error_inherits_fact_not_collected(self): + """FactPreconditionError is a FactNotCollected (and a FactError).""" + exc = FactPreconditionError(ZfsPools, "reason") + assert isinstance(exc, FactNotCollected) + assert isinstance(exc, FactError) From 5b8df1fa1f390e06bac31fd9a3e9ec9432b0d3f2 Mon Sep 17 00:00:00 2001 From: Simon Maillard Date: Tue, 21 Apr 2026 08:01:03 +0000 Subject: [PATCH 2/2] feat(facts.zfs): add check_preconditions for ZfsDatasets and update command method --- src/pyinfra/facts/zfs.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/pyinfra/facts/zfs.py b/src/pyinfra/facts/zfs.py index 7e5212a9e..38be64471 100644 --- a/src/pyinfra/facts/zfs.py +++ b/src/pyinfra/facts/zfs.py @@ -34,16 +34,24 @@ def process(self, output): class ZfsDatasets(FactBase): - @override - def command(self) -> str: - return "zfs get -H all" - @override def requires_command(self) -> str: return "zfs" default = dict + @override + def check_preconditions(self, state, host): + from pyinfra.facts.server import KernelModules + + modules = host.get_fact(KernelModules) or {} + if "zfs" not in modules: + return "kernel module 'zfs' is not loaded" + + @override + def command(self) -> str: + return "zfs get -H all" + @override def process(self, output): return _process_zfs_props_table(output)