diff --git a/CONTEXT.md b/CONTEXT.md index e54ba09..b39118c 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -48,8 +48,10 @@ The target service and everything reachable from it through `depends_on` closure joins the pod, so only the closure is emitted, only its hostnames are resolvable, and pod-level options (`dns`, `sysctls`, `extra_hosts`) are unioned and conflict-checked across it and nothing else. A service outside the closure never runs, -which is why `profiles` is inert and why validation is closure-scoped rather than -document-wide. +which is why `profiles` is inert. Validation is not scoped to it, though: the dependency +graph is checked document-wide (`graph.validate_graph`), because whether Docker rejects a +document cannot depend on which service the caller happened to target +([#87](https://github.com/modern-python/compose2pod/issues/87)). **Rule one / rule two**: The two directions of the Docker-rejection parity rule diff --git a/compose2pod/graph.py b/compose2pod/graph.py index 2678fd4..98a8e9a 100644 --- a/compose2pod/graph.py +++ b/compose2pod/graph.py @@ -154,28 +154,62 @@ def hostnames(services: dict[str, Any]) -> list[str]: return names -def startup_order(services: dict[str, Any], target: str) -> list[str]: - """Dependency closure of target in start order (dependencies first, target last).""" - if target not in services: - msg = f"target service '{target}' not found" - raise UnsupportedComposeError(msg) +def _walk(services: dict[str, Any], roots: list[str]) -> list[str]: + """Depth-first walk from `roots`, in start order (dependencies first, root last). + + One walker for both callers below: the closure a `--target` starts, and the + whole-document pass that validates it. Both need the same two refusals -- a + dependency naming no service, and a cycle -- so both come from here rather + than from two implementations that can drift apart in what they refuse. + + `declared_by` is the service whose `depends_on` reached `name`, which is what + the unknown-dependency message needs: a whole-document walk refuses a typo in + a service the user never asked to run, so the refusal has to say where it is. + A root is passed as its own declarer and never uses it -- every root here is + already a key of `services`. + """ order: list[str] = [] state: dict[str, str] = {} - def visit(name: str) -> None: + def visit(name: str, declared_by: str) -> None: if state.get(name) == "visiting": msg = f"dependency cycle involving '{name}'" raise UnsupportedComposeError(msg) if state.get(name) == "done": return if name not in services: - msg = f"unknown dependency '{name}'" + msg = f"service {declared_by!r}: unknown dependency {name!r}" raise UnsupportedComposeError(msg) state[name] = "visiting" for dep in depends_on(services[name]): - visit(dep) + visit(dep, name) state[name] = "done" order.append(name) - visit(target) + for root in roots: + visit(root, root) return order + + +def startup_order(services: dict[str, Any], target: str) -> list[str]: + """Dependency closure of target in start order (dependencies first, target last).""" + if target not in services: + msg = f"target service '{target}' not found" + raise UnsupportedComposeError(msg) + return _walk(services, [target]) + + +def validate_graph(services: dict[str, Any]) -> None: + """Refuse a dependency graph naming a service the document does not define, or a cycle. + + Walks from every service, not from the `--target`, which is the difference + that closes issue 87. `docker compose config` validates the whole document + ("depends on undefined service", "dependency cycle detected") and a document + it refuses is one compose2pod must refuse (ADR-0006), even where the broken + service is one no target reaches and the generated script would never start. + + The cost is the reason the issue stayed open: a typo in a service nobody + targets now refuses the whole file, for every target in it. That is Docker's + own behaviour, and the hard rule leaves no room to keep the difference. + """ + _walk(services, list(services)) diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 1fb8a52..94f198d 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -6,7 +6,7 @@ from compose2pod import podman, stores, values from compose2pod.exceptions import UnsupportedComposeError -from compose2pod.graph import depends_on, hostnames +from compose2pod.graph import depends_on, hostnames, validate_graph from compose2pod.healthcheck import has_healthcheck, health_cmd, interval_seconds from compose2pod.keys import ( SERVICE_KEYS, @@ -1020,13 +1020,21 @@ def _sweep_document(compose: dict[str, Any]) -> None: def _validate_depends_on(services: dict[str, Any]) -> None: - """Cross-service depends_on checks: known conditions, service_healthy needs a healthcheck.""" + """Cross-service depends_on checks: the graph resolves, conditions are known, service_healthy is met. + + `validate_graph` runs first for a reason beyond ordering: it refuses every + dependency naming a service the document does not define, so `services[dep]` + below needs no membership guard. That guard used to be there and used to + matter -- a `service_healthy` dependency on a missing service was simply + skipped, which is the closure-scoped hole issue 87 closed. + """ + validate_graph(services) for name, svc in services.items(): for dep, condition in depends_on(svc).items(): if condition not in DEPENDS_ON_CONDITIONS: msg = f"service {name!r}: depends_on {dep!r} has unsupported condition {condition!r}" raise UnsupportedComposeError(msg) - if condition == "service_healthy" and dep in services and not has_healthcheck(services[dep]): + if condition == "service_healthy" and not has_healthcheck(services[dep]): msg = f"service {name!r}: depends on {dep!r} (service_healthy) but {dep!r} has no healthcheck" raise UnsupportedComposeError(msg) diff --git a/docs/adr/0006-docker-rejection-parity.md b/docs/adr/0006-docker-rejection-parity.md index d7d59ba..962a3d4 100644 --- a/docs/adr/0006-docker-rejection-parity.md +++ b/docs/adr/0006-docker-rejection-parity.md @@ -57,15 +57,19 @@ named volume) ([#114](https://github.com/modern-python/compose2pod/issues/114)), `mode` is refused at the other end of the range, where podman 6.0.1's `crun` will not mount it. The `integration` job pins `ubuntu-24.04` for the same reason: it is the runner that ships the floor, and on a newer one the job would measure a podman no user of the floor has. -Two residuals are open by design. `depends_on` errors among services outside the target's closure -are accepted here and rejected by Docker -([#87](https://github.com/modern-python/compose2pod/issues/87)): the one place the hard rule is -knowingly broken, so it is executed rather than described. `tests/conformance/corpus_residual/` -holds both documents, the summary prints them, and the test fails when a residual *closes*, since a -catalogue nobody re-runs goes stale in the direction that looks green. `assert_rule` still raises -for every document outside that directory, so the rule stays hard everywhere it is not deliberately -suspended. The other residual is the drive-qualified *bind* (`C:\data:/var`), a limitation rather -than rule two, since podman mounts that source through `--mount` and only the short `-v` spec +The hard rule has no exceptions left. It had two, and both were the same one: a `depends_on` +naming an undefined service, and a dependency cycle, among services outside the target's closure +were rejected by Docker and accepted here +([#87](https://github.com/modern-python/compose2pod/issues/87)), because both checks lived in the +closure walk `--target` drives and a service nothing targets is never walked. `graph.validate_graph` +walks from every service instead, so the verdict on the *document* no longer depends on which +service the caller asked to run -- which is Docker's own behaviour, and the only reading the hard +rule allows. The price is the reason the issue stayed open rather than something the fix hides: one +service's typo now refuses the file for every target in it. Both documents moved out of a residual +catalogue and into `tests/conformance/corpus/`, where `assert_rule` -- which raises on exactly that +combination -- is what holds them closed, so the rule is hard everywhere with nothing suspended. +What remains open is a limitation rather than a residual: the drive-qualified *bind* +(`C:\data:/var`), since podman mounts that source through `--mount` and only the short `-v` spec cannot spell it -- the long form already emits `--mount`, so the capability is reachable today and only the short spelling is missing ([#111](https://github.com/modern-python/compose2pod/issues/111)). diff --git a/tests/conformance/conftest.py b/tests/conformance/conftest.py index 5efee4e..dc9b7e2 100644 --- a/tests/conformance/conftest.py +++ b/tests/conformance/conftest.py @@ -34,12 +34,6 @@ # global so it is unambiguously one collector per pytest run, not one per import. _OVER_REJECTIONS: pytest.StashKey[list[str]] = pytest.StashKey() -# Every catalogued rule-one residual confirmed this run, as `` labels. -# Kept apart from the over-rejections: an over-rejection is allowed by the rule, while -# a residual is the rule being broken on purpose (issue 87), and reading them in one -# list would blur the two directions the whole harness exists to keep apart. -_RESIDUALS: pytest.StashKey[list[str]] = pytest.StashKey() - @pytest.hookimpl(tryfirst=True) def pytest_collection_modifyitems(items: "list[pytest.Item]") -> None: @@ -52,11 +46,10 @@ def pytest_collection_modifyitems(items: "list[pytest.Item]") -> None: def pytest_configure(config: pytest.Config) -> None: """Create this run's over-rejection collector before any conformance test executes.""" config.stash[_OVER_REJECTIONS] = [] - config.stash[_RESIDUALS] = [] def pytest_terminal_summary(terminalreporter: pytest.TerminalReporter) -> None: - """Print every over-reject verdict and every confirmed residual collected this run. + """Print every over-reject verdict collected this run. Over-rejections never fail the build (see `assert_rule`); this is the harness's only way of keeping them visible, which is what the tracked-limitation issues @@ -64,15 +57,6 @@ def pytest_terminal_summary(terminalreporter: pytest.TerminalReporter) -> None: case for `just test-ci` (the conformance suite is deselected there and this hook never runs a probe, so the list stays empty). """ - residuals = terminalreporter.config.stash.get(_RESIDUALS, []) - if residuals: - terminalreporter.section("conformance: rule-one residuals (docker rejects, compose2pod accepts)") - for label in residuals: - terminalreporter.write_line(label) - terminalreporter.write_line( - f"{len(residuals)} residual(s) -- the hard rule, knowingly broken; " - "https://github.com/modern-python/compose2pod/issues/87" - ) over_rejections = terminalreporter.config.stash.get(_OVER_REJECTIONS, []) if not over_rejections: return @@ -143,7 +127,9 @@ def assert_rule(tmp_path: Path, request: pytest.FixtureRequest) -> Callable[[dic tracked issue; every 'over-reject' verdict is also recorded under the calling test's id for `pytest_terminal_summary` to print at the end of the run). Raises AssertionError on the one forbidden combination: Docker refuses and we - accept. + accept. That combination has no catalogued exceptions: issue 87's two residuals + were the last, and closing it moved both documents into `corpus/`, where this + fixture is what holds them closed. """ def _assert(compose: dict[str, Any]) -> str: @@ -161,24 +147,3 @@ def _assert(compose: dict[str, Any]) -> str: return "over-reject" return _assert - - -@pytest.fixture -def assert_residual(tmp_path: Path, request: pytest.FixtureRequest) -> Callable[[dict[str, Any]], None]: - """Assert one document still breaks rule one, and record it for the run's summary. - - The inverse of `assert_rule`, which raises on this combination: here it is the - expected result, and either half changing is what fails. A residual that closed - leaves a file claiming a breach that no longer exists, which is worse than no - catalogue at all. - """ - - def _assert(compose: dict[str, Any]) -> None: - text = yaml.safe_dump(compose, sort_keys=False) - assert not _docker_accepts(text, tmp_path), "docker now accepts this document, so it documents no residual" - assert _compose2pod_accepts(text, tmp_path), ( - "compose2pod now rejects this document -- the residual is closed, delete the file" - ) - request.config.stash[_RESIDUALS].append(request.node.nodeid) - - return _assert diff --git a/tests/conformance/corpus/depends_on_cycle_outside_closure.yaml b/tests/conformance/corpus/depends_on_cycle_outside_closure.yaml new file mode 100644 index 0000000..d192fc8 --- /dev/null +++ b/tests/conformance/corpus/depends_on_cycle_outside_closure.yaml @@ -0,0 +1,14 @@ +# Docker rejects the document ("dependency cycle detected") and so does compose2pod, +# though `app` is the target and its closure never reaches `a` or `b`: the graph is +# validated document-wide, independently of --target (issue 87). +services: + app: + image: nginx + a: + image: nginx + depends_on: + - b + b: + image: nginx + depends_on: + - a diff --git a/tests/conformance/corpus/depends_on_ghost_outside_closure.yaml b/tests/conformance/corpus/depends_on_ghost_outside_closure.yaml new file mode 100644 index 0000000..7ca51d4 --- /dev/null +++ b/tests/conformance/corpus/depends_on_ghost_outside_closure.yaml @@ -0,0 +1,10 @@ +# Docker rejects the document ("depends on undefined service") and so does compose2pod, +# though `app` is the target and its closure never reaches `other`: the graph is +# validated document-wide, independently of --target (issue 87). +services: + app: + image: nginx + other: + image: nginx + depends_on: + - ghost diff --git a/tests/conformance/corpus_residual/depends_on_cycle_outside_closure.yaml b/tests/conformance/corpus_residual/depends_on_cycle_outside_closure.yaml deleted file mode 100644 index 7879cdb..0000000 --- a/tests/conformance/corpus_residual/depends_on_cycle_outside_closure.yaml +++ /dev/null @@ -1,13 +0,0 @@ -# Docker rejects the document ("dependency cycle detected"); compose2pod accepts it, -# because `app` is the target and its closure never reaches `a` or `b`. Residual, issue 87. -services: - app: - image: nginx - a: - image: nginx - depends_on: - - b - b: - image: nginx - depends_on: - - a diff --git a/tests/conformance/corpus_residual/depends_on_ghost_outside_closure.yaml b/tests/conformance/corpus_residual/depends_on_ghost_outside_closure.yaml deleted file mode 100644 index c3fec01..0000000 --- a/tests/conformance/corpus_residual/depends_on_ghost_outside_closure.yaml +++ /dev/null @@ -1,9 +0,0 @@ -# Docker rejects the document ("depends on undefined service"); compose2pod accepts it, -# because `app` is the target and its closure never reaches `other`. Residual, issue 87. -services: - app: - image: nginx - other: - image: nginx - depends_on: - - ghost diff --git a/tests/conformance/test_residuals.py b/tests/conformance/test_residuals.py deleted file mode 100644 index 367df74..0000000 --- a/tests/conformance/test_residuals.py +++ /dev/null @@ -1,33 +0,0 @@ -"""The documents where rule one is knowingly broken, probed instead of described. - -ADR-0006 calls `accepted(compose2pod) ⊆ accepted(docker)` hard, and issue 87 records two -exceptions as a deliberate ruling: a `depends_on` naming an undefined service, and a -dependency cycle, both on services outside the `--target`'s closure, which `startup_order` -never walks. `assert_rule` raises on exactly that combination, so neither could live in -`corpus/` -- and so neither was measured anywhere, which left the one hard rule's known -breach resting on prose. These files are that breach, executed. -""" - -from collections.abc import Callable -from pathlib import Path -from typing import Any - -import pytest -import yaml - - -_RESIDUAL_CORPUS = sorted((Path(__file__).parent / "corpus_residual").glob("*.yaml")) - - -@pytest.mark.parametrize("path", _RESIDUAL_CORPUS, ids=lambda p: p.stem) -def test_a_catalogued_residual_still_breaks_rule_one( - path: Path, assert_residual: Callable[[dict[str, Any]], None] -) -> None: - """A file here fails when the residual *closes*, which is when it should be deleted. - - Tolerating a catalogued exception is not the point -- an entry nobody re-runs goes - stale in the direction that looks green, which is how issue 86's unmeasured claim - survived long enough to ship. Cataloguing it is only worth anything if the catalogue - is wrong when the world changes. - """ - assert_residual(yaml.safe_load(path.read_text())) diff --git a/tests/test_graph.py b/tests/test_graph.py index a676b73..b3d13e9 100644 --- a/tests/test_graph.py +++ b/tests/test_graph.py @@ -1,7 +1,7 @@ import pytest from compose2pod.exceptions import UnsupportedComposeError -from compose2pod.graph import depends_on, hostnames, startup_order +from compose2pod.graph import depends_on, hostnames, startup_order, validate_graph from compose2pod.parsing import validate @@ -228,3 +228,33 @@ def test_diamond_dependency_visits_shared_service_once(self) -> None: assert order.index("c") < order.index("a") assert order.index("c") < order.index("b") assert order[-1] == "target" + + +class TestValidateGraph: + def test_well_formed_document_passes(self, chats_compose: dict) -> None: + assert validate_graph(chats_compose["services"]) is None + + def test_unknown_dependency_outside_any_closure_raises(self) -> None: + services = { + "app": {"image": "x"}, + "other": {"image": "x", "depends_on": ["ghost"]}, + } + with pytest.raises(UnsupportedComposeError, match=r"service 'other': unknown dependency 'ghost'"): + validate_graph(services) + + def test_cycle_outside_any_closure_raises(self) -> None: + services = { + "app": {"image": "x"}, + "a": {"image": "x", "depends_on": ["b"]}, + "b": {"image": "x", "depends_on": ["a"]}, + } + with pytest.raises(UnsupportedComposeError, match=r"dependency cycle involving 'a'"): + validate_graph(services) + + def test_a_service_reached_from_two_roots_is_walked_once(self) -> None: + services = { + "shared": {"image": "x"}, + "a": {"image": "x", "depends_on": ["shared"]}, + "b": {"image": "x", "depends_on": ["shared"]}, + } + assert validate_graph(services) is None diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 6fe0381..4f1ae88 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -433,11 +433,30 @@ def test_service_healthy_dependency_with_healthcheck_is_accepted(self) -> None: } assert validate(compose) == [] - def test_service_healthy_dependency_on_unknown_service_is_out_of_scope(self) -> None: - assert ( + def test_service_healthy_dependency_on_unknown_service_raises(self) -> None: + with pytest.raises(UnsupportedComposeError, match=r"service 'app': unknown dependency 'ghost'"): validate({"services": {"app": {"image": "x", "depends_on": {"ghost": {"condition": "service_healthy"}}}}}) - == [] - ) + + def test_unknown_dependency_outside_the_target_closure_raises_at_gate(self) -> None: + compose = { + "services": { + "app": {"image": "x"}, + "other": {"image": "x", "depends_on": ["ghost"]}, + } + } + with pytest.raises(UnsupportedComposeError, match=r"service 'other': unknown dependency 'ghost'"): + validate(compose) + + def test_dependency_cycle_outside_the_target_closure_raises_at_gate(self) -> None: + compose = { + "services": { + "app": {"image": "x"}, + "a": {"image": "x", "depends_on": ["b"]}, + "b": {"image": "x", "depends_on": ["a"]}, + } + } + with pytest.raises(UnsupportedComposeError, match=r"dependency cycle involving 'a'"): + validate(compose) def test_unknown_depends_on_condition_raises(self) -> None: compose = {