Add profile composition via extends - #525
Open
RonaldHensbergen wants to merge 7 commits into
Open
Conversation
Profiles can now declare a top-level `extends: [name-or-path, ...]` field to compose from one or more parent profiles instead of duplicating shared configuration. Parents are resolved and merged left-to-right (later parents win), then the child profile is merged on top of all parents; extends chains are transitive. This reuses the exact deep-merge/module-merge-by-id engine and provenance model built for environment overlays (#229), rather than introducing a second merge engine, per the issue's explicit constraint. New cli/overlay.py helpers: - _merge_profile_docs(): the merge step shared by environment overlays and extends composition (previously duplicated inline in resolve_profile()). - _compose_extends(): resolves a profile's extends chain recursively, detecting cycles (E106), non-list/empty extends (E103), parents that resolve outside the profiles root (E104), and missing parents (E105). - resolve_extends(): a lighter entry point (no environment overlay, no full validate_loaded_profile()) used by build_plan()/validate_profile() so extends applies even without --environment, while preserving their own defensive diagnostic handling for malformed profiles. cli/planner.py's build_plan() and cli/validator.py's validate_profile() now resolve `extends` unconditionally rather than only when an --environment is selected, since extends is a property of the profile file itself. extends and --environment compose together: parents are merged first, then the child, then the environment overlay on top. Added `extends` to cli/resources/profile.schema.json and documented the feature in README.md alongside Environment Overlays. Closes #175.
…, error codes Clarifies that extends is read from profile.yaml (not a CLI flag), adds a multi-parent example, and documents the E103-E106 diagnostic codes introduced for extends composition.
Code review of PR #525 found that _derive_profiles_root() silently fell back to the profile's parent directory when no "profiles/" segment was present in its path, quietly widening the extends security boundary (E104) to an ad hoc root instead of rejecting resolution outright. This mirrors the existing fail-closed pattern already used by _derive_allowed_module_root() in cli/loader.py, which returns None (causing outright rejection) rather than substituting a narrower/wrong root. _derive_profiles_root() now returns None instead of profile_dir.parent when no "profiles" path segment is found, and _compose_extends() raises E104 in that case rather than resolving extends against a guessed root. Added a regression test.
Third review round found: - cli/main.py's _collect_profile_env_vars() (cds init) called load_yaml_file() directly when environment is None, dropping secrets/env vars declared only in parent profiles. - cli/getter.py's _collect_asset_roots() (cds get) likewise bypassed extends, silently omitting modules contributed only by a parent profile from asset fetching. - _derive_profiles_root() matched the first (outermost) "profiles" path segment instead of the last (innermost) one, letting an extends ref potentially escape the intended profiles tree. Both cds init and cds get now resolve through cli.overlay's resolve_extends()/resolve_profile(), and _derive_profiles_root() anchors on the innermost "profiles" segment. Added regression tests for all three fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Round 4 (cli/getter.py): _collect_extends_profile_dirs() had no cycle
detection of its own, so a cyclic extends chain caused unbounded
recursion / RecursionError in cds get instead of a clean error.
Threaded a _visited frozenset through the recursion, mirroring
_compose_extends's cycle-tracking stack.
Round 5 (cli/security.py): validate/run_security_validation still
called _load_yaml() directly on the profile when --environment was
omitted, so cds security silently skipped config/modules/secrets
declared only in a parent profile. Now routes through
resolve_extends(), consistent with cds init/get/plan/validate.
Removed the now-dead _load_yaml() helper.
Round 6 (cli/overlay.py): _merge_profile_docs() crashed with a
TypeError if a parent profile or environment overlay set `spec: null`,
since merged.setdefault("spec", {}) is a no-op when "spec" is already
present with value None. Normalize back to a dict before assigning
spec.modules.
Added regression tests: cycle detection with diamond-shaped (non-
cyclic) extends graphs in cds get, extends-parent-only findings
surfacing in cds security without --environment, and the spec:null
crash fix, plus a new tests/fixtures/security/extends-parent-secret
fixture.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements issue #175: profiles can now declare a top-level
extends: [name-or-path, ...]field to compose from one or more parent/base profiles, reducing duplication across related profiles.Semantics (per #175's acceptance criteria)
extendsaccepts a non-empty list of parent references — either a bare profile name (resolved under the profiles root) or a path relative to the child profile's directory.spec.modulesmerges by stableid(not array position); any other array is replaced wholesale, not concatenated.extendschains are transitive (parents may themselves extend other profiles).extends, parents resolving outside the profiles root, and missing parent files all fail with path-aware diagnostics (new codesE103–E106).--environment: parents merge first, then the child, then the selected environment overlay on top of the fully-composed result.Design constraint honored
Per the issue: "This issue generalizes the resolver implemented for #218; it must not introduce a second merge engine or a different provenance model." This PR reuses
cli/overlay.py's existing_merge_value()/_merge_modules()engine and provenance model — no new merge logic was written. The per-overlay module/duplicate/shape validation that used to be inlined inresolve_profile()was extracted into a shared_validate_modules_shape()+_merge_profile_docs()so both environment overlays andextendscomposition go through the identical code path.Wiring
build_plan()andvalidate_profile()now resolveextendsunconditionally, not only when--environmentis passed, sinceextendsis a property of the profile file itself. A new lighter entry point,resolve_extends(), does this without also runningresolve_profile()'s fullvalidate_loaded_profile()pass, preserving those callers' own defensive diagnostic handling for malformed profiles.Testing
tests/test_overlay.py(single/multiple parents, relative-path refs, transitive chains, cycle detection, missing/out-of-root parents, malformedextends, combination with--environment, and a no-extendsregression check).tests/test_planner.pyconfirmingbuild_plan()honorsextendswith no--environmentflag.make lintclean (ruff, yamllint, markdownlint, renovate-config-validator).Closes #175.