From ed16571becff8f7f8f919001de4f262dc7e84bd9 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Tue, 14 Jul 2026 20:22:49 +0300 Subject: [PATCH] fix: refuse a null wherever docker compose 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. That makes accepting-what-Docker-refuses actively harmful rather than merely lax -- the document is already broken in the real workflow, so converting it anyway means CI goes green on a file docker compose would not run. A false green is worse than a hard error. So the test is not "does this null drop behavior?" but "would docker compose run this file at all?". Eight positions failed it, accepted here and refused by docker compose config, each emitting nothing: healthcheck.test / .interval / .timeout / .retries / .start_period deploy.resources / .limits / .reservations top-level networks: / volumes: / secrets: / configs: This reverses changes/2026-07-13.10's null-scalar ruling (a null healthcheck timeout/retries/start_period meant "unset"). 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 INSIDE a value is untouched and still accepted, because Docker accepts it too: environment: {KEY: null} is host-passthrough, labels: {KEY: null} an empty label. --- architecture/supported-subset.md | 29 +++++-- compose2pod/parsing.py | 31 +++++++- compose2pod/resources.py | 23 +++++- ...026-07-14.08-nested-null-content-blocks.md | 78 +++++++++++++++++++ tests/test_parsing.py | 63 +++++++++++---- tests/test_resources.py | 19 +++-- 6 files changed, 212 insertions(+), 31 deletions(-) create mode 100644 planning/changes/2026-07-14.08-nested-null-content-blocks.md 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