diff --git a/architecture/supported-subset.md b/architecture/supported-subset.md index c4c91d3..b1d8db3 100644 --- a/architecture/supported-subset.md +++ b/architecture/supported-subset.md @@ -95,6 +95,19 @@ emit nothing, so it is refused rather than silently dropped rather than a per-key decision to keep in sync. `x-` extension keys are exempt: their contents are arbitrary user payload compose2pod never reads. +The same rule holds **wherever a null can appear** — the healthcheck sub-keys, +`deploy.resources` and its `limits`/`reservations`, and the top-level +`networks:`/`volumes:`/`secrets:`/`configs:` blocks. compose2pod is a drop-in +replacement for `docker compose` on rootless runners: the file it converts is +the file the developer runs locally. So a document `docker compose` will not run +must not pass green here — accepting a null it refuses would emit a script for a +file that is already broken upstream, turning a hard error into a false green. +Parity on *refusal* is what the drop-in role demands; it is not parity for its +own sake, and the package keeps its documented divergences elsewhere. A null +*inside* a value is a different thing and stays accepted, because Docker accepts +it too: `environment: {KEY: null}` is host-passthrough, `labels: {KEY: null}` an +empty label. + - **Supported:** `image`, `build`, `command`, `entrypoint`, `environment`, `env_file`, `volumes`, `healthcheck`, `depends_on`, `networks`, `hostname`, `container_name`, `tmpfs`, `secrets`, `configs`, plus the declarative @@ -424,13 +437,15 @@ exception in this group, validated as an actual bool like which has no sub-second resolution, so `"500ms"` and `0` both poll once a second. - **`timeout`, `retries`, `start_period`:** each must be a number (int or - float), a string, or `null` when present. A mapping or list raises - rather than reaching its `--health-*` flag as a literal Python `repr()`. - **An explicit `null` scalar means unset** — its `--health-*` flag is - omitted entirely, keyed off the *value*, not key presence, so `timeout: - null` and an omitted `timeout` behave identically. This matches `docker - compose config`, and is the same treatment this package already gives a - null `environment`/`volumes`/`command` value elsewhere. + float) or a string. A mapping or list raises rather than reaching its + `--health-*` flag as a literal Python `repr()`. +- **A null raises in every healthcheck position** — `test`, `interval`, + `timeout`, `retries`, `start_period` — because `docker compose config` + refuses each. A bare `test:` would silently drop the healthcheck entirely; + a bare `timeout:` drops nothing on its own, but the document carrying it is + one `docker compose` will not run, and compose2pod is a drop-in replacement + for it — so passing CI green on such a file would be a false green. An + *omitted* key is a different thing and stays fine: podman's default applies. - **Extension fields:** any `x-`-prefixed healthcheck key is accepted and ignored silently. - Everything else raises. diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 3055ae6..8c2a9a5 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -23,6 +23,21 @@ DEPENDS_ON_CONDITIONS = {"service_started", "service_healthy", "service_completed_successfully"} +def _reject_null_healthcheck_values(name: str, healthcheck: dict[str, Any]) -> None: + """Refuse a null in any healthcheck position -- `docker compose config` refuses each. + + A bare `test:` would silently drop the healthcheck entirely. A bare `timeout:` + drops nothing by itself, but the document carrying it is one `docker compose` + will not run, and compose2pod is a drop-in replacement for it -- so emitting a + script for such a file would turn a hard error into a false green. An *omitted* + key is a different thing and stays fine: podman's default applies. + """ + for key in ("test", "interval", *_HEALTHCHECK_SCALAR_KEYS): + if key in healthcheck and healthcheck[key] is None: + msg = f"service {name!r}: healthcheck {key!r} must not be null" + raise UnsupportedComposeError(msg) + + def _validate_service_healthcheck(name: str, svc: dict[str, Any]) -> None: """Check healthcheck is a mapping with supported keys and a parseable interval.""" healthcheck = svc.get("healthcheck") @@ -42,6 +57,7 @@ def _validate_service_healthcheck(name: str, svc: dict[str, Any]) -> None: if key not in SUPPORTED_HEALTHCHECK_KEYS: msg = f"service {name!r}: unsupported healthcheck key '{key}'" raise UnsupportedComposeError(msg) + _reject_null_healthcheck_values(name, healthcheck) if "interval" in healthcheck: interval_seconds(healthcheck["interval"]) if "test" in healthcheck: @@ -50,7 +66,7 @@ def _validate_service_healthcheck(name: str, svc: dict[str, Any]) -> None: # again for the actual --health-cmd value at emit time). health_cmd(healthcheck["test"]) for key in _HEALTHCHECK_SCALAR_KEYS: - if key in healthcheck and healthcheck[key] is not None and not is_number(healthcheck[key]): + if key in healthcheck and not is_number(healthcheck[key]): msg = f"service {name!r}: healthcheck {key!r} must be a number or string" raise UnsupportedComposeError(msg) @@ -400,6 +416,18 @@ def _validate_depends_on(services: dict[str, Any]) -> None: raise UnsupportedComposeError(msg) +def _reject_null_top_level_blocks(compose: dict[str, Any]) -> None: + """Refuse a bare top-level block -- `docker compose config` refuses each. + + `services` has its own message ("no services defined"); `version`/`name` are + scalars, not blocks. + """ + for key in ("networks", "volumes", "secrets", "configs"): + if key in compose and compose[key] is None: + msg = f"top-level {key!r} must not be null" + raise UnsupportedComposeError(msg) + + def validate(compose: dict[str, Any]) -> list[str]: """Check the compose document against the supported subset. @@ -419,6 +447,7 @@ def validate(compose: dict[str, Any]) -> list[str]: if unknown_top: msg = f"unsupported top-level keys: {sorted(unknown_top)}" raise UnsupportedComposeError(msg) + _reject_null_top_level_blocks(compose) if "networks" in compose: warnings.append("ignoring top-level 'networks' (all services share the pod namespace)") if "volumes" in compose: diff --git a/compose2pod/resources.py b/compose2pod/resources.py index 0b2cce5..99c0172 100644 --- a/compose2pod/resources.py +++ b/compose2pod/resources.py @@ -17,8 +17,8 @@ def _check_number(name: str, field: str, value: Any) -> None: # noqa: ANN401 - def _validate_limits(name: str, svc: dict[str, Any], limits: Any) -> None: # noqa: ANN401 - Compose values are untyped - if limits is None: - return + # No `limits is None` escape: `_reject_null_block` refuses a null `limits:` + # upstream, so a null reaching here is a wrong shape like any other. if not isinstance(limits, dict): msg = f"service {name!r}: deploy.resources.limits must be a mapping" raise UnsupportedComposeError(msg) @@ -36,8 +36,7 @@ def _validate_limits(name: str, svc: dict[str, Any], limits: Any) -> None: # no def _validate_reservations(name: str, svc: dict[str, Any], reservations: Any) -> None: # noqa: ANN401 - Compose values are untyped - if reservations is None: - return + # No `reservations is None` escape -- see `_validate_limits`. if not isinstance(reservations, dict): msg = f"service {name!r}: deploy.resources.reservations must be a mapping" raise UnsupportedComposeError(msg) @@ -57,6 +56,19 @@ def _validate_reservations(name: str, svc: dict[str, Any], reservations: Any) -> raise UnsupportedComposeError(msg) +def _reject_null_block(name: str, path: str, parent: dict[str, Any], key: str) -> None: + """Refuse a null where a block of content belongs. + + `deploy: {resources: {limits: }}` is the limits' contents deleted, not a + request for no limits: emit would silently drop `--memory`/`--cpus` and say + nothing. A key that is simply *absent* is fine -- that is a document not + asking for limits at all. + """ + if key in parent and parent[key] is None: + msg = f"service {name!r}: '{path}' must not be null" + raise UnsupportedComposeError(msg) + + def validate_deploy(name: str, svc: dict[str, Any]) -> None: """Validate a service's deploy block: only deploy.resources, only mappable fields, no legacy conflicts.""" deploy = svc.get("deploy") @@ -70,12 +82,15 @@ def validate_deploy(name: str, svc: dict[str, Any]) -> None: if unknown: msg = f"service {name!r}: deploy: only 'resources' is supported (got {sorted(unknown)})" raise UnsupportedComposeError(msg) + _reject_null_block(name, "deploy.resources", deploy, "resources") resources = deploy.get("resources") if resources is None: return if not isinstance(resources, dict): msg = f"service {name!r}: deploy.resources must be a mapping" raise UnsupportedComposeError(msg) + _reject_null_block(name, "deploy.resources.limits", resources, "limits") + _reject_null_block(name, "deploy.resources.reservations", resources, "reservations") require_string_keys(f"service {name!r}: deploy.resources", resources) unknown = set(resources) - {"limits", "reservations"} if unknown: diff --git a/planning/changes/2026-07-14.08-nested-null-content-blocks.md b/planning/changes/2026-07-14.08-nested-null-content-blocks.md new file mode 100644 index 0000000..1c1e4e0 --- /dev/null +++ b/planning/changes/2026-07-14.08-nested-null-content-blocks.md @@ -0,0 +1,78 @@ +--- +summary: A null is refused in every nested and top-level position Docker refuses one (healthcheck sub-keys, deploy's resource blocks, the top-level networks/volumes/secrets/configs blocks), so a document docker compose would not run cannot pass CI green here. +--- + +# Design: Refuse a null wherever Docker refuses one + +## Summary + +`2026-07-14.07` matched Docker's null policy for *service* keys. It stopped +there. Inside `healthcheck` and `deploy`, and in the top-level blocks, a null is +still accepted and silently emits nothing. + +Carry the same rule down: **a null is refused wherever `docker compose config` +refuses one.** Measured, position by position. + +## Motivation + +compose2pod exists to run a Compose file on a rootless CI runner where +`docker compose` and `podman kube play` cannot. It is a **drop-in replacement**: +the file it converts is the same file the developer runs locally with +`docker compose up`. + +That makes accepting-what-Docker-refuses actively harmful, not merely lax: + +- **Docker refuses, compose2pod accepts** → the document is already broken in the + developer's real workflow. Converting it anyway does not rescue anything; it + means **CI goes green on a file `docker compose` would not run**. A false + green is worse than a hard error. +- **Docker accepts, compose2pod refuses** → a working file breaks. That is the + direction that must never happen. + +So the test is not "does this null drop behavior?" but "**would `docker compose` +run this file at all?**" If it would not, there is no point emitting a podman +script for it. + +Eight positions failed that test — accepted here, refused by +`docker compose config` (v5.1.2), each emitting nothing: + +- `healthcheck.test`, `healthcheck.interval`, `healthcheck.timeout`, + `healthcheck.retries`, `healthcheck.start_period` +- `deploy.resources`, `deploy.resources.limits`, `deploy.resources.reservations` +- top-level `networks:`, `volumes:`, `secrets:`, `configs:` + +## Design + +Each position rejects an explicit null, with a message naming the path. A key +that is simply *absent* stays fine — that is a document not asking for the thing +at all, which Docker also accepts. + +**This reverses `2026-07-13.10`'s null-scalar ruling** (a null +`healthcheck.timeout`/`retries`/`start_period` meant "unset", emitting no flag). +That ruling was taken on the stated premise that it matched +`docker compose config`. It does not: Docker refuses all three. The premise was +wrong, so the conclusion goes with it. A null scalar drops nothing on its own, +but the document carrying it is one Docker would not run — and shipping a green +CI result for such a file is the failure this change exists to prevent. + +## Non-goals + +- Not touching a null *inside* a value (`environment: {KEY: null}` is Compose's + host-passthrough; `labels: {KEY: null}` is an empty label). Docker accepts + both, so compose2pod must too. +- Not pursuing Docker parity as an end in itself. The project keeps its + documented divergences where they buy something (it never builds; `profiles` + is closure-authoritative). Parity on *refusal* is what the drop-in role + demands, so a file that cannot run under `docker compose` cannot pass here. + +## Testing + +`just test-ci` at 100%. Each of the twelve positions gets a test — the eight +newly refused, plus the absent-key cases that must stay accepted — so the rule is +pinned position by position rather than implied. + +## Risk + +- **A document with a bare `timeout:` or `networks:` now raises** where it + previously ran. `docker compose` already refuses it, so the file was never + runnable in the workflow compose2pod serves; it was passing CI on a lie. diff --git a/tests/test_parsing.py b/tests/test_parsing.py index dd55f6d..1e09921 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -371,20 +371,6 @@ def test_healthcheck_scalars_accept_ints_and_strings(self) -> None: } assert validate(compose) == [] - def test_healthcheck_scalars_accept_explicit_null(self) -> None: - # A null scalar is treated as unset (see emit._health_flags), same - # ruling `environment`/`volumes`/`command` already get for a null - # value -- it must not raise at the gate. - compose = { - "services": { - "app": { - "image": "x", - "healthcheck": {"test": "true", "retries": None, "timeout": None, "start_period": None}, - } - } - } - assert validate(compose) == [] - def test_healthcheck_retries_mapping_rejected_at_gate(self) -> None: # Used to be silently accepted and mis-emitted as the literal # --health-retries "{'a': 1}". @@ -960,3 +946,52 @@ def test_null_extension_field_is_accepted(self) -> None: def test_absent_key_is_not_a_null_key(self) -> None: # The check is about a key present with no value, not a key that is missing. assert validate({"services": {"app": {"image": "x"}}}) == [] + + +class TestNullContentBlocks: + """A null is refused wherever `docker compose config` refuses one. + + compose2pod is a drop-in replacement for `docker compose` on rootless + runners: the file it converts is the file the developer runs locally. A + document Docker will not run must not pass CI green here, so accepting a + null it refuses would ship a false green. Verdicts measured against + `docker compose config` v5.1.2, position by position. + """ + + HEALTHCHECK_SCALARS = ("interval", "timeout", "retries", "start_period") + TOP_LEVEL_BLOCKS = ("networks", "volumes", "secrets", "configs") + + @pytest.mark.parametrize("key", HEALTHCHECK_SCALARS) + def test_null_healthcheck_scalar_is_refused(self, key: str) -> None: + # Reverses the earlier "null scalar means unset" ruling: that was taken on + # the premise it matched Docker, and Docker refuses all four. + healthcheck = {"test": ["CMD", "true"], key: None} + with pytest.raises(UnsupportedComposeError, match=f"healthcheck '{key}' must not be null"): + validate({"services": {"app": {"image": "x", "healthcheck": healthcheck}}}) + + @pytest.mark.parametrize("key", TOP_LEVEL_BLOCKS) + def test_null_top_level_block_is_refused(self, key: str) -> None: + with pytest.raises(UnsupportedComposeError, match=f"top-level '{key}' must not be null"): + validate({"services": {"app": {"image": "x"}}, key: None}) + + def test_null_healthcheck_test_is_refused(self) -> None: + # Used to emit no --health-cmd at all: the check silently evaporated. + with pytest.raises(UnsupportedComposeError, match="healthcheck 'test' must not be null"): + validate({"services": {"app": {"image": "x", "healthcheck": {"test": None, "interval": "1s"}}}}) + + def test_null_deploy_resources_is_refused(self) -> None: + with pytest.raises(UnsupportedComposeError, match=r"'deploy\.resources' must not be null"): + validate({"services": {"app": {"image": "x", "deploy": {"resources": None}}}}) + + def test_null_deploy_limits_is_refused(self) -> None: + # Used to emit no --memory/--cpus: the limits silently did not exist. + with pytest.raises(UnsupportedComposeError, match=r"'deploy\.resources\.limits' must not be null"): + validate({"services": {"app": {"image": "x", "deploy": {"resources": {"limits": None}}}}}) + + def test_null_deploy_reservations_is_refused(self) -> None: + with pytest.raises(UnsupportedComposeError, match=r"'deploy\.resources\.reservations' must not be null"): + validate({"services": {"app": {"image": "x", "deploy": {"resources": {"reservations": None}}}}}) + + def test_absent_blocks_are_not_null_blocks(self) -> None: + assert validate({"services": {"app": {"image": "x", "deploy": {"resources": {}}}}}) == [] + assert validate({"services": {"app": {"image": "x", "healthcheck": {"test": ["CMD", "true"]}}}}) == [] diff --git a/tests/test_resources.py b/tests/test_resources.py index d13b024..04fd319 100644 --- a/tests/test_resources.py +++ b/tests/test_resources.py @@ -34,7 +34,13 @@ def test_unsupported_resources_section_rejected(self) -> None: validate_deploy("app", _svc({"resources": {"foo": {}}})) def test_resources_absent_is_noop(self) -> None: - validate_deploy("app", _svc({"resources": None})) + # Genuinely absent -- a document not asking for resources at all. + validate_deploy("app", _svc({})) + + def test_null_resources_is_refused(self) -> None: + # `resources:` with its contents deleted. docker compose config refuses it. + with pytest.raises(UnsupportedComposeError, match=r"'deploy.resources' must not be null"): + validate_deploy("app", _svc({"resources": None})) def test_resources_not_mapping_rejected(self) -> None: with pytest.raises(UnsupportedComposeError, match=r"deploy.resources must be a mapping"): @@ -89,11 +95,14 @@ def test_reservation_memory_conflict_rejected(self) -> None: with pytest.raises(UnsupportedComposeError, match=match): validate_deploy("app", _svc(deploy, mem_reservation="256m")) - def test_null_limits_is_noop(self) -> None: - validate_deploy("app", _svc({"resources": {"limits": None}})) + def test_null_limits_is_refused(self) -> None: + # Used to emit no --memory/--cpus at all: the limits silently did not exist. + with pytest.raises(UnsupportedComposeError, match=r"'deploy.resources.limits' must not be null"): + validate_deploy("app", _svc({"resources": {"limits": None}})) - def test_null_reservations_is_noop(self) -> None: - validate_deploy("app", _svc({"resources": {"reservations": None}})) + def test_null_reservations_is_refused(self) -> None: + with pytest.raises(UnsupportedComposeError, match=r"'deploy.resources.reservations' must not be null"): + validate_deploy("app", _svc({"resources": {"reservations": None}})) def test_deploy_mixed_type_unknown_keys_do_not_crash_raw(self) -> None: # sorted(unknown) used to crash raw (TypeError: '<' not supported