diff --git a/architecture/glossary.md b/architecture/glossary.md index 4d28ae3..053945e 100644 --- a/architecture/glossary.md +++ b/architecture/glossary.md @@ -5,8 +5,9 @@ capability pages share. One term, what it *is* (not what it does), and the synonyms to reject. **Service-key spec**: -The `(validate, emit)` pair for one Compose service key — how that key is checked -and how it renders to `podman run` flags. In code, `KeySpec` in `keys.py`. +The `(validate, emit, merge)` triple for one Compose service key — how that key +is checked, how it renders to `podman run` flags, and (for list/map-shaped keys) +how it merges across `extends`. In code, `KeySpec` in `keys.py`. _Avoid_: handler, plugin, rule. **Service-key registry**: diff --git a/compose2pod/extends.py b/compose2pod/extends.py index 198e0f2..6ce3886 100644 --- a/compose2pod/extends.py +++ b/compose2pod/extends.py @@ -3,21 +3,15 @@ from typing import Any from compose2pod.exceptions import UnsupportedComposeError +from compose2pod.keys import SERVICE_KEYS, _pairs_to_mapping -_MERGE_KEYS = {"environment", "labels", "annotations", "extra_hosts", "ulimits", "healthcheck", "depends_on"} -_CONCAT_KEYS = { - "cap_add", - "cap_drop", - "security_opt", - "devices", - "group_add", - "secrets", - "configs", - "volumes", - "tmpfs", - "env_file", -} +# Merge policy for keys with a SERVICE_KEYS KeySpec comes from spec.merge (see +# _merge below); these two sets cover only the remaining structural keys, which +# have no KeySpec. Unifying structural-key merge policy is deferred — see +# decisions/2026-07-12-reject-structural-key-registry.md's revisit trigger. +_STRUCTURAL_MERGE_KEYS = {"environment", "extra_hosts", "healthcheck", "depends_on"} +_STRUCTURAL_CONCAT_KEYS = {"secrets", "configs", "volumes", "tmpfs", "env_file"} def _extends_target(name: str, ext: Any) -> str: # noqa: ANN401 - Compose values are untyped @@ -39,21 +33,12 @@ def _extends_target(name: str, ext: Any) -> str: # noqa: ANN401 - Compose value return service -def _env_list_to_map(items: list[Any]) -> dict[str, Any]: - """Normalize a `[KEY=value, BARE]` environment list to a mapping (bare -> None).""" - result: dict[str, Any] = {} - for item in items: - key, sep, value = str(item).partition("=") - result[key] = value if sep else None - return result - - def _as_mapping(key: str, name: str, value: Any) -> dict[str, Any]: # noqa: ANN401 - Compose values are untyped if isinstance(value, dict): return value if isinstance(value, list): if key == "environment": - return _env_list_to_map(value) + return _pairs_to_mapping(name, key, value) if key == "depends_on": return {dep: {} for dep in value} msg = f"service {name!r}: cannot merge {key!r} across incompatible forms" @@ -73,9 +58,12 @@ def _merge(base: dict[str, Any], local: dict[str, Any], name: str) -> dict[str, """Merge `local` onto `base` per key category: mapping-merge, sequence-concat, else override.""" merged: dict[str, Any] = dict(base) for key, local_val in local.items(): - if key in base and key in _MERGE_KEYS: + spec = SERVICE_KEYS.get(key) + if key in base and spec is not None and spec.merge is not None: + merged[key] = spec.merge(name, key, base[key], local_val) + elif key in base and key in _STRUCTURAL_MERGE_KEYS: merged[key] = {**_as_mapping(key, name, base[key]), **_as_mapping(key, name, local_val)} - elif key in base and key in _CONCAT_KEYS: + elif key in base and key in _STRUCTURAL_CONCAT_KEYS: merged[key] = _as_list(key, name, base[key]) + _as_list(key, name, local_val) else: merged[key] = local_val diff --git a/compose2pod/keys.py b/compose2pod/keys.py index 3a7965e..9fd537a 100644 --- a/compose2pod/keys.py +++ b/compose2pod/keys.py @@ -38,10 +38,11 @@ def _key_value_pairs(value: list[Any] | dict[str, Any]) -> list[Any]: @dataclasses.dataclass(frozen=True, slots=True, kw_only=True) class KeySpec: - """A service-key spec: how one Compose service key is validated and emitted.""" + """A service-key spec: how one Compose service key is validated, emitted, and merged across extends.""" validate: Callable[[str, str, Any], None] emit: Callable[[Any], list[Token]] + merge: Callable[[str, str, Any, Any], Any] | None = None def _validate_scalar(name: str, key: str, value: Any) -> None: # noqa: ANN401 - Compose values are untyped YAML/JSON @@ -78,6 +79,40 @@ def _validate_map(name: str, key: str, value: Any) -> None: # noqa: ANN401 - Co raise UnsupportedComposeError(msg) +def _as_list(name: str, key: str, value: Any) -> list[Any]: # noqa: ANN401 - Compose values are untyped YAML/JSON + """Normalize list-or-scalar-string form to a list, for merging across extends.""" + if isinstance(value, list): + return list(value) + if isinstance(value, str): + return [value] + msg = f"service {name!r}: cannot merge {key!r} across incompatible forms" + raise UnsupportedComposeError(msg) + + +def _concat_list(name: str, key: str, base: Any, local: Any) -> list[Any]: # noqa: ANN401 - Compose values are untyped YAML/JSON + """Merge policy for list-shaped keys: concatenate base then local.""" + return _as_list(name, key, base) + _as_list(name, key, local) + + +def _pairs_to_mapping(name: str, key: str, value: Any) -> dict[str, Any]: # noqa: ANN401 - Compose values are untyped YAML/JSON + """Normalize list-or-dict key-value form to a mapping; inverse of _key_value_pairs.""" + if isinstance(value, dict): + return value + if isinstance(value, list): + result: dict[str, Any] = {} + for item in value: + pair_key, sep, pair_value = str(item).partition("=") + result[pair_key] = pair_value if sep else None + return result + msg = f"service {name!r}: cannot merge {key!r} across incompatible forms" + raise UnsupportedComposeError(msg) + + +def _merge_map(name: str, key: str, base: Any, local: Any) -> dict[str, Any]: # noqa: ANN401 - Compose values are untyped YAML/JSON + """Merge policy for map-shaped keys: key-by-key merge, local wins.""" + return {**_pairs_to_mapping(name, key, base), **_pairs_to_mapping(name, key, local)} + + def _scalar(flag: str) -> KeySpec: def emit(value: Any) -> list[Token]: # noqa: ANN401 - Compose values are untyped YAML/JSON return [flag, _Expand(value=str(value))] @@ -106,7 +141,7 @@ def emit(value: Any) -> list[Token]: # noqa: ANN401 - Compose values are untype tokens += [flag, _Expand(value=str(item))] return tokens - return KeySpec(validate=_validate_list, emit=emit) + return KeySpec(validate=_validate_list, emit=emit, merge=_concat_list) def _map(flag: str) -> KeySpec: @@ -116,7 +151,7 @@ def emit(value: Any) -> list[Token]: # noqa: ANN401 - Compose values are untype tokens += [flag, _Expand(value=str(pair))] return tokens - return KeySpec(validate=_validate_map, emit=emit) + return KeySpec(validate=_validate_map, emit=emit, merge=_merge_map) def _extra_host_pairs(value: list[Any] | dict[str, Any]) -> list[Any]: @@ -188,7 +223,7 @@ def _emit_ulimits(value: Any) -> list[Token]: # noqa: ANN401 - Compose values a "labels": _map("--label"), "annotations": _map("--annotation"), "pull_policy": KeySpec(validate=_validate_pull_policy, emit=_emit_pull_policy), - "ulimits": KeySpec(validate=_validate_ulimits, emit=_emit_ulimits), + "ulimits": KeySpec(validate=_validate_ulimits, emit=_emit_ulimits, merge=_merge_map), "mem_limit": _number_scalar("--memory"), "memswap_limit": _number_scalar("--memory-swap"), "mem_reservation": _number_scalar("--memory-reservation"), diff --git a/planning/changes/2026-07-13.05-derive-extends-merge-policy-from-keyspec.md b/planning/changes/2026-07-13.05-derive-extends-merge-policy-from-keyspec.md new file mode 100644 index 0000000..833c84d --- /dev/null +++ b/planning/changes/2026-07-13.05-derive-extends-merge-policy-from-keyspec.md @@ -0,0 +1,133 @@ +--- +summary: KeySpec (keys.py) gained a merge field baked in by _list()/_map(); extends.py now derives concat/merge for the 8 SERVICE_KEYS-shaped keys from it instead of a hand-copied set, and list-form labels/annotations merge correctly across extends as a side effect. +--- + +# Design: derive extends' merge policy from KeySpec + +## Summary + +Add a `merge: Callable[[Any, Any], Any] | None` field to `KeySpec`. The +`_list()`/`_map()` factories in `keys.py` set it automatically (concat / dict-merge +respectively), so every `SERVICE_KEYS` entry built through them gets extends +support for free. `extends.py`'s `_merge` looks up `SERVICE_KEYS[key].merge` +first, falling back to a shrunk, structural-only `_MERGE_KEYS`/`_CONCAT_KEYS` for +the 9 keys that still have no `KeySpec`. A new `_pairs_to_mapping` in `keys.py` +(the inverse of `_key_value_pairs`) backs the map-merge callable and, as a side +effect, fixes list-form `labels`/`annotations` merging — currently untested and +currently broken. + +## Motivation + +`extends.py`'s `_MERGE_KEYS`/`_CONCAT_KEYS` (`extends.py:8-20`) hand-copy shape +knowledge that `SERVICE_KEYS`'s validators (`keys.py:176-205`) already encode, +in a file that doesn't import `keys.py` at all. For the 8 keys that are both in +`SERVICE_KEYS` and in one of these two sets (`labels`, `annotations`, `ulimits`, +`group_add`, `cap_add`, `cap_drop`, `security_opt`, `devices`), the two sources +happen to agree today but nothing enforces it. Add a new `_list("--foo")` key to +`SERVICE_KEYS` and forget `extends._CONCAT_KEYS`, and the merge silently +degrades from "concatenate across the inheritance chain" to "last writer +wins" — `extends.py`'s fallback branch (`_merge`, line 81) never raises. +`tests/test_extends.py` asserts specific keys, not consistency with +`SERVICE_KEYS`'s shapes, so this class of bug isn't caught by CI. + +Separately: `keys.py`'s `_validate_map` accepts list-form *or* dict-form input +for `labels`/`annotations` (that's what `_key_value_pairs` exists for), but +`extends.py`'s `_as_mapping` only normalizes list-form for `environment` and +`depends_on`. List-form `labels`/`annotations` under `extends` today raises +"cannot merge ... across incompatible forms" — untested, and narrower than what +`keys.py` itself accepts everywhere else. + +## Design + +**`KeySpec` gains a `merge` field:** + +```python +@dataclasses.dataclass(frozen=True, slots=True, kw_only=True) +class KeySpec: + validate: Callable[[str, str, Any], None] + emit: Callable[[Any], list[Token]] + merge: Callable[[str, str, Any, Any], Any] | None = None +``` + +`_list()` and `_map()` set it automatically — no opt-in argument, so a future +key built through either factory can't skip it: + +```python +def _list(flag: str) -> KeySpec: + ... + return KeySpec(validate=_validate_list, emit=emit, merge=_concat_list) + +def _map(flag: str) -> KeySpec: + ... + return KeySpec(validate=_validate_map, emit=emit, merge=_merge_map) +``` + +`_concat_list(name, key, base, local)` normalizes both sides via the existing +list-or-str handling (moved from `extends._as_list`) and concatenates. +`_merge_map(name, key, base, local)` normalizes both sides via the new +`_pairs_to_mapping` (list-or-dict → dict, the inverse of `_key_value_pairs`, +living beside it in `keys.py`) and dict-merges, local winning. `merge`'s +`(name, key, ...)` prefix mirrors `validate`'s signature, so a merge callable +can raise the same service/key-named `UnsupportedComposeError` as validation +does. `ulimits`'s bespoke `KeySpec` +(`keys.py:191`) sets `merge=_merge_map` explicitly since it bypasses the `_map()` +factory. + +**`extends._merge` gains a first lookup:** + +```python +def _merge(base: dict[str, Any], local: dict[str, Any], name: str) -> dict[str, Any]: + merged: dict[str, Any] = dict(base) + for key, local_val in local.items(): + spec = SERVICE_KEYS.get(key) + if key in base and spec is not None and spec.merge is not None: + merged[key] = spec.merge(name, key, base[key], local_val) + elif key in base and key in _MERGE_KEYS: # structural keys only + ... + elif key in base and key in _CONCAT_KEYS: # structural keys only + ... + else: + merged[key] = local_val + return merged +``` + +`_MERGE_KEYS`/`_CONCAT_KEYS` shrink to the 9 keys with no `KeySpec`: +`_MERGE_KEYS = {"environment", "extra_hosts", "healthcheck", "depends_on"}`, +`_CONCAT_KEYS = {"secrets", "configs", "volumes", "tmpfs", "env_file"}` — their +`_as_mapping`/`_as_list` handling is unchanged. A one-line comment marks these +as pending `decisions/2026-07-12-reject-structural-key-registry.md`'s narrow +single-sourcing revisit trigger, not a green light to fold them into +`SERVICE_KEYS` now. + +**`architecture/glossary.md`**: the *Service-key spec* entry gains a clause — +the `(validate, emit)` pair becomes `(validate, emit, merge)`, `merge` optional +and shape-derived. + +## Non-goals + +- Unifying the 9 structural keys' merge policy — deferred; would conflate this + change with candidate #3 of the 2026-07-13 architecture review (owning + module per structural key), and `decisions/2026-07-12-reject-structural-key-registry.md` + already rejects a uniform structural registry on shape-heterogeneity grounds + that apply equally here. +- Changing `SERVICE_KEYS` membership or emitted flags — behavior-preserving for + every key already in the registry. + +## Testing + +`tests/test_keys.py`: a parametrized test over every `SERVICE_KEYS` entry +asserting `spec.merge is not None` iff the key was built via `_list()`/`_map()` +(or is `ulimits`), i.e. shape and merge-presence never drift. +`tests/test_extends.py`: a new case for list-form `labels` merging across +`extends` (the fixed gap — currently raises, should merge), plus the existing +dict-form-labels and concat-key tests re-asserted unchanged. `just test-ci` at +100%, `just lint-ci` clean, `just check-planning`. + +## Risk + +- **`_pairs_to_mapping` diverges from `_key_value_pairs`'s null-value + semantics** (low x med): both live in `keys.py` side by side, and the + parametrized `SERVICE_KEYS` test exercises every map-shaped key through the + same round trip. +- **Structural-key comment goes stale** (low x low): points at a decision file + with its own revisit trigger, not a TODO with no owner. diff --git a/tests/test_extends.py b/tests/test_extends.py index 461d309..6316969 100644 --- a/tests/test_extends.py +++ b/tests/test_extends.py @@ -73,6 +73,34 @@ def test_labels_mapping_merge(self) -> None: } assert _services(doc)["web"]["labels"] == {"team": "core", "tier": "web"} + def test_labels_list_form_normalized_and_merged(self) -> None: + doc = { + "services": { + "base": {"image": "x", "labels": ["team=core", "BARE"]}, + "web": {"extends": {"service": "base"}, "labels": ["team=web"]}, + } + } + assert _services(doc)["web"]["labels"] == {"team": "web", "BARE": None} + + def test_incompatible_labels_form_is_refused(self) -> None: + doc = { + "services": { + "base": {"image": "x", "labels": {"team": "core"}}, + "web": {"extends": {"service": "base"}, "labels": 5}, + } + } + with pytest.raises(UnsupportedComposeError, match="cannot merge 'labels' across incompatible forms"): + resolve_extends(doc) + + def test_cap_add_scalar_normalized_before_concat(self) -> None: + doc = { + "services": { + "base": {"image": "x", "cap_add": "NET_ADMIN"}, + "web": {"extends": {"service": "base"}, "cap_add": ["SYS_TIME"]}, + } + } + assert _services(doc)["web"]["cap_add"] == ["NET_ADMIN", "SYS_TIME"] + def test_depends_on_list_and_map_forms_merge(self) -> None: doc = { "services": { @@ -186,3 +214,13 @@ def test_extends_non_dict_base_defers_to_validate(self) -> None: result = resolve_extends(doc) assert "extends" not in result["services"]["web"] assert result["services"]["base"] is None + + def test_incompatible_structural_concat_form_is_refused(self) -> None: + doc = { + "services": { + "base": {"image": "x", "env_file": ["base.env"]}, + "web": {"extends": {"service": "base"}, "env_file": {"bad": "shape"}}, + } + } + with pytest.raises(UnsupportedComposeError, match="cannot merge 'env_file' across incompatible forms"): + resolve_extends(doc) diff --git a/tests/test_keys.py b/tests/test_keys.py index fcd8baa..f0f69b0 100644 --- a/tests/test_keys.py +++ b/tests/test_keys.py @@ -1,4 +1,15 @@ -from compose2pod.keys import SERVICE_KEYS, STRUCTURAL_KEYS +import pytest + +from compose2pod.exceptions import UnsupportedComposeError +from compose2pod.keys import ( + SERVICE_KEYS, + STRUCTURAL_KEYS, + _concat_list, + _merge_map, + _validate_list, + _validate_map, + _validate_ulimits, +) from compose2pod.parsing import SUPPORTED_SERVICE_KEYS @@ -58,3 +69,46 @@ def test_supported_service_keys_snapshot() -> None: "dns_opt", "sysctls", } == SUPPORTED_SERVICE_KEYS + + +@pytest.mark.parametrize("key", sorted(SERVICE_KEYS)) +def test_merge_present_iff_list_or_map_shaped(key: str) -> None: + """A key's merge callable must be set exactly when its shape supports one. + + Ties merge-presence to the validator each entry actually uses, rather than + a hand-copied key-name list, so a future _list()/_map() key can't silently + end up with no merge callable. + """ + spec = SERVICE_KEYS[key] + is_list_or_map_shaped = spec.validate in (_validate_list, _validate_map, _validate_ulimits) + assert (spec.merge is not None) == is_list_or_map_shaped + + +class TestMergeCallables: + """Direct tests of the merge policy functions, independent of any caller. + + extends.py (Task 2) will call these through SERVICE_KEYS[key].merge, but + that wiring doesn't exist yet — these tests exercise every branch of + _concat_list/_as_list/_merge_map/_pairs_to_mapping on their own so Task 1 + is fully covered without depending on Task 2. + """ + + def test_concat_list_merges_list_forms(self) -> None: + assert _concat_list("web", "cap_add", ["NET_ADMIN"], ["SYS_TIME"]) == ["NET_ADMIN", "SYS_TIME"] + + def test_concat_list_normalizes_scalar_string_form(self) -> None: + assert _concat_list("web", "cap_add", "NET_ADMIN", ["SYS_TIME"]) == ["NET_ADMIN", "SYS_TIME"] + + def test_concat_list_refuses_incompatible_form(self) -> None: + with pytest.raises(UnsupportedComposeError, match="cannot merge 'cap_add' across incompatible forms"): + _concat_list("web", "cap_add", ["NET_ADMIN"], {"bad": "shape"}) + + def test_merge_map_merges_dict_forms(self) -> None: + assert _merge_map("web", "labels", {"team": "core"}, {"tier": "web"}) == {"team": "core", "tier": "web"} + + def test_merge_map_normalizes_list_form(self) -> None: + assert _merge_map("web", "labels", ["team=core", "BARE"], ["team=web"]) == {"team": "web", "BARE": None} + + def test_merge_map_refuses_incompatible_form(self) -> None: + with pytest.raises(UnsupportedComposeError, match="cannot merge 'labels' across incompatible forms"): + _merge_map("web", "labels", {"team": "core"}, 5)