feat(config): uniform deep-merge for component inheritance#565
Merged
Conversation
Component overrides merged whole-replace into the shared top-level defaults, so a partial override (only environment_config.prod, or only tag_grammar.prefix) silently dropped every inherited sibling. Replace it with a uniform field-by-field deep merge: an unset override inherits, a scalar or list replaces, and a nested block or keyed map merges key-by-key. extra_paths/shared_paths keep their existing union. The merge is a generic-JSON round trip over the same fidelity contract clone() already relies on; omitempty on every override field means an unset key is absent and never clobbers the inherited value. Signed-off-by: Joshua Temple <joshua.temple@stablekernel.com>
The deep merge builds the override patch from the marshalled component, and a bare bool/int with omitempty marshals its zero value to nothing. A component that set an inherited boolean back to false (for example deployments.enabled: false under a shared enabled: true) therefore merged nothing and silently kept the inherited true.
Pointer-ize the inheritable bool/int override fields so nil (unset) is distinct from an explicit false or 0, matching the AllowBreakingChanges and TelemetryConfig.JobSummary precedent: DeploymentsConfig.{Enabled,KeepPriorActive}, ReleaseConfig.Disabled, ChangelogConfig.{Disabled,Contributors}, PRPreviewConfig.{Enabled,Comment}, ValidateCheckConfig.Enabled, ValidateConfig.{SupportsDryRun,Retries}, EnvironmentConfig.WaitTimer, and DispatchInput.Required. Readers go through nil-safe accessor methods that centralize the unset default. Slice-element scalars (builds, deploys, external) are unaffected because a component list replaces the shared list wholesale.
Signed-off-by: Joshua Temple <joshua.temple@stablekernel.com>
The uniform field-merge corrupted XOR-exclusive and atomic blocks by composing one component's fields onto the inherited block's fields. The reachable security case: a shared validate.secrets: inherit plus a component narrowing to an explicit allow-list resolved to Inherit AND Map set; generation gives inherit precedence and emitted secrets: inherit, silently broadening the component's least-privilege scope. Add a replace-leaf set to the merge core: secrets (SecretsConfig, inherit XOR allow-list, custom decoder), runs_on (RunsOn, scalar|list|object polymorph, custom decoder), and release_token_app / state_token_app (AppTokenSource, atomic app_id+private_key credential). These whole-replace on override instead of field-merging, matching XOR/atomic semantics and the prior whole-replace behavior. A comment documents the class so a future exclusive block is added rather than silently corrupted. Signed-off-by: Joshua Temple <joshua.temple@stablekernel.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
1 participant
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.
Problem: component inheritance whole-replaced each block, so a partial override (a component setting only
environment_config.prod, or onlytag_grammar.prefix) silently dropped every inherited sibling. The demo manifest already documented this as "additive" but the code did not merge.Fix:
ResolveComponentnow applies a uniform field-by-field deep merge of the component overrides onto the shared top-level defaults:tag_grammar.prefixset keeps the inheritedprerelease_token/separator/dryrun_token).environment_configmerge by key and field-by-field within a key (onlyprodset keeps shareddev/staging, and aprodentry that sets onlywait_timerkeeps the inheritedgha_environment).extra_paths/shared_pathskeep their existing union.The merge is a generic-JSON round trip over the same fidelity contract
clone()already relies on;omitemptyon every override field means an unset key is absent from the patch and never clobbers the inherited value.Verification:
components_deepmerge_test.go) fail before / pass after: partialenvironment_configandtag_grammaroverrides now inherit siblings; guards proveextra_paths/shared_pathsstill union and a full block override still wins.go test ./...staying green. The one behavior change is exactly what a partial override should now produce.57-component-partial-inherit.yaml: top-leveldeployments.enabled: truewith a component setting onlydeployments.keep_prior_active. Verified by generating locally that this now emits deployment reporting withauto_inactive=falsefor the partial-override component andauto_inactive=truefor the plain inheritor; under whole-replace the partial-override component emitted no deployment step at all (its inheritedenabledwas dropped). Drift-free roundtrip asserted.go build,go test ./...,go test -race ./..., andgolangci-lint run ./...clean on both the root and thee2emodule.reference/manifest.mddocuments the deep-merge rule per field; the components guide points to it.