From f300f964ca9581a89243537b7aa5ee992f889210 Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sun, 20 Sep 2026 17:19:28 +0300 Subject: [PATCH] feat: tell the user podman's reason in the four refusals that kept it ADR-0006 says the rule-two/limitation distinction is held apart in the refusal messages. Four messages stated a rule and kept its reason in the source, so a reader learned what compose2pod wants, not why podman cannot have it. The long-form `type` pair needed a split rather than a suffix. `volume 'type' must be one of [...]` fires both for `cluster`/`npipe`, which docker accepts (measured, docker compose config v5.1.2) and podman cannot express, and for a misspelling like `bnid`, which docker rejects too. The first is rule two and the second is rule one, so giving the whole message a podman clause would have attached a podman reason to a refusal podman has nothing to do with. Both verdicts are now pinned in the conformance corpus, since citing podman is only legitimate while docker itself takes the document. The list is held empty from the other side: a `REFUSALS` row exists because podman will not make the mount, so its refusal's reason is podman's, and a row that names no claim marks a message keeping that reason to itself. The claim-coverage gate could not see those four -- there was no claim to scan -- and now fails on the next one. Closes #121 --- compose2pod/parsing.py | 22 +++++++++------ docs/adr/0006-docker-rejection-parity.md | 11 +++++--- .../corpus/volume_long_form_cluster_type.yaml | 7 +++++ .../volume_long_form_misspelled_type.yaml | 7 +++++ tests/conformance/test_corpus.py | 27 +++++++++++++++++++ tests/integration/refusals.py | 21 ++++++++++----- tests/test_parsing.py | 27 ++++++++++++++++++- tests/test_podman_claim_coverage.py | 19 ++++++++++++- 8 files changed, 122 insertions(+), 19 deletions(-) create mode 100644 tests/conformance/corpus/volume_long_form_cluster_type.yaml create mode 100644 tests/conformance/corpus/volume_long_form_misspelled_type.yaml diff --git a/compose2pod/parsing.py b/compose2pod/parsing.py index 5127272..71387f3 100644 --- a/compose2pod/parsing.py +++ b/compose2pod/parsing.py @@ -207,6 +207,7 @@ def _reject_drive_shaped_volume(name: str, volume: str) -> None: _VOLUME_LONG_TYPES = ("bind", "volume", "tmpfs", "image") +_DOCKER_ONLY_VOLUME_TYPES = ("cluster", "npipe") _VOLUME_LONG_KEYS = {"type", "source", "target", "read_only", "consistency"} # Docker's own per-type nested option map keys (measured, docker compose config # v5.1.2). `create_host_path`/`nocopy` are real docker keys, so they land here @@ -246,7 +247,10 @@ def _validate_service_volumes(name: str, svc: dict[str, Any]) -> None: _reject_drive_shaped_volume(name, volume) kind, _ = _classify_volume(volume) if kind == "anonymous" and not volume.startswith("/"): - msg = f"service {name!r}: anonymous volume '{volume}' must be an absolute path" + msg = ( + f"service {name!r}: anonymous volume '{volume}' must be an absolute path " + f"({podman.REFUSES_RELATIVE_CONTAINER_PATH})" + ) raise UnsupportedComposeError(msg) # A named or bind entry needs no further shape check here -- both are # accepted; podman creates a named volume implicitly on first @@ -260,8 +264,9 @@ def _validate_volume_long_form(name: str, entry: dict[str, Any]) -> None: type (bind/volume/tmpfs/image), source, target, read_only, consistency, plus the one nested option map matching `type` (a mismatched sub-map is refused -- a deliberate stricter-than-docker check; docker - accepts-and-ignores it). cluster/npipe types are refused (podman cannot - express them). + accepts-and-ignores it). cluster and npipe are split off first because + docker takes them and podman cannot express them, which is a different + refusal from a misspelling docker rejects too. `type` is validated before the unknown-key check (unlike every other field here) because the check itself needs `vtype` to know which sub-map key -- @@ -269,6 +274,9 @@ def _validate_volume_long_form(name: str, entry: dict[str, Any]) -> None: """ keys = require_string_keys(f"service {name!r}: volume", entry) vtype = entry.get("type") + if vtype in _DOCKER_ONLY_VOLUME_TYPES: + msg = f"service {name!r}: volume 'type: {vtype}' is not supported ({podman.CANNOT_EXPRESS})" + raise UnsupportedComposeError(msg) if vtype not in _VOLUME_LONG_TYPES: msg = f"service {name!r}: volume 'type' must be one of {list(_VOLUME_LONG_TYPES)}" raise UnsupportedComposeError(msg) @@ -281,11 +289,9 @@ def _validate_volume_long_form(name: str, entry: dict[str, Any]) -> None: msg = f"service {name!r}: volume 'target' must be a string" raise UnsupportedComposeError(msg) if not target.startswith("/") and not values.has_variable(target): - # podman rejects a relative --mount target for every type ("must be - # an absolute path"); docker accepts it. A ${VAR} target is - # host-dependent, so it is carved out like every other - # values.has_variable case in this file. - msg = f"service {name!r}: volume 'target' must be an absolute path" + # A ${VAR} target is host-dependent, so it is carved out like every + # other values.has_variable case in this file. + msg = f"service {name!r}: volume 'target' must be an absolute path ({podman.REFUSES_RELATIVE_CONTAINER_PATH})" raise UnsupportedComposeError(msg) _validate_volume_long_form_source(name, vtype, entry.get("source")) if "read_only" in entry and not values.is_bool_like(entry["read_only"]): diff --git a/docs/adr/0006-docker-rejection-parity.md b/docs/adr/0006-docker-rejection-parity.md index 4f56cee..d7d59ba 100644 --- a/docs/adr/0006-docker-rejection-parity.md +++ b/docs/adr/0006-docker-rejection-parity.md @@ -38,9 +38,14 @@ its clause from there, and `tests/test_podman_claim_coverage.py` pairs each site with the row that measures it, in both directions. The handle is the attribute name, not the wording, because prose is not a registry -- a message is assembled from an f-string, a shared preamble or a lookup table depending on the site. A refusal that makes no claim is out of scope by -construction, which is how `network_mode` needs no row; a refusal whose reason is podman's but whose -message keeps that to itself is invisible to the gate, and that list is -[#121](https://github.com/modern-python/compose2pod/issues/121). +construction, which is how `network_mode` needs no row. A refusal whose reason is podman's but +whose message keeps that to itself would be invisible to that gate, so the table closes it from +the other side: every `REFUSALS` row names a claim, because the table's premise is that podman +will not make the mount. That is what emptied +[#121](https://github.com/modern-python/compose2pod/issues/121)'s list, which is also why a +long-form `type: cluster` no longer refuses with the same message as a misspelled one -- docker +takes cluster and npipe, so refusing them is rule two and says so, while `bnid` is rule one and +podman has nothing to do with it. Verdicts are per version, and the supported range is stated rather than implied: the rulings here are measured against `docker compose config` v5.1.2 and podman 4.9.3, and compose2pod supports podman 4.9 and up. Rule two reads across that whole range. A form is accepted only where podman diff --git a/tests/conformance/corpus/volume_long_form_cluster_type.yaml b/tests/conformance/corpus/volume_long_form_cluster_type.yaml new file mode 100644 index 0000000..e2105af --- /dev/null +++ b/tests/conformance/corpus/volume_long_form_cluster_type.yaml @@ -0,0 +1,7 @@ +services: + app: + image: nginx + volumes: + - type: cluster + source: x + target: /d diff --git a/tests/conformance/corpus/volume_long_form_misspelled_type.yaml b/tests/conformance/corpus/volume_long_form_misspelled_type.yaml new file mode 100644 index 0000000..1e56a72 --- /dev/null +++ b/tests/conformance/corpus/volume_long_form_misspelled_type.yaml @@ -0,0 +1,7 @@ +services: + app: + image: nginx + volumes: + - type: bnid + source: x + target: /d diff --git a/tests/conformance/test_corpus.py b/tests/conformance/test_corpus.py index 17355bf..c06d43a 100644 --- a/tests/conformance/test_corpus.py +++ b/tests/conformance/test_corpus.py @@ -145,6 +145,33 @@ def test_volume_windows_drive_letter_bind_is_a_catalogued_over_rejection( assert assert_rule(yaml.safe_load(path.read_text())) == "over-reject" +def test_volume_long_form_cluster_type_is_a_catalogued_over_rejection( + assert_rule: Callable[[dict[str, Any]], str], +) -> None: + """Docker accepts `type: cluster`; podman has no such mount, so we refuse it -- rule two. + + Asserted rather than left to the generic corpus run for the usual reason: `over-reject` + is an allowed verdict, so the run stays green whichever way this file falls. What it + pins is the half of issue #121's split that only docker can answer -- the refusal cites + podman, and citing podman is only legitimate while docker itself takes the document. + """ + path = Path(__file__).parent / "corpus" / "volume_long_form_cluster_type.yaml" + assert assert_rule(yaml.safe_load(path.read_text())) == "over-reject" + + +def test_volume_long_form_misspelled_type_is_rejected_by_both( + assert_rule: Callable[[dict[str, Any]], str], +) -> None: + """The other half of the split: docker rejects a typo too, so podman is not the reason. + + `volume 'type' must be one of [...]` fires for both this and `cluster`, and the two are + refused for opposite reasons. Pinning the verdict keeps the message that claims nothing + about podman attached to the case where podman has nothing to do with it. + """ + path = Path(__file__).parent / "corpus" / "volume_long_form_misspelled_type.yaml" + assert assert_rule(yaml.safe_load(path.read_text())) == "both-reject" + + def test_volume_single_letter_source_is_a_catalogued_over_rejection( assert_rule: Callable[[dict[str, Any]], str], ) -> None: diff --git a/tests/integration/refusals.py b/tests/integration/refusals.py index 95c7c7d..8a6192d 100644 --- a/tests/integration/refusals.py +++ b/tests/integration/refusals.py @@ -30,9 +30,11 @@ A row measuring a refusal whose message draws a clause from `compose2pod/podman.py` names the `site` that makes the claim and the `claim` it makes, which is what -`tests/test_podman_claim_coverage.py` gates on. A row whose refusal makes no such claim -- -a relative target, an unsupported long-form `type` -- leaves both empty: the reason is -podman's, but the message keeps it to itself, which is issue #121's subject. +`tests/test_podman_claim_coverage.py` gates on. Every `REFUSALS` row names one, because the +table's own premise is that podman will not make the mount: a row that named none would mark +a message stating a rule while keeping podman's reason to itself, which is what #121 found +in four of them. Only a `LIMITATIONS` row leaves both empty, and only where the limit really +is the short form's rather than podman's. Four claims, four experiments. `network_mode` alone has no row: it is refused under ADR-0003, not rule two, and podman honours it (#115). The gate that every rule-two site @@ -127,12 +129,13 @@ def _one_volume(entry: "str | dict[str, Any]") -> dict[str, Any]: _NOT_ABSOLUTE = "podman refuses a container path that is not absolute" _SHORT_FORM_CANNOT_EMIT = "which the short form cannot emit" -_UNSUPPORTED_LONG_TYPE = "volume 'type' must be one of" REFUSALS: list[Refusal] = [ Refusal( id="anonymous-volume-relative-target", + site="parsing._validate_service_volumes", + claim="REFUSES_RELATIVE_CONTAINER_PATH", compose=_one_volume("a"), refusal_match="anonymous volume 'a' must be an absolute path", podman_argv=["--mount", "type=volume,dst=a"], @@ -167,6 +170,8 @@ def _one_volume(entry: "str | dict[str, Any]") -> dict[str, Any]: ), Refusal( id="long-form-relative-target", + site="parsing._validate_volume_long_form", + claim="REFUSES_RELATIVE_CONTAINER_PATH", compose=_one_volume({"type": "volume", "target": "rel"}), refusal_match="volume 'target' must be an absolute path", podman_argv=["--mount", "type=volume,dst=rel"], @@ -174,15 +179,19 @@ def _one_volume(entry: "str | dict[str, Any]") -> dict[str, Any]: ), Refusal( id="long-form-type-cluster", + site="parsing._validate_volume_long_form", + claim="CANNOT_EXPRESS", compose=_one_volume({"type": "cluster", "target": "/data"}), - refusal_match=_UNSUPPORTED_LONG_TYPE, + refusal_match="volume 'type: cluster' is not supported", podman_argv=["--mount", "type=cluster,dst=/data"], control_argv=_ANONYMOUS_CONTROL, ), Refusal( id="long-form-type-npipe", + site="parsing._validate_volume_long_form", + claim="CANNOT_EXPRESS", compose=_one_volume({"type": "npipe", "target": "/data"}), - refusal_match=_UNSUPPORTED_LONG_TYPE, + refusal_match="volume 'type: npipe' is not supported", podman_argv=["--mount", "type=npipe,dst=/data"], control_argv=_ANONYMOUS_CONTROL, ), diff --git a/tests/test_parsing.py b/tests/test_parsing.py index 4d63047..b6d368a 100644 --- a/tests/test_parsing.py +++ b/tests/test_parsing.py @@ -121,7 +121,7 @@ def test_long_volume_entry_rejects(self) -> None: # Each case drives one distinct reject branch (all six needed for 100% coverage). cases = [ ([5], r"volume entry must be a string or mapping"), - ([{"type": "cluster", "source": "x", "target": "/d"}], r"volume 'type' must be one of"), + ([{"type": "bnid", "source": "x", "target": "/d"}], r"volume 'type' must be one of"), ([{"type": "volume", "target": 5}], r"volume 'target' must be a string"), ([{"type": "bind", "target": "/d"}], r"bind volume 'source' must be a string"), ([{"type": "tmpfs", "source": "x", "target": "/t"}], r"tmpfs volume takes no 'source'"), @@ -151,6 +151,27 @@ def test_long_volume_relative_target_rejected(self) -> None: with pytest.raises(UnsupportedComposeError, match=r"volume 'target' must be an absolute path"): validate({"services": {"app": {"image": "x", "volumes": [entry]}}}) + def test_a_volume_type_docker_takes_and_podman_cannot_is_refused_with_podmans_reason(self) -> None: + # docker compose config v5.1.2 accepts cluster and npipe, so refusing them is rule two. + for vtype in ("cluster", "npipe"): + entry = {"type": vtype, "source": "x", "target": "/d"} + with pytest.raises( + UnsupportedComposeError, + match=rf"volume 'type: {vtype}' is not supported \(podman cannot express it\)", + ): + validate({"services": {"app": {"image": "x", "volumes": [entry]}}}) + + def test_a_volume_type_docker_rejects_too_is_refused_without_a_podman_reason(self) -> None: + # docker compose config v5.1.2 rejects a typo as well, so podman has nothing to do with it. + with pytest.raises(UnsupportedComposeError, match=r"volume 'type' must be one of") as refusal: + validate({"services": {"app": {"image": "x", "volumes": [{"type": "bnid", "target": "/d"}]}}}) + + assert "podman" not in str(refusal.value) + + def test_a_relative_long_form_target_is_refused_with_podmans_reason(self) -> None: + with pytest.raises(UnsupportedComposeError, match=r"podman refuses a container path that is not absolute"): + validate({"services": {"app": {"image": "x", "volumes": [{"type": "volume", "target": "rel"}]}}}) + def test_long_volume_variable_target_accepted(self) -> None: # A ${VAR}-carrying target is host-dependent -- accepted, matching # every other values.has_variable carve-out in parsing.py. @@ -268,6 +289,10 @@ def test_relative_anonymous_volume_raises(self) -> None: with pytest.raises(UnsupportedComposeError, match="absolute"): validate({"services": {"app": {"image": "x", "volumes": ["./cache"]}}}) + def test_a_relative_anonymous_volume_is_refused_with_podmans_reason(self) -> None: + with pytest.raises(UnsupportedComposeError, match=r"podman refuses a container path that is not absolute"): + validate({"services": {"app": {"image": "x", "volumes": ["./cache"]}}}) + def test_string_volumes_rejected_at_gate(self) -> None: # Used to be iterated character-wise: "/data:/data" reported the nonsense # "anonymous volume 'd'", and "/" was silently accepted as -v "/". diff --git a/tests/test_podman_claim_coverage.py b/tests/test_podman_claim_coverage.py index e377085..b3fa443 100644 --- a/tests/test_podman_claim_coverage.py +++ b/tests/test_podman_claim_coverage.py @@ -64,7 +64,8 @@ def test_every_claim_compose2pod_makes_about_podman_is_measured_by_a_row() -> No Out of scope by construction: a refusal that makes no claim here. `network_mode` is refused under docs/adr/0003-the-shared-namespace-decides-key-classification.md and podman honours it, so it has no claim and needs no row. A refusal whose reason *is* podman's but - whose message keeps that to itself is invisible to this gate, which is issue #121. + whose message keeps that to itself would be invisible to this gate, which is what the + invariant below covers from the other side. """ unmeasured = sorted(claim_sites(_PACKAGE) - _measured_pairs()) @@ -73,6 +74,22 @@ def test_every_claim_compose2pod_makes_about_podman_is_measured_by_a_row() -> No ) +def test_every_refusal_podman_agrees_with_tells_the_user_podmans_reason() -> None: + """INVARIANT: a rule-two refusal states the reason that makes it legitimate, not just the rule. + + A `REFUSALS` row exists because podman will not make the mount, so the refusal's reason is + podman's. A row with no claim therefore marks a message that states a rule while keeping + its reason in the source, which is what issue #121 found in four of them, and which the + gate above cannot see because there is no claim for it to scan. + + `LIMITATIONS` is where a refusal with no podman reason belongs: the drive-qualified bind + is the short form's own limit, and its message says so. + """ + silent = sorted(row.id for row in REFUSALS if not row.claim) + + assert silent == [], "\n".join(f"{row} refuses for podman's reason without telling the user" for row in silent) + + def test_every_row_naming_a_claim_names_one_the_source_still_makes() -> None: """INVARIANT: a row measures a claim that exists, at the site that makes it.