From 94f4b9208105a2b35be9531c3489e58efc3c49cc Mon Sep 17 00:00:00 2001 From: Simon Maillard Date: Sun, 26 Apr 2026 12:59:25 +0000 Subject: [PATCH] docs: add Claude skills for operations/facts/tests conventions Move detailed file-scoped conventions out of local-only AI configs and into tracked Claude skills under .claude/skills/, following the pattern proposed in upstream PR #1683: - writing-operations: operation patterns, _inner() composition, error handling - writing-facts: FactBase anatomy, parsing, multi-file marker pattern - writing-operation-tests: JSON/YAML fixture schema for tests/operations/ - writing-fact-tests: JSON/YAML fixture schema for tests/facts/ Also enrich AGENTS.md with a few items previously held only locally: - Command objects (StringCommand, QuoteString, MaskString, ...) - Connector data (connector_data_meta / host.connector_data) - Coverage gevent concurrency note - Pointer to pyinfra-dev-deploy for working with multiple PRs - Index of the new skills under "Detailed conventions" --- .claude/skills/writing-fact-tests/SKILL.md | 153 +++++++++++++++ .claude/skills/writing-facts/SKILL.md | 182 ++++++++++++++++++ .../skills/writing-operation-tests/SKILL.md | 150 +++++++++++++++ .claude/skills/writing-operations/SKILL.md | 159 +++++++++++++++ AGENTS.md | 32 ++- 5 files changed, 675 insertions(+), 1 deletion(-) create mode 100644 .claude/skills/writing-fact-tests/SKILL.md create mode 100644 .claude/skills/writing-facts/SKILL.md create mode 100644 .claude/skills/writing-operation-tests/SKILL.md create mode 100644 .claude/skills/writing-operations/SKILL.md diff --git a/.claude/skills/writing-fact-tests/SKILL.md b/.claude/skills/writing-fact-tests/SKILL.md new file mode 100644 index 000000000..48b0ac477 --- /dev/null +++ b/.claude/skills/writing-fact-tests/SKILL.md @@ -0,0 +1,153 @@ +--- +name: writing-fact-tests +--- + +JSON / YAML fixture format for fact unit tests. + +**Applies to:** `tests/facts/**/*.{json,yaml}` + +Fact tests live in `tests/facts/./` as JSON or YAML files. +Each file is one test case. The test runner (`tests/test_facts.py`) discovers +them automatically via the `TestGenerator` metaclass (which is ignored by pytest +collection to avoid warnings). + +**Prefer YAML for new fixtures.** To cover a new code path, add a fixture — do not +write a new Python test. + +## Fixture schema (JSON) + +```json +{ + "command": "the shell command the fact produces", + "requires_command": "binary-name", + "arg": [], + "output": [ + "line 1 of command stdout", + "line 2 of command stdout" + ], + "fact": +} +``` + +## Fixture schema (YAML) + +```yaml +command: shell command the fact runs +requires_command: binary # optional +output: | + raw stdout to parse +fact: + item: + key: value # expected return value of process() +``` + +### Fields + +| Field | Required | Description | +|---|---|---| +| `command` | Yes | Exact string returned by `fact.command(...)` | +| `requires_command` | No | Exact string returned by `fact.requires_command(...)` | +| `arg` | No | List of positional arg lists, one per call variant. Omit if the fact takes no args | +| `output` | Yes | Simulated stdout lines (list of strings, no trailing newlines) | +| `fact` | Yes | Expected return value of `fact.process(output)` | + +### `arg` format + +For facts that take arguments, `arg` is a list of argument lists: + +```json +"arg": [ + ["/etc/apt/trusted.gpg.d", "/etc/apt/keyrings"] +] +``` + +For facts with no arguments, omit `arg` entirely. + +### `output` format + +Each string is one line of stdout. Empty strings represent blank lines. +Use real command output whenever possible — copy from an actual system. + +```json +"output": [ + "##PYINFRA_FILE /etc/apt/sources.list", + "deb http://archive.ubuntu.com/ubuntu focal main", + "" +] +``` + +### `fact` format + +The `fact` field must exactly match what `fact.process(output)` returns. + +For facts that return lists of dataclasses with dict-like access (e.g. `AptRepo`), +serialize each item as a plain dict: + +```json +"fact": [ + { + "type": "deb", + "url": "http://archive.ubuntu.com/ubuntu", + "distribution": "focal", + "components": ["main"], + "options": {} + } +] +``` + +## File naming + +Name files descriptively after the scenario being tested: + +``` +tests/facts/apt.AptSources/ + sources.json # standard .list file with mixed entries + component_with_number.json # component name contains a digit + deb822_sources.json # .sources (deb822) format, multi-stanza + deb822_enabled_no.json # Enabled: no → excluded from results + deb822_multi_uris_suites.json # multiple URIs and Suites → cartesian expansion + deb822_malformed.json # missing required field → empty result + deb822_inline_gpg_key.json # Signed-By with inline PGP block (continuation lines) + deb822_trusted.json # Trusted: yes option +``` + +## Coverage checklist for a new fact + +| Scenario | File suffix | +|---|---| +| Typical happy-path output | `.json` | +| Empty output (nothing installed/configured) | `empty.json` | +| Malformed or unexpected output | `malformed.json` or `bad_output.json` | +| Output with comments / blank lines | `with_comments.json` | +| Args variation (if fact takes args) | `with_args.json` | + +## Multi-file parsing (##PYINFRA_FILE marker) + +When a fact uses the file-boundary marker pattern, each test output must include +the `##PYINFRA_FILE` marker lines, and the `command` field must match the exact +shell command emitted by the fact: + +```json +{ + "output": [ + "##PYINFRA_FILE /etc/apt/sources.list", + "deb http://example.com/debian bookworm main", + "" + ], + "command": "sh -c 'for f in /etc/apt/sources.list ...; do ...; echo \"##PYINFRA_FILE $f\"; ...; done'", + "fact": [...] +} +``` + +## Running + +```bash +uv run pytest tests/test_facts.py -k "" +``` + +## Tips + +- Copy `command` verbatim from `fact.command()` — run `python3 -c "from pyinfra.facts.X import Y; print(Y().command())"` if unsure. +- Use real-world output samples (from actual machines) for believable test data. +- For dataclass-returning facts with dict-like access, the serializer calls `to_json()` — make sure the expected dict matches that method's output exactly. +- When adding a new test file, run `uv run pytest tests/test_facts.py -k "ClassName"` to validate immediately. diff --git a/.claude/skills/writing-facts/SKILL.md b/.claude/skills/writing-facts/SKILL.md new file mode 100644 index 000000000..0e5adcf91 --- /dev/null +++ b/.claude/skills/writing-facts/SKILL.md @@ -0,0 +1,182 @@ +--- +name: writing-facts +--- + +Patterns and conventions for writing pyinfra facts. + +**Applies to:** `src/pyinfra/facts/**/*.py` + +Facts live in `src/pyinfra/facts/` and inherit from `FactBase`. They query remote system +state by running a shell command and parsing its output. + +## Module structure + +```python +from __future__ import annotations + +from typing_extensions import override + +from pyinfra.api import FactBase +``` + +## Fact anatomy + +```python +class MyFact(FactBase[ReturnType]): + """ + Returns a description of what is returned. + + .. code:: python + + {"key": "value"} + """ + + # Simple facts: just set a command string + command = "some-command --with-colons" + + # Or implement command() as a method when args are needed + @override + def command(self, arg1: str, arg2: str | None = None) -> str: + return f"some-command {arg1}" + + # Optional: declare what must be installed for this fact to run + @override + def requires_command(self, *args, **kwargs) -> str: + return "some-binary" + + # Default value when the fact returns nothing / command fails + default = dict # or list, or None + + @override + def process(self, output) -> ReturnType: + result = {} + for line in output: + if not line or line.startswith("#"): + continue + # parse line… + return result +``` + +## Key rules + +- **`output` is a list of strings**, one per line, already stripped of the trailing newline. +- **Return structured data** — dicts, lists, or primitives. Avoid returning raw strings. +- **`default`** must be a callable (e.g. `dict`, `list`) or `None`. It is called when the + command produces no output or the fact is not applicable. +- **`requires_command`** returns the binary that must exist on the remote host. Name it + exactly as it appears in `PATH` (without arguments). +- **`check_preconditions`** returns `None` (proceed) or a reason string (skip with explanation). + The framework is phase-aware: failures are silent during prepare and raised during execute. +- **`abstract = True`** marks a base class that is not directly usable as a fact. +- Use `from __future__ import annotations` and `@override` on every overridden method. + +## Facts with arguments + +When a fact accepts arguments, `command()` and `requires_command()` must accept the +same arguments: + +```python +class FileSha256(FactBase[str | None]): + @override + def command(self, path: str) -> str: + return f"sha256sum {path} 2>/dev/null | cut -d' ' -f1" + + @override + def requires_command(self, path: str) -> str: + return "sha256sum" +``` + +Call with: `host.get_fact(FileSha256, path="/etc/passwd")` + +## Parsing patterns + +```python +# Split on colon (GPG --with-colons, apt-cache, etc.) +bits = line.split(":") + +# Split on first occurrence only (key: value pairs) +key, _, value = line.partition(":") + +# Regex for complex formats +import re +match = re.match(r"^(\S+)\s+(\S+)", line) +if match: + name, version = match.groups() +``` + +## Shared base classes + +- **`GpgFactBase`** (`facts/gpg.py`) — base for facts that parse `gpg --with-colons` output. + Override `key_record_type` / `subkey_record_type` to target pub/sec keys. +- **`FactBase`** — the universal base. Generic parameter `FactBase[T]` documents return type. + +## Shell command robustness + +Prefer the framework hooks over shell-level guards: + +- **Missing binary** → declare `requires_command()`. The framework skips the fact + (returning the `default`) when the binary is absent, instead of running the command. +- **Phase / context guards** (e.g. only on certain OS, only when a feature is enabled) + → implement `check_preconditions()`. + +Reserve shell guards for cases the framework hooks cannot express: + +```python +# Suppress errors gracefully when files may not exist +command = "cat /etc/file 2>/dev/null || true" + +# Guard glob expansion when directory may be empty +command = "ls /etc/apt/sources.list.d/*.list 2>/dev/null || true" +``` + +Do **not** use the legacy `! command -v binary || real-command` pattern in new +facts — use `requires_command()` instead. A few older facts still embed it; do +not replicate them. + +## File-boundary marker pattern (multi-file parsing) + +When a single fact command reads multiple files, use a marker line to separate them: + +```python +@override +def command(self) -> str: + return ( + "sh -c '" + "for f in /etc/apt/sources.list.d/*.sources; do " + '[ -e "$f" ] || continue; ' + 'echo "##PYINFRA_FILE $f"; ' + 'cat "$f"; ' + "echo; " + "done'" + ) + +@override +def process(self, output): + current_file = None + buffer = [] + + def flush(): + nonlocal buffer, current_file + if current_file and buffer: + # parse buffer for current_file + pass + buffer = [] + + for line in output: + if line.startswith("##PYINFRA_FILE "): + flush() + current_file = line[15:].strip() + continue + buffer.append(line) + + flush() +``` + +Use `##PYINFRA_FILE` (not `##FILE`) as the marker prefix — it is unique enough to +avoid collision with legitimate file content. + +## After adding a fact + +1. Add fixtures in `tests/facts/./` (see the `writing-fact-tests` skill). +2. Register the module in `pyinfra-metadata.toml` so docs generation picks it up. +3. Run `./scripts/generate_facts_docs.py` to update docs. diff --git a/.claude/skills/writing-operation-tests/SKILL.md b/.claude/skills/writing-operation-tests/SKILL.md new file mode 100644 index 000000000..6bae0ed56 --- /dev/null +++ b/.claude/skills/writing-operation-tests/SKILL.md @@ -0,0 +1,150 @@ +--- +name: writing-operation-tests +--- + +JSON / YAML fixture format for operation unit tests. + +**Applies to:** `tests/operations/**/*.{json,yaml}` + +Operation tests live in `tests/operations/./` as JSON or YAML files. +Each file is one test case. The test runner (`tests/test_operations.py`) discovers +them automatically via the `TestGenerator` metaclass (which is ignored by pytest +collection to avoid warnings). + +**Prefer YAML for new fixtures.** To cover a new code path, add a fixture — do not +write a new Python test. + +## Fixture schema (JSON) + +```json +{ + "args": [], + "kwargs": { + "param_name": "value" + }, + "facts": { + "FactModule.FactClass": { + "arg1=value1": + } + }, + "commands": [ + "shell command 1", + "shell command 2" + ] +} +``` + +## Fixture schema (YAML) + +```yaml +args: + - positional_arg +kwargs: + param: value +facts: + module.FactClass: {} # dict of mock values keyed by object_id and attribute +commands: + - shell command that should be produced +``` + +### Fields + +| Field | Required | Description | +|---|---|---| +| `args` | Yes | Positional arguments to the operation (usually `[]`) | +| `kwargs` | Yes | Keyword arguments to the operation | +| `facts` | No | Facts to pre-populate in the cache | +| `commands` | Yes | Expected list of commands that the operation yields | +| `exception` | No | Class name (e.g. `"OperationError"`) or `{name, message}` of the expected exception | +| `noop_description` | No | Expected `host.noop()` description string | + +### `facts` format + +Keys use `"Module.Class"` notation matching the fact's Python path relative to +`pyinfra.facts`. Values are dicts keyed by `"arg=value"` argument strings: + +```json +"facts": { + "files.File": { + "path=/etc/apt/keyrings/test.gpg": null + }, + "files.Directory": { + "path=/etc/apt/keyrings": null + }, + "gpg.GpgKeyrings": { + "arg=[[\"/etc/apt/keyrings\"]]": {} + } +} +``` + +- `null` means the fact returned no result (e.g. file does not exist). +- Omitting a fact key means it will NOT be in the cache — the operation must not call it. +- For facts with no arguments, use `""` as the key: `{"": }`. + +### `commands` format + +Each element is either: +- A **string** — a shell command matched verbatim. +- A **dict** — a special command type, e.g. a file upload: + +```json +{"type": "put", "src": "local/path", "dest": "/remote/path", "mode": "644"} +``` + +### Error cases + +Use `"exception"` instead of `"commands"` when the operation should raise: + +```json +{ + "args": [], + "kwargs": {}, + "exception": "OperationError" +} +``` + +## File naming + +Name files descriptively after the scenario, using underscores: + +``` +tests/operations/gpg.key/ + local_file.json # install from local .asc + remote_url.json # install from HTTPS URL + keyserver_single.json # single key from keyserver + keyserver_multiple.json # multiple keys from keyserver + keyserver_idempotent.json # key already present → no commands + no_src_or_keyserver.json # missing args → OperationError + remove_file.json # present=False +``` + +## Idempotency tests + +Always add a `*_idempotent.json` or `*_existing.json` case where the facts show +the desired state is already reached, and `commands` is `[]`: + +```json +{ + "args": [], + "kwargs": {"name": "existing-package"}, + "facts": { + "deb.DebPackages": { + "": {"existing-package": {"version": "1.0"}} + } + }, + "commands": [] +} +``` + +## Running + +```bash +uv run pytest tests/test_operations.py -k "." +``` + +## Tips + +- Always include a test for the **happy path** (change needed) and **idempotent path** (no change). +- Include tests for **invalid inputs** that should raise `OperationError`. +- Reproduce real-world strings (e.g. exact `gpg` or `apt-get` command lines) — copy from + actual command output, not guessed. diff --git a/.claude/skills/writing-operations/SKILL.md b/.claude/skills/writing-operations/SKILL.md new file mode 100644 index 000000000..e187ae18c --- /dev/null +++ b/.claude/skills/writing-operations/SKILL.md @@ -0,0 +1,159 @@ +--- +name: writing-operations +--- + +Patterns and conventions for writing pyinfra operations. + +**Applies to:** `src/pyinfra/operations/**/*.py` + +Operations live in `src/pyinfra/operations/` and are decorated with `@operation()`. +They are **generator functions** that yield shell commands — never execute them directly. + +## Module structure + +```python +""" +Short module docstring (displayed in docs). +""" + +from __future__ import annotations + +from pyinfra import host +from pyinfra.api import OperationError, operation +from pyinfra.facts.something import SomeFact + +from . import files # reuse other operations via ._inner() +``` + +## Operation anatomy + +```python +@operation() +def my_operation( + required_arg: str, + optional_arg: str | None = None, + flag: bool = False, +): + """ + One-line summary of what the operation does. + + + required_arg: description of this argument + + optional_arg: description, mention the default behaviour + + flag: description of what True enables + """ + # 1. Query current state via facts (cached per deploy) + current_state = host.get_fact(SomeFact) + + # 2. Decide if a change is needed + if required_arg in current_state: + host.noop(f"{required_arg} already present") + return + + # 3. Yield shell command strings or Command objects + yield f"some-command {required_arg}" +``` + +## Key rules + +- **Always check facts first** — use `host.get_fact()` to read current state before yielding. +- **Use `host.noop()`** when no change is needed. Never return silently without a comment. +- **Yield, never execute** — operations are collected during prepare phase and run later. +- **Delegate to other operations** via `yield from other_op._inner(...)`, never call them directly. +- **`@operation(is_idempotent=False)`** only for commands that must always run (e.g. `exec`, `shell`). +- **Raise `OperationError`** for unrecoverable errors detected during prepare (wrong args, etc.). +- Avoid `subprocess`, `os.system`, or any direct execution inside an operation body. +- **Optional parameter defaults** must be `None`, not `""`. Older operations use `""` defaults; do not replicate this pattern. +- **Mirror the underlying CLI's flag names** when picking parameter names. Examples + of the convention to follow for new code: + - `systemd.service(daemon_reload=...)` → `systemctl daemon-reload` + - `git.repo(rebase=..., depth=..., recursive_submodules=...)` → `git pull --rebase`, + `git clone --depth`, `git submodule update --recursive` + + Convert the flag to `snake_case` and drop the leading dashes. Diverge only when + the CLI name is ambiguous out of context, conflicts with a + [global argument](../../../src/pyinfra/api/arguments.py) (e.g. `name`, `_sudo`), + or is genuinely misleading. Document the mapping in the docstring when the rename + is non-obvious. + +## Reusing operations internally + +```python +# Correct — use ._inner() to compose operations +yield from files.directory._inner(path="/etc/apt/keyrings", present=True) +yield from files.file._inner(path=dest, mode="644") + +# Wrong — never call the operation directly (it wraps in a new operation context) +files.directory(path="/etc/apt/keyrings", present=True) # DON'T DO THIS +``` + +## Temporary files and directories + +```python +# Use host helpers — they produce unique names per host/run +temp_file = host.get_temp_filename("prefix") +temp_dir = host.get_temp_filename("gpg-keyserver") +``` + +## Shell commands and safety + +User-supplied values must be composed using `StringCommand` + `QuoteString` / `MaskString` +from `pyinfra.api`. Do not use plain string formatting (e.g. `"rm -f {}".format(path)`) +for user-controlled values. + +```python +# Simple string +yield "apt-get update" + +# f-string for variable interpolation (quote user input) +yield f'gpg --dearmor -o "{dest}" "{src}"' + +# Multi-step pipeline: use && to chain +yield f'curl -fsSL "{url}" | gpg --dearmor -o "{dest}"' + +# Use StringCommand for pre-built command objects +from pyinfra.api import StringCommand, QuoteString +yield StringCommand("rm", "-f", QuoteString(user_path)) +``` + +## Error handling + +```python +# Raise OperationError for invalid arguments (detected at prepare time) +if src is None and keyserver is None: + raise OperationError("Either src or keyserver must be provided") + +# Use assertions sparingly — prefer explicit OperationError with a message +``` + +## Docstring format + +pyinfra uses `+ param: description` bullets (parsed by +`scripts/generate_operations_docs.py`). Do not use Google/NumPy/Sphinx style — it +will silently break docs generation. + +```python +""" +Summary line. + ++ arg_name: description of argument ++ other_arg: description (mention default/None behaviour explicitly) + +Example section heading: + Relevant note about this group of args. + +.. warning:: + Deprecation or danger notice. +""" +``` + +## Helpers in `util/` + +- `src/pyinfra/operations/util/packaging.py` — `ensure_packages()` for generic package management +- `src/pyinfra/operations/util/docker.py` — helpers for Docker operations +- Add new helpers here when logic is shared across multiple operation modules. + +## After adding an operation + +1. Add fixtures in `tests/operations/./` (see the `writing-operation-tests` skill). +2. Register the module in `pyinfra-metadata.toml` so docs generation picks it up. +3. Run `./scripts/generate_operations_docs.py` to update docs. diff --git a/AGENTS.md b/AGENTS.md index 74a89ad2c..04f5706f5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -86,7 +86,19 @@ packages, not added to this repo. rather than explicit passing. **Concurrency** — Uses gevent greenlets for parallel host execution. `pyinfra_cli/main.py` -monkey-patches stdlib at startup. +monkey-patches stdlib at startup. Coverage uses gevent concurrency mode (configured in +`pyproject.toml`). + +**Command objects** (from `pyinfra.api`): +- `StringCommand("cmd", "arg", ...)` — basic shell command, accepts mixed strings and + `QuoteString` / `MaskString` parts. +- `FileUploadCommand` / `RsyncCommand` — file transfer commands. +- `FunctionCommand(func, args, kwargs)` — Python function executed at run time. +- `QuoteString("value")` — value to be shell-quoted (use for any user-controlled input). +- `MaskString("secret")` — value masked from logs. + +**Connector data** — Each connector defines typed data in a `connector_data_meta` dict +mapping keys to `DataMeta` objects. Operations / facts read it via `host.connector_data`. ### Adding Operations / Facts @@ -143,6 +155,24 @@ break docs generation. **Optional parameter defaults** — optional parameters must default to `None`, not `""`. Older operations in the codebase use `""` defaults; do not replicate this pattern. +### Detailed conventions + +In-depth, file-scoped patterns are packaged as Claude skills under +[`.claude/skills/`](.claude/skills/): + +- [`writing-operations`](.claude/skills/writing-operations/SKILL.md) — operation module + structure, `_inner()` composition, `host.get_temp_filename()`, error handling, helpers + in `src/pyinfra/operations/util/`. +- [`writing-facts`](.claude/skills/writing-facts/SKILL.md) — `FactBase` anatomy, + `requires_command` / `check_preconditions` / `default` / `abstract`, parsing patterns, + shell robustness, `##PYINFRA_FILE` multi-file marker, `GpgFactBase`. +- [`writing-operation-tests`](.claude/skills/writing-operation-tests/SKILL.md) — + full JSON / YAML fixture schema for `tests/operations/`, `facts` keyed by `arg=value`, + idempotency cases, error cases. +- [`writing-fact-tests`](.claude/skills/writing-fact-tests/SKILL.md) — + full JSON / YAML fixture schema for `tests/facts/`, `arg` multi-call, multi-file marker + handling, coverage checklist. + ## Branch Strategy PRs target the latest major branch (`3.x`). One branch per major version exists (`2.x`, `1.x`,