|
| 1 | +--- |
| 2 | +summary: Move the `environment` service key into the SERVICE_KEYS registry as `_map("-e")`, deleting its hand-rolled validate/emit/merge across parsing/emit/extends so it gains the single-spec locality (and drift-immunity) the 29 registry keys already have. |
| 3 | +--- |
| 4 | + |
| 5 | +# Design: move `environment` into the service-key registry |
| 6 | + |
| 7 | +## Summary |
| 8 | + |
| 9 | +`environment` is handled outside `SERVICE_KEYS` today — its validate lives in |
| 10 | +`parsing._validate_environment`, its emit in the `-e` branch of `emit._env_flags`, |
| 11 | +its merge in `extends._STRUCTURAL_MERGE_KEYS` — even though it is provably |
| 12 | +`_map("-e")`: the same `validate_map`, the same `["-e", Expand(str(pair))]` emit |
| 13 | +loop, and the same `_merge_map` its registry twins `labels`/`annotations` already |
| 14 | +use. This change adds `"environment": _map("-e")` to `SERVICE_KEYS` and deletes |
| 15 | +the three hand-rolled halves, giving `environment` one spec = validate+emit+merge |
| 16 | +in one place. Behavior-preserving. |
| 17 | + |
| 18 | +## Motivation |
| 19 | + |
| 20 | +`environment` re-implements a registry abstraction the code already trusts for 29 |
| 21 | +keys, and the hand-rolling carries the exact drift hazard the registry retires: |
| 22 | +its validate (`parsing.py`, `validate_map`) and its merge normalizer |
| 23 | +(`extends._as_mapping`) live in different modules with no binding, so the |
| 24 | +`extends` structural branch must re-derive the accepted grammar by hand to avoid |
| 25 | +laundering a shape the gate would reject (`extends.py:83-119`). The registry |
| 26 | +branch closes this by construction — it runs `spec.validate` on each side before |
| 27 | +`spec.merge` (`extends.py:145-156`). Moving `environment` in inherits that |
| 28 | +immunity for free. This is the narrow move licensed as examined-and-in-scope by |
| 29 | +`decisions/2026-07-12-reject-structural-key-registry.md` (the reopen note): a key |
| 30 | +that shares one uniform shape, not a structural-key registry. |
| 31 | + |
| 32 | +## Design |
| 33 | + |
| 34 | +Four source edits: |
| 35 | + |
| 36 | +- **`keys.py`** — add `"environment": _map("-e")` to `SERVICE_KEYS`; remove |
| 37 | + `"environment"` from `STRUCTURAL_KEYS`. |
| 38 | +- **`parsing.py`** — delete `_validate_environment` and its call in |
| 39 | + `_validate_service`. `environment` is now validated by the existing |
| 40 | + `SERVICE_KEYS` loop (`spec.validate`, `parsing.py:560-562`). The old |
| 41 | + `if ... is not None` guard was already dead: `_reject_null_values` refuses a |
| 42 | + null `environment` earlier in the same function. |
| 43 | +- **`emit.py`** — rename `_env_flags` → `_env_file_flags` and drop its `-e` |
| 44 | + loop, keeping only the `--env-file` emission; update the `run_flags` call site. |
| 45 | + `environment`'s `-e` tokens now come from the `SERVICE_KEYS` loop. |
| 46 | +- **`extends.py`** — remove `"environment"` from `_STRUCTURAL_MERGE_KEYS`; it |
| 47 | + now merges via `SERVICE_KEYS["environment"].merge`. |
| 48 | + |
| 49 | +Invariants held: |
| 50 | + |
| 51 | +- **Gate accept-list**: `SUPPORTED_SERVICE_KEYS = set(SERVICE_KEYS) | STRUCTURAL_KEYS` |
| 52 | + — `environment` only switches sides; the union is identical, so no key's |
| 53 | + accept/reject status changes. |
| 54 | +- **Emit**: `-e KEY=val` tokens are byte-identical. The `-e` flags move *after* |
| 55 | + `--env-file` in the generated `podman run` line, but podman's env precedence is |
| 56 | + by source, not command-line order — `--env` overrides `--env-file` regardless |
| 57 | + (stated in `podman-run.1`'s ENVIRONMENT precedence list) — so the container |
| 58 | + environment is unchanged, and Compose's "environment overrides env_file" still |
| 59 | + holds. |
| 60 | +- **Validate**: both the old and new paths call `validate_map`; the refusal |
| 61 | + message for a malformed `environment` is unchanged. |
| 62 | + |
| 63 | +## Non-goals |
| 64 | + |
| 65 | +- Moving `tmpfs`/`env_file`/`volumes` — `tmpfs` needs a bespoke validator |
| 66 | + (scalar-or-list vs `_list`'s list-only) and `env_file`/`volumes` need |
| 67 | + `project_dir`, which would force widening `KeySpec.emit`. Out of scope here. |
| 68 | +- Any change to `SERVICE_KEYS.emit`'s signature — `environment` fits |
| 69 | + `emit(value)` as-is. |
| 70 | + |
| 71 | +## Testing |
| 72 | + |
| 73 | +`just test-ci` at 100% coverage, `just lint-ci` clean, `just check-planning`. |
| 74 | + |
| 75 | +- `tests/test_keys.py` — `SERVICE_KEYS` count +1, `STRUCTURAL_KEYS` −1, union |
| 76 | + unchanged, `SERVICE_KEYS`/`STRUCTURAL_KEYS` still disjoint; the parametrized |
| 77 | + merge test now asserts `environment` carries `merge=_merge_map`. |
| 78 | +- `tests/test_emit.py` — the positional `-e` assertions (`flags[4:6]`, etc.) |
| 79 | + shift; rewrite them to locate the `-e` pair rather than a fixed index, and keep |
| 80 | + the `env_file`-only case (now `--env-file` at the front of the env flags). |
| 81 | +- `tests/test_parsing.py` — malformed `environment` still refused with the same |
| 82 | + `validate_map` message. |
| 83 | +- `tests/test_extends.py` — add a case proving the registry branch now validates |
| 84 | + each side before merging (an extends chain whose individual side has a |
| 85 | + malformed `environment` is refused, matching `labels`/`annotations`), plus the |
| 86 | + existing valid list-form/dict-form `environment` merges re-asserted unchanged. |
| 87 | +- Conformance + integration suites — green, no behavior change. |
| 88 | + |
| 89 | +## Risk |
| 90 | + |
| 91 | +- **A test constructs an extends chain with a per-side-malformed `environment` |
| 92 | + that was previously accepted post-merge** (low × med): now refused earlier by |
| 93 | + the registry branch. This is the intended drift-close (identical to |
| 94 | + `labels`/`annotations` today) — grep `test_extends.py` first; if such a case |
| 95 | + exists it is re-asserted as a refusal, not a regression. |
| 96 | +- **Missed positional `-e` assertion in `test_emit.py`** (low × low): caught by |
| 97 | + `just test-ci` before push. |
0 commit comments