From 3ab6f37d93c8abb1419f19abd65f861e15ee20b3 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sat, 11 Jul 2026 18:48:29 -0400 Subject: [PATCH 1/3] feat(config): uniform deep-merge for component inheritance 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 --- docs/src/content/docs/guides/components.md | 5 + docs/src/content/docs/reference/manifest.md | 20 +++ .../57-component-partial-inherit.yaml | 88 ++++++++++ internal/config/components.go | 156 +++++++++--------- internal/config/components_deepmerge_test.go | 151 +++++++++++++++++ 5 files changed, 345 insertions(+), 75 deletions(-) create mode 100644 e2e/scenarios/57-component-partial-inherit.yaml create mode 100644 internal/config/components_deepmerge_test.go diff --git a/docs/src/content/docs/guides/components.md b/docs/src/content/docs/guides/components.md index 86cad232..e61c4801 100644 --- a/docs/src/content/docs/guides/components.md +++ b/docs/src/content/docs/guides/components.md @@ -40,6 +40,11 @@ and overridden per component only where it differs. When a `components:` block is present, the top-level config becomes the set of **shared defaults** every component inherits. Each entry under `components:` overrides those defaults where it sets a value and inherits them everywhere else. +Overrides are a deep merge: a component that sets only part of a block, such as a +single `environment_config` entry or one `tag_grammar` field, keeps the inherited +siblings rather than dropping them. See the [override +matrix](/cascade/reference/manifest/#inheritable-overrides) for the precise +per-field rules. This manifest ships an `api` service and a `web` frontend from one repository. The shared defaults set the CLI pin, the trunk branch, and the default environment diff --git a/docs/src/content/docs/reference/manifest.md b/docs/src/content/docs/reference/manifest.md index fa7e4510..3f42690f 100644 --- a/docs/src/content/docs/reference/manifest.md +++ b/docs/src/content/docs/reference/manifest.md @@ -813,6 +813,26 @@ where an override is meaningful. An unset field takes the shared top-level value `extra_triggers`, `pr_preview`, `validate_check`, `rollback`, `deployments`, `environment_config`, `triggers`, `release_token`, and `release_token_app`. +Inheritance is a **deep merge**. When a component overrides a block, it merges +field-by-field into the inherited default rather than replacing it wholesale, so +a partial override never drops the shared siblings it did not mention: + +- A **nested block** merges recursively. A component that sets only + `tag_grammar.prefix` keeps the inherited `prerelease_token`, `prerelease_separator`, + and `dryrun_token`. A component that sets only `deployments.keep_prior_active` + keeps the inherited `deployments.enabled`. +- A **keyed map** such as `environment_config` merges by key, and the entry under + each key merges field-by-field. A component that configures only its `prod` + environment keeps the shared `dev` and `staging` entries, and a `prod` entry that + sets only `wait_timer` keeps the inherited `gha_environment` for `prod`. +- A **scalar or a list** replaces. A component `environments` list overrides the + shared list outright, and a scalar such as `release_trigger` overrides the shared + value. + +The one exception to replace-a-list is the additive path set: a component's +`extra_paths` unions with the top-level `shared_paths` rather than replacing it. +See [Shared paths](#shared-paths). + `concurrency.cancel_in_progress` is inheritable, but `concurrency.group` is not: the orchestrate, promote, and rollback groups are derived per component so runs never serialize across components. Setting a component `concurrency.group` is a diff --git a/e2e/scenarios/57-component-partial-inherit.yaml b/e2e/scenarios/57-component-partial-inherit.yaml new file mode 100644 index 00000000..de3d436a --- /dev/null +++ b/e2e/scenarios/57-component-partial-inherit.yaml @@ -0,0 +1,88 @@ +name: "Per-Component Partial Override Inherits" +description: | + Proves uniform deep-merge inheritance for components end to end. The manifest + enables the GitHub Deployments integration once at the top level + (deployments.enabled: true) and lets one component set only a single sub-field of + that block (api sets deployments.keep_prior_active: true) while the other + component (web) carries no deployments override at all. + + Under whole-replace inheritance a partial override dropped every inherited + sibling, so api's block would have collapsed to {keep_prior_active: true} with + enabled defaulting back to false: api's orchestrate workflow would have emitted + no deployment reporting at all. Under deep-merge api keeps the inherited + enabled: true and layers its keep_prior_active on top, so its finalize job + reports the deployment with auto_inactive=false, while web inherits the plain + enabled: true and reports with the default auto_inactive=true. The scenario + generates the per-component set, proves the roundtrip is drift-free, and asserts + each component's orchestrate workflow carries the correctly merged deployment + reporting rather than the sibling's. + +config: + trunk_branch: main + environments: [dev, prod] + deployments: + enabled: true + builds: + - name: app + workflow: build.yaml + triggers: ["services/**"] + deploys: + - name: app + workflow: deploy.yaml + triggers: ["services/**"] + components: + api: + path: services/api + tag_prefix: api- + deployments: + keep_prior_active: true + web: + path: services/web + tag_prefix: web- + +steps: + - name: "Seed both component subtrees" + action: commit + commit: + message: "seed component sources" + files: + services/api/main.go: | + package main + + func main() {} + services/web/main.go: | + package main + + func main() {} + + - name: "Regenerate the per-component set and confirm no drift" + action: verify + verify: + regenerate: true + expect_exit: 0 + + - name: "Each component's deployment reporting reflects the merged block" + action: verify + verify: + regenerate: true + expect_exit: 0 + # api set only keep_prior_active; it inherits the top-level enabled: true, so + # its orchestrate workflow still creates a deployment and, because + # keep_prior_active merged in, reports it with auto_inactive=false. web + # inherits the bare enabled: true and reports with the default + # auto_inactive=true. The not_contains cross-checks that neither component + # picked up the other's auto_inactive value. + expect: + workflow_files: + - path: ".github/workflows/orchestrate-api.yaml" + contains: + - "Create deployment" + - "auto_inactive=false" + not_contains: + - "auto_inactive=true" + - path: ".github/workflows/orchestrate-web.yaml" + contains: + - "Create deployment" + - "auto_inactive=true" + not_contains: + - "auto_inactive=false" diff --git a/internal/config/components.go b/internal/config/components.go index d5c8d090..d6261210 100644 --- a/internal/config/components.go +++ b/internal/config/components.go @@ -128,83 +128,22 @@ func (c *TrunkConfig) ResolveComponent(name string) (*ResolvedComponent, error) eff.Components = nil // an effective per-component config has no nested components eff.SharedPaths = nil // top-level sugar, expanded into per-component extra paths below - // Required per-component tag namespace. - eff.TagPrefix = comp.TagPrefix - - // Inheritable overrides: apply only where the component set a value. - if comp.TagGrammar != nil { - eff.TagGrammar = comp.TagGrammar - } - if comp.Environments != nil { - eff.Environments = comp.Environments - } - if comp.ReleaseTrigger != "" { - eff.ReleaseTrigger = comp.ReleaseTrigger - } - if comp.AllowBreakingChanges != nil { - eff.AllowBreakingChanges = *comp.AllowBreakingChanges - } - if comp.Validate != nil { - eff.Validate = comp.Validate - } - if comp.Builds != nil { - eff.Builds = comp.Builds - } - if comp.Deploys != nil { - eff.Deploys = comp.Deploys - } - if comp.Publish != nil { - eff.Publish = comp.Publish - } - if comp.External != nil { - eff.External = comp.External - } - if comp.Notify != nil { - eff.Notify = comp.Notify - } - if comp.Release != nil { - eff.Release = comp.Release - } - if comp.Changelog != nil { - eff.Changelog = comp.Changelog - } - if comp.RunsOn != nil { - eff.RunsOn = comp.RunsOn - } - if comp.JobTimeoutMinutes != nil { - eff.JobTimeoutMinutes = *comp.JobTimeoutMinutes - } - if comp.DispatchInputs != nil { - eff.DispatchInputs = comp.DispatchInputs - } - if comp.ExtraTriggers != nil { - eff.ExtraTriggers = comp.ExtraTriggers - } - if comp.PRPreview != nil { - eff.PRPreview = comp.PRPreview - } - if comp.ValidateCheck != nil { - eff.ValidateCheck = comp.ValidateCheck - } - if comp.Rollback != nil { - eff.Rollback = comp.Rollback - } - if comp.Deployments != nil { - eff.Deployments = comp.Deployments - } - if comp.EnvironmentConfig != nil { - eff.EnvironmentConfig = comp.EnvironmentConfig - } - if comp.Triggers != nil { - eff.Triggers = comp.Triggers - } - if comp.ReleaseToken != "" { - eff.ReleaseToken = comp.ReleaseToken - } - if comp.ReleaseTokenApp != nil { - eff.ReleaseTokenApp = comp.ReleaseTokenApp + // Inheritable overrides: uniform deep-merge. Every inheritable block merges + // field-by-field into the shared defaults rather than replacing it wholesale. + // An override the component leaves unset keeps the inherited value; a scalar + // or slice the component sets replaces the inherited one; a nested block (or a + // keyed map such as environment_config) merges key-by-key so a partial + // override never drops the inherited siblings. The one axis that stays a union + // rather than a merge is extra_paths/shared_paths, resolved separately below. + if err := deepMergeComponentOverrides(eff, comp); err != nil { + return nil, err } + // Required per-component tag namespace. deepMergeComponentOverrides already + // carries comp.TagPrefix through; set it explicitly so the required namespace + // is unmistakable and independent of the merge path. + eff.TagPrefix = comp.TagPrefix + // Concurrency: cancel_in_progress is inheritable, the group is derived per // component. Start from the shared cancel policy, let the component override // it, and always compose the per-component group so the shared lane trap @@ -271,6 +210,73 @@ func mergePaths(lists ...[]string) []string { return out } +// deepMergeComponentOverrides folds a component's inheritable overrides into the +// shared defaults in place, field-by-field. It marshals both the base config and +// the component override to a generic JSON tree and recursively merges the +// override onto the base: an object present on both sides merges key-by-key (so a +// nested block or a keyed map such as environment_config keeps its inherited +// siblings), while a scalar or array on the override side replaces the inherited +// value. A field the component leaves unset is absent from its JSON (every +// override field carries omitempty), so it never clobbers the inherited value. +// +// The JSON round trip is the same fidelity contract clone() already depends on: +// every TrunkConfig field is JSON-tagged and none is json:"-" except the inline +// Extra catch-all, which is empty here (validation rejects unknown keys before +// resolution). Component-only keys such as path and extra_paths have no +// TrunkConfig field and are ignored on decode; concurrency is overwritten by the +// per-component derivation that follows, so leaving it in the merge is harmless. +func deepMergeComponentOverrides(base *TrunkConfig, override ComponentConfig) error { + baseJSON, err := json.Marshal(base) + if err != nil { + return fmt.Errorf("merging component overrides: %w", err) + } + overrideJSON, err := json.Marshal(override) + if err != nil { + return fmt.Errorf("merging component overrides: %w", err) + } + var baseTree, overrideTree map[string]any + if err := json.Unmarshal(baseJSON, &baseTree); err != nil { + return fmt.Errorf("merging component overrides: %w", err) + } + if err := json.Unmarshal(overrideJSON, &overrideTree); err != nil { + return fmt.Errorf("merging component overrides: %w", err) + } + mergeJSONTrees(baseTree, overrideTree) + mergedJSON, err := json.Marshal(baseTree) + if err != nil { + return fmt.Errorf("merging component overrides: %w", err) + } + // Reset before decoding so no stale slice or map tail from the pre-merge base + // lingers; the merged tree is a superset of the base keys, so this is a full + // rewrite of the effective config. + *base = TrunkConfig{} + if err := json.Unmarshal(mergedJSON, base); err != nil { + return fmt.Errorf("merging component overrides: %w", err) + } + return nil +} + +// mergeJSONTrees recursively merges the override tree onto the base tree in +// place, implementing the deep-merge inheritance rule at the generic-JSON level: +// when a key holds an object on both sides the objects merge recursively; a null +// override is treated as unset and leaves the base untouched; anything else +// (scalar or array) replaces the base value. It is the merge core behind +// deepMergeComponentOverrides. +func mergeJSONTrees(base, override map[string]any) { + for key, ov := range override { + if ov == nil { + continue // an explicit null is treated as unset: inherit the base + } + if ovMap, ok := ov.(map[string]any); ok { + if baseMap, ok := base[key].(map[string]any); ok { + mergeJSONTrees(baseMap, ovMap) + continue + } + } + base[key] = ov + } +} + // TagGrammarSpec returns the tag grammar a component reads and emits its versions // under: the component's resolved grammar (carrying its required tag_prefix and // any inherited or overridden tag_grammar block) with StrictPrefix forced true. diff --git a/internal/config/components_deepmerge_test.go b/internal/config/components_deepmerge_test.go new file mode 100644 index 00000000..e143dae5 --- /dev/null +++ b/internal/config/components_deepmerge_test.go @@ -0,0 +1,151 @@ +package config + +import "testing" + +// TestResolveComponent_DeepMergesEnvironmentConfig proves a component that sets +// only one environment's config inherits the shared entries for the others, and +// that within an overridden entry the unset fields fall back to the shared +// entry's values. Under the previous whole-replace semantics the component's +// single-key map dropped the shared dev/staging entries entirely. +func TestResolveComponent_DeepMergesEnvironmentConfig(t *testing.T) { + cfg := parseInline(t, ` +trunk_branch: main +environments: [dev, staging, prod] +environment_config: + dev: + gha_environment: development + staging: + gha_environment: staging-env + prod: + gha_environment: production + wait_timer: 10 +components: + api: + path: services/api + tag_prefix: api- + environment_config: + prod: + wait_timer: 15 + web: + path: services/web + tag_prefix: web- +`) + + rc, err := cfg.ResolveComponent("api") + if err != nil { + t.Fatalf("ResolveComponent(api): %v", err) + } + ec := rc.Config.EnvironmentConfig + if len(ec) != 3 { + t.Fatalf("environment_config keys = %v, want dev/staging/prod all present", ec) + } + if ec["dev"].GHAEnvironment != "development" { + t.Errorf("dev env_config dropped: %#v", ec["dev"]) + } + if ec["staging"].GHAEnvironment != "staging-env" { + t.Errorf("staging env_config dropped: %#v", ec["staging"]) + } + // Within the overridden prod entry: wait_timer overrides, gha_environment + // inherits the shared entry (within-key deep merge). + if ec["prod"].WaitTimer != 15 { + t.Errorf("prod wait_timer = %d, want override 15", ec["prod"].WaitTimer) + } + if ec["prod"].GHAEnvironment != "production" { + t.Errorf("prod gha_environment = %q, want inherited production", ec["prod"].GHAEnvironment) + } +} + +// TestResolveComponent_DeepMergesTagGrammar proves a component that sets only +// tag_grammar.prefix inherits the shared prerelease_token and separator instead +// of dropping them. Under whole-replace the sibling sub-fields were lost. +func TestResolveComponent_DeepMergesTagGrammar(t *testing.T) { + cfg := parseInline(t, ` +trunk_branch: main +tag_grammar: + prefix: v + prerelease_token: rc + prerelease_separator: "-" +components: + api: + path: services/api + tag_prefix: api- + tag_grammar: + prefix: api +`) + + rc, err := cfg.ResolveComponent("api") + if err != nil { + t.Fatalf("ResolveComponent(api): %v", err) + } + tg := rc.Config.TagGrammar + if tg == nil { + t.Fatal("tag_grammar is nil, want merged block") + } + if tg.Prefix == nil || *tg.Prefix != "api" { + t.Errorf("tag_grammar.prefix = %v, want override api", tg.Prefix) + } + if tg.PreReleaseToken == nil || *tg.PreReleaseToken != "rc" { + t.Errorf("tag_grammar.prerelease_token = %v, want inherited rc", tg.PreReleaseToken) + } + if tg.PreReleaseSeparator == nil || *tg.PreReleaseSeparator != "-" { + t.Errorf("tag_grammar.prerelease_separator = %v, want inherited -", tg.PreReleaseSeparator) + } +} + +// TestResolveComponent_ExtraPathsStillUnion guards the one axis that must NOT +// become a deep-merge: a component's extra_paths still unions with the top-level +// shared_paths, deduplicated and sorted. +func TestResolveComponent_ExtraPathsStillUnion(t *testing.T) { + cfg := parseInline(t, ` +trunk_branch: main +shared_paths: [libs/**] +components: + api: + path: services/api + tag_prefix: api- + extra_paths: [protos/**] +`) + + rc, err := cfg.ResolveComponent("api") + if err != nil { + t.Fatalf("ResolveComponent(api): %v", err) + } + got := map[string]bool{} + for _, p := range rc.ExtraPaths { + got[p] = true + } + if !got["libs/**"] || !got["protos/**"] { + t.Errorf("ExtraPaths = %v, want union of libs/** and protos/**", rc.ExtraPaths) + } +} + +// TestResolveComponent_FullBlockOverrideStillWins proves that when a component +// sets every field of a nested block, the component's values win outright: deep +// merge reduces to replacement when nothing is left to inherit. +func TestResolveComponent_FullBlockOverrideStillWins(t *testing.T) { + cfg := parseInline(t, ` +trunk_branch: main +tag_grammar: + prefix: v + prerelease_token: rc +components: + api: + path: services/api + tag_prefix: api- + tag_grammar: + prefix: api + prerelease_token: beta +`) + + rc, err := cfg.ResolveComponent("api") + if err != nil { + t.Fatalf("ResolveComponent(api): %v", err) + } + tg := rc.Config.TagGrammar + if tg.Prefix == nil || *tg.Prefix != "api" { + t.Errorf("prefix = %v, want api", tg.Prefix) + } + if tg.PreReleaseToken == nil || *tg.PreReleaseToken != "beta" { + t.Errorf("prerelease_token = %v, want override beta", tg.PreReleaseToken) + } +} From bba17f8d85612bc820bf335b766a7303dcbab41e Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sat, 11 Jul 2026 19:13:40 -0400 Subject: [PATCH 2/3] fix(config): honor explicit component opt-out in deep-merge inheritance 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 --- docs/src/content/docs/reference/manifest.md | 5 +- e2e/scenarios/58-component-opt-out-bool.yaml | 77 ++++++++++++++++++ internal/config/components_deepmerge_test.go | 55 ++++++++++++- internal/config/native_deployments_test.go | 4 +- internal/config/parse_test.go | 14 ++-- internal/config/ptr_helpers_test.go | 7 ++ internal/config/schema_v1.go | 80 ++++++++++++++++--- internal/config/schema_v1_test.go | 2 +- internal/config/types.go | 57 +++++++++++-- internal/config/validate_environment_test.go | 14 ++-- internal/config/validate_v1.go | 2 +- internal/environments/payload.go | 2 +- internal/environments/payload_test.go | 2 +- internal/environments/ptr_helpers_test.go | 5 ++ internal/generate/cli_validation_test.go | 2 +- internal/generate/command.go | 2 +- internal/generate/custom_changelog_test.go | 2 +- internal/generate/generator.go | 4 +- internal/generate/generator_test.go | 6 +- internal/generate/graph.go | 4 +- internal/generate/graph_test.go | 2 +- .../least_privilege_permissions_test.go | 2 +- internal/generate/native_deployments.go | 7 +- internal/generate/native_deployments_test.go | 2 +- internal/generate/plan.go | 2 +- internal/generate/pr_preview.go | 4 +- internal/generate/pr_preview_test.go | 6 +- internal/generate/promote.go | 2 +- internal/generate/ptr_helpers_test.go | 6 ++ internal/generate/validate_check.go | 2 +- internal/generate/validate_check_test.go | 14 ++-- 31 files changed, 321 insertions(+), 74 deletions(-) create mode 100644 e2e/scenarios/58-component-opt-out-bool.yaml create mode 100644 internal/config/ptr_helpers_test.go create mode 100644 internal/environments/ptr_helpers_test.go create mode 100644 internal/generate/ptr_helpers_test.go diff --git a/docs/src/content/docs/reference/manifest.md b/docs/src/content/docs/reference/manifest.md index 3f42690f..03793f44 100644 --- a/docs/src/content/docs/reference/manifest.md +++ b/docs/src/content/docs/reference/manifest.md @@ -827,7 +827,10 @@ a partial override never drops the shared siblings it did not mention: sets only `wait_timer` keeps the inherited `gha_environment` for `prod`. - A **scalar or a list** replaces. A component `environments` list overrides the shared list outright, and a scalar such as `release_trigger` overrides the shared - value. + value. An explicit opt-out is honored: a component that sets an inherited boolean + back to `false` (for example `deployments.enabled: false` under a shared + `deployments.enabled: true`), or an inherited number to `0`, overrides the shared + value rather than inheriting it. The one exception to replace-a-list is the additive path set: a component's `extra_paths` unions with the top-level `shared_paths` rather than replacing it. diff --git a/e2e/scenarios/58-component-opt-out-bool.yaml b/e2e/scenarios/58-component-opt-out-bool.yaml new file mode 100644 index 00000000..ee54d53e --- /dev/null +++ b/e2e/scenarios/58-component-opt-out-bool.yaml @@ -0,0 +1,77 @@ +name: "Per-Component Boolean Opt-Out" +description: | + Proves the opt-out direction of uniform deep-merge inheritance end to end. The + manifest enables the GitHub Deployments integration once at the top level + (deployments.enabled: true). One component (api) sets deployments.enabled: false + to opt out, while the other (web) carries no override and inherits the shared + enabled: true. + + The inheritable boolean is pointer-typed so an explicit false is distinct from + unset: a component that opts out marshals its false into the merge and overrides + the inherited true rather than dropping to nothing and silently staying enabled. + So api's orchestrate workflow emits no deployment reporting at all, while web's + still reports the deployment. The scenario generates the per-component set, + proves the roundtrip is drift-free, and asserts the opt-out component carries no + deployment step while its sibling does. + +config: + trunk_branch: main + environments: [dev, prod] + deployments: + enabled: true + builds: + - name: app + workflow: build.yaml + triggers: ["services/**"] + deploys: + - name: app + workflow: deploy.yaml + triggers: ["services/**"] + components: + api: + path: services/api + tag_prefix: api- + deployments: + enabled: false + web: + path: services/web + tag_prefix: web- + +steps: + - name: "Seed both component subtrees" + action: commit + commit: + message: "seed component sources" + files: + services/api/main.go: | + package main + + func main() {} + services/web/main.go: | + package main + + func main() {} + + - name: "Regenerate the per-component set and confirm no drift" + action: verify + verify: + regenerate: true + expect_exit: 0 + + - name: "The opt-out component drops deployment reporting; the sibling keeps it" + action: verify + verify: + regenerate: true + expect_exit: 0 + # api set deployments.enabled: false, overriding the inherited true, so its + # orchestrate workflow creates no deployment. web inherited the shared true, so + # its workflow still creates one. This is the opt-out direction the pointer- + # typed inheritable fields make correct. + expect: + workflow_files: + - path: ".github/workflows/orchestrate-api.yaml" + not_contains: + - "Create deployment" + - path: ".github/workflows/orchestrate-web.yaml" + contains: + - "Create deployment" diff --git a/internal/config/components_deepmerge_test.go b/internal/config/components_deepmerge_test.go index e143dae5..aea46bd0 100644 --- a/internal/config/components_deepmerge_test.go +++ b/internal/config/components_deepmerge_test.go @@ -47,8 +47,8 @@ components: } // Within the overridden prod entry: wait_timer overrides, gha_environment // inherits the shared entry (within-key deep merge). - if ec["prod"].WaitTimer != 15 { - t.Errorf("prod wait_timer = %d, want override 15", ec["prod"].WaitTimer) + if ec["prod"].WaitTimerMinutes() != 15 { + t.Errorf("prod wait_timer = %d, want override 15", ec["prod"].WaitTimerMinutes()) } if ec["prod"].GHAEnvironment != "production" { t.Errorf("prod gha_environment = %q, want inherited production", ec["prod"].GHAEnvironment) @@ -119,6 +119,57 @@ components: } } +// TestResolveComponent_DeepMergeOptOutOverridesInheritedTrue proves the opt-out +// direction: a component that sets an inherited boolean back to false overrides +// the inherited true rather than silently inheriting it. This is the regression +// the pointer-typed override fields guard against: with a bare bool the +// component's explicit false marshalled to nothing under the deep merge and the +// component silently stayed enabled. +func TestResolveComponent_DeepMergeOptOutOverridesInheritedTrue(t *testing.T) { + cfg := parseInline(t, ` +trunk_branch: main +environments: [dev, prod] +deployments: + enabled: true +changelog: + disabled: true +components: + api: + path: services/api + tag_prefix: api- + deployments: + enabled: false + changelog: + disabled: false + web: + path: services/web + tag_prefix: web- +`) + + api, err := cfg.ResolveComponent("api") + if err != nil { + t.Fatalf("ResolveComponent(api): %v", err) + } + if api.Config.Deployments.IsEnabled() { + t.Error("api set deployments.enabled: false but stayed enabled (inherited the shared true)") + } + if api.Config.Changelog.IsDisabled() { + t.Error("api set changelog.disabled: false but stayed disabled (inherited the shared true)") + } + + // The sibling with no override still inherits the shared true values. + web, err := cfg.ResolveComponent("web") + if err != nil { + t.Fatalf("ResolveComponent(web): %v", err) + } + if !web.Config.Deployments.IsEnabled() { + t.Error("web should inherit the shared deployments.enabled: true") + } + if !web.Config.Changelog.IsDisabled() { + t.Error("web should inherit the shared changelog.disabled: true") + } +} + // TestResolveComponent_FullBlockOverrideStillWins proves that when a component // sets every field of a nested block, the component's values win outright: deep // merge reduces to replacement when nothing is left to inherit. diff --git a/internal/config/native_deployments_test.go b/internal/config/native_deployments_test.go index 4b0bd068..054abf70 100644 --- a/internal/config/native_deployments_test.go +++ b/internal/config/native_deployments_test.go @@ -15,7 +15,7 @@ environment_config: production: environment_url: "https://app.example.com" `) - if cfg.Deployments == nil || !cfg.Deployments.Enabled || !cfg.Deployments.KeepPriorActive { + if !cfg.Deployments.IsEnabled() || !cfg.Deployments.KeepsPriorActive() { t.Fatalf("deployments: %#v", cfg.Deployments) } ec, ok := cfg.EnvironmentConfig["production"] @@ -35,7 +35,7 @@ func TestDeploymentsValidatesAtCurrentSchemaVersion(t *testing.T) { SchemaVersion: CurrentSchemaVersion, TrunkBranch: "main", Environments: []string{"production"}, - Deployments: &DeploymentsConfig{Enabled: true, KeepPriorActive: true}, + Deployments: &DeploymentsConfig{Enabled: boolPtr(true), KeepPriorActive: boolPtr(true)}, EnvironmentConfig: map[string]EnvironmentConfig{ "production": {EnvironmentURL: "https://app.example.com"}, }, diff --git a/internal/config/parse_test.go b/internal/config/parse_test.go index 7172cfc2..467a191a 100644 --- a/internal/config/parse_test.go +++ b/internal/config/parse_test.go @@ -66,7 +66,7 @@ func TestParse(t *testing.T) { if cfg.Validate.Workflow != ".github/workflows/validate.yaml" { t.Errorf("Validate.Workflow = %q, want %q", cfg.Validate.Workflow, ".github/workflows/validate.yaml") } - if !cfg.Validate.SupportsDryRun { + if !cfg.Validate.DryRunSupported() { t.Error("Validate.SupportsDryRun = false, want true") } @@ -505,7 +505,7 @@ func TestParse_ReleaseAndChangelogConfig(t *testing.T) { t.Fatal("Release is nil") return } - if cfg.Release.Disabled { + if cfg.Release.IsDisabled() { t.Error("Release.Disabled should be false (enabled by default)") } if cfg.Release.Tag != "goreleaser.tag" { @@ -552,7 +552,7 @@ func TestParse_ReleaseDisabled(t *testing.T) { t.Fatal("Release is nil") return } - if !cfg.Release.Disabled { + if !cfg.Release.IsDisabled() { t.Error("Release.Disabled should be true") } } @@ -721,8 +721,8 @@ func TestChangelogEnabled(t *testing.T) { expected bool }{ {"nil changelog - enabled by default", nil, true}, - {"disabled false - enabled", &ChangelogConfig{Disabled: false}, true}, - {"disabled true - explicitly disabled", &ChangelogConfig{Disabled: true}, false}, + {"disabled false - enabled", &ChangelogConfig{Disabled: boolPtr(false)}, true}, + {"disabled true - explicitly disabled", &ChangelogConfig{Disabled: boolPtr(true)}, false}, {"with workflow - custom enabled", &ChangelogConfig{Workflow: ".github/workflows/changelog.yaml"}, true}, } @@ -743,8 +743,8 @@ func TestReleaseEnabled(t *testing.T) { expected bool }{ {"nil release - enabled by default", nil, true}, - {"disabled false - enabled", &ReleaseConfig{Disabled: false}, true}, - {"disabled true - explicitly disabled", &ReleaseConfig{Disabled: true}, false}, + {"disabled false - enabled", &ReleaseConfig{Disabled: boolPtr(false)}, true}, + {"disabled true - explicitly disabled", &ReleaseConfig{Disabled: boolPtr(true)}, false}, {"with tag - external release enabled", &ReleaseConfig{Tag: "goreleaser.tag"}, true}, } diff --git a/internal/config/ptr_helpers_test.go b/internal/config/ptr_helpers_test.go new file mode 100644 index 00000000..32c6a60d --- /dev/null +++ b/internal/config/ptr_helpers_test.go @@ -0,0 +1,7 @@ +package config + +// boolPtr and intPtr build pointers to literal values for tests that populate +// the pointer-typed inheritable override fields (see the deep-merge inheritance +// contract on PRPreviewConfig). +func boolPtr(b bool) *bool { return &b } +func intPtr(i int) *int { return &i } diff --git a/internal/config/schema_v1.go b/internal/config/schema_v1.go index 50979f91..7d96c4ce 100644 --- a/internal/config/schema_v1.go +++ b/internal/config/schema_v1.go @@ -200,8 +200,16 @@ type DispatchInput struct { Default interface{} `yaml:"default,omitempty" json:"default,omitempty"` // Description is the operator-facing help text. Description string `yaml:"description,omitempty" json:"description,omitempty"` - // Required marks the input as required. - Required bool `yaml:"required,omitempty" json:"required,omitempty"` + // Required marks the input as required. A pointer so an explicit per-component + // opt-out (required: false against an inherited required: true) survives the + // dispatch_inputs deep merge, which merges by input key. + Required *bool `yaml:"required,omitempty" json:"required,omitempty"` +} + +// IsRequired reports whether the dispatch input is required. Nil-safe; defaults +// to false when unset. +func (d DispatchInput) IsRequired() bool { + return d.Required != nil && *d.Required } // Dispatch input type constants (GHA dispatch input types). @@ -247,14 +255,39 @@ type WorkflowRunTrigger struct { type MergeGroupTrigger struct{} // PRPreviewConfig is the opt-in read-only PR plan-preview lane (#40). +// +// Enabled and Comment are pointers so an unset field (nil) is distinct from an +// explicit false. Component inheritance is a deep merge over the marshalled +// override, so a bare bool would make a component that sets `enabled: false` +// against an inherited `enabled: true` marshal to nothing and silently inherit +// the true; a pointer keeps the explicit opt-out. Same reasoning as +// AllowBreakingChanges and TelemetryConfig.JobSummary. type PRPreviewConfig struct { - Enabled bool `yaml:"enabled,omitempty" json:"enabled,omitempty"` - Comment bool `yaml:"comment,omitempty" json:"comment,omitempty"` + Enabled *bool `yaml:"enabled,omitempty" json:"enabled,omitempty"` + Comment *bool `yaml:"comment,omitempty" json:"comment,omitempty"` +} + +// IsEnabled reports whether the PR-preview lane is on. Nil-safe: an unset block +// or an unset enabled is off. +func (p *PRPreviewConfig) IsEnabled() bool { + return p != nil && p.Enabled != nil && *p.Enabled +} + +// HasComment reports whether the lane posts a sticky comment. Nil-safe. +func (p *PRPreviewConfig) HasComment() bool { + return p != nil && p.Comment != nil && *p.Comment } // ValidateCheckConfig is the opt-in manifest-validation-as-a-PR-check lane (#41). +// Enabled is a pointer so an explicit per-component opt-out survives the +// inheritance deep merge (see PRPreviewConfig). type ValidateCheckConfig struct { - Enabled bool `yaml:"enabled,omitempty" json:"enabled,omitempty"` + Enabled *bool `yaml:"enabled,omitempty" json:"enabled,omitempty"` +} + +// IsEnabled reports whether the validate-check lane is on. Nil-safe. +func (v *ValidateCheckConfig) IsEnabled() bool { + return v != nil && v.Enabled != nil && *v.Enabled } // MergeQueueConfig is the opt-in merge-queue validation lane (#42). The raw @@ -308,12 +341,27 @@ const ( // When enabled, the finalize job creates a Deployment per target environment // and reports in_progress then success/failure status after each deploy. type DeploymentsConfig struct { - // Enabled activates GitHub Deployments API reporting in the finalize job. - Enabled bool `yaml:"enabled,omitempty" json:"enabled,omitempty"` + // Enabled activates GitHub Deployments API reporting in the finalize job. A + // pointer so an explicit per-component opt-out (enabled: false against an + // inherited enabled: true) survives the inheritance deep merge rather than + // marshalling to nothing and silently inheriting the true. + Enabled *bool `yaml:"enabled,omitempty" json:"enabled,omitempty"` // KeepPriorActive prevents GitHub from auto-inactivating prior deployments // for the same environment. Maps to auto_inactive:false in the Deployments API. - // Default false relies on GitHub native auto-inactivation. - KeepPriorActive bool `yaml:"keep_prior_active,omitempty" json:"keep_prior_active,omitempty"` + // Default (nil) relies on GitHub native auto-inactivation. A pointer for the + // same opt-out reason as Enabled. + KeepPriorActive *bool `yaml:"keep_prior_active,omitempty" json:"keep_prior_active,omitempty"` +} + +// IsEnabled reports whether GitHub Deployments reporting is on. Nil-safe. +func (d *DeploymentsConfig) IsEnabled() bool { + return d != nil && d.Enabled != nil && *d.Enabled +} + +// KeepsPriorActive reports whether prior deployments stay active +// (auto_inactive:false). Nil-safe; defaults to false when unset. +func (d *DeploymentsConfig) KeepsPriorActive() bool { + return d != nil && d.KeepPriorActive != nil && *d.KeepPriorActive } // RollbackConfig is the opt-in configuration block for the generated rollback @@ -409,7 +457,10 @@ type EnvironmentConfig struct { RequiredReviewers []string `yaml:"required_reviewers,omitempty" json:"required_reviewers,omitempty"` // WaitTimer is the delay, in minutes, before a job targeting this // environment runs. GitHub allows an integer between 0 and 43200 (30 days). - WaitTimer int `yaml:"wait_timer,omitempty" json:"wait_timer,omitempty"` + // A pointer so an explicit per-component opt-out (wait_timer: 0 against an + // inherited non-zero value) survives the inheritance deep merge rather than + // marshalling to nothing and silently inheriting the shared value. + WaitTimer *int `yaml:"wait_timer,omitempty" json:"wait_timer,omitempty"` // BranchPolicy selects which branches may deploy to this environment. It // maps to GitHub's deployment_branch_policy model: "protected" (only // protected branches), "custom" (only branches matching BranchPatterns or @@ -436,6 +487,15 @@ type EnvironmentConfig struct { EnvironmentURL string `yaml:"environment_url,omitempty" json:"environment_url,omitempty"` } +// WaitTimerMinutes returns the configured wait timer in minutes, or 0 when +// unset. Centralizes the nil default for the now-pointer WaitTimer field. +func (e EnvironmentConfig) WaitTimerMinutes() int { + if e.WaitTimer == nil { + return 0 + } + return *e.WaitTimer +} + // Environment branch-policy mode constants. They map onto GitHub's // deployment_branch_policy model: protected_branches, custom_branch_policies, // or null (all branches). diff --git a/internal/config/schema_v1_test.go b/internal/config/schema_v1_test.go index 8fe2523a..53d76f7c 100644 --- a/internal/config/schema_v1_test.go +++ b/internal/config/schema_v1_test.go @@ -223,7 +223,7 @@ environment_config: if et == nil || et.Schedule[0].Cron != "0 6 * * 1" || et.RepositoryDispatch == nil || et.WorkflowRun == nil || et.MergeGroup == nil { t.Fatalf("extra_triggers: %#v", et) } - if !cfg.PRPreview.Enabled || !cfg.ValidateCheck.Enabled || !cfg.MergeQueue.Enabled { + if !cfg.PRPreview.IsEnabled() || !cfg.ValidateCheck.IsEnabled() || !cfg.MergeQueue.Enabled { t.Fatalf("pr lanes: %#v %#v %#v", cfg.PRPreview, cfg.ValidateCheck, cfg.MergeQueue) } if cfg.Telemetry.Adapter != "none" || cfg.EnvironmentConfig["prod"].GHAEnvironment != "production" { diff --git a/internal/config/types.go b/internal/config/types.go index 787eea05..2e2c7c11 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -556,12 +556,16 @@ type ValidateConfig struct { Run string `yaml:"run,omitempty" json:"run,omitempty"` // rejected: inline run is no longer supported; use workflow: Shell string `yaml:"shell,omitempty" json:"shell,omitempty"` // rejected: inline shell is no longer supported; use workflow: Triggers []string `yaml:"triggers,omitempty" json:"triggers,omitempty"` // File patterns that should trigger validation - SupportsDryRun bool `yaml:"supports_dry_run,omitempty" json:"supports_dry_run,omitempty"` + // SupportsDryRun and Retries are pointers so an explicit per-component + // opt-out survives the inheritance deep merge (see PRPreviewConfig): a bare + // bool/int set to its zero value against an inherited non-zero would marshal + // to nothing and silently inherit. + SupportsDryRun *bool `yaml:"supports_dry_run,omitempty" json:"supports_dry_run,omitempty"` Inputs map[string]interface{} `yaml:"inputs,omitempty" json:"inputs,omitempty"` EnvInputs map[string]map[string]interface{} `yaml:"env_inputs,omitempty" json:"env_inputs,omitempty"` RunPolicy string `yaml:"run_policy,omitempty" json:"run_policy,omitempty"` OnFailure string `yaml:"on_failure,omitempty" json:"on_failure,omitempty"` - Retries int `yaml:"retries,omitempty" json:"retries,omitempty"` + Retries *int `yaml:"retries,omitempty" json:"retries,omitempty"` TimeoutMinutes int `yaml:"timeout_minutes,omitempty" json:"timeout_minutes,omitempty"` // Job-level timeout-minutes (omits when 0) // Per-callback fields. secrets and permissions are wired into generation: @@ -576,6 +580,20 @@ type ValidateConfig struct { Concurrency *ConcurrencyConfig `yaml:"concurrency,omitempty" json:"concurrency,omitempty"` } +// DryRunSupported reports whether the validate callback supports a dry run. +// Nil-safe; defaults to false when unset. +func (v *ValidateConfig) DryRunSupported() bool { + return v != nil && v.SupportsDryRun != nil && *v.SupportsDryRun +} + +// RetryCount returns the configured validate retry count, or 0 when unset. +func (v *ValidateConfig) RetryCount() int { + if v == nil || v.Retries == nil { + return 0 + } + return *v.Retries +} + // BuildConfig defines a build target type BuildConfig struct { Name string `yaml:"name" json:"name"` @@ -689,7 +707,10 @@ type PublishConfig struct { // ReleaseConfig defines release management settings type ReleaseConfig struct { - Disabled bool `yaml:"disabled,omitempty" json:"disabled,omitempty"` // true = disabled + // Disabled is a pointer so an explicit per-component opt-in (disabled: false + // against an inherited disabled: true) survives the inheritance deep merge + // rather than marshalling to nothing and silently inheriting the true. + Disabled *bool `yaml:"disabled,omitempty" json:"disabled,omitempty"` // true = disabled Tag string `yaml:"tag,omitempty" json:"tag,omitempty"` // callback.output reference for external releases // Workflow is the optional reusable-workflow path dispatched after a final @@ -799,16 +820,36 @@ func (n *NotifyConfig) GetToken() string { return normalizeTokenExpression(n.Token) } -// ChangelogConfig defines changelog generation settings +// ChangelogConfig defines changelog generation settings. Disabled and +// Contributors are pointers so an explicit per-component override of either to +// false survives the inheritance deep merge (see PRPreviewConfig). type ChangelogConfig struct { - Disabled bool `yaml:"disabled,omitempty" json:"disabled,omitempty"` // true = disabled + Disabled *bool `yaml:"disabled,omitempty" json:"disabled,omitempty"` // true = disabled Workflow string `yaml:"workflow,omitempty" json:"workflow,omitempty"` // custom changelog workflow - Contributors bool `yaml:"contributors,omitempty" json:"contributors,omitempty"` // true = include contributors section + Contributors *bool `yaml:"contributors,omitempty" json:"contributors,omitempty"` // true = include contributors section +} + +// IsDisabled reports whether changelog generation is turned off. Nil-safe; +// defaults to false (enabled) when unset. +func (c *ChangelogConfig) IsDisabled() bool { + return c != nil && c.Disabled != nil && *c.Disabled +} + +// IncludesContributors reports whether the contributors section is emitted. +// Nil-safe; defaults to false when unset. +func (c *ChangelogConfig) IncludesContributors() bool { + return c != nil && c.Contributors != nil && *c.Contributors +} + +// IsDisabled reports whether release management is turned off. Nil-safe; +// defaults to false (enabled) when unset. +func (r *ReleaseConfig) IsDisabled() bool { + return r != nil && r.Disabled != nil && *r.Disabled } // ReleaseEnabled returns true if release management is enabled (default: true) func (c *TrunkConfig) ReleaseEnabled() bool { - return c.Release == nil || !c.Release.Disabled + return c.Release == nil || !c.Release.IsDisabled() } // HasExternalRelease returns true if an external tool creates releases @@ -823,7 +864,7 @@ func (c *TrunkConfig) HasCustomChangelog() bool { // ChangelogEnabled returns true if changelog generation is enabled (default: true) func (c *TrunkConfig) ChangelogEnabled() bool { - return c.Changelog == nil || !c.Changelog.Disabled + return c.Changelog == nil || !c.Changelog.IsDisabled() } // HasReleaseArtifacts returns true if any build has artifacts configured diff --git a/internal/config/validate_environment_test.go b/internal/config/validate_environment_test.go index f2a73f5a..18c2d896 100644 --- a/internal/config/validate_environment_test.go +++ b/internal/config/validate_environment_test.go @@ -31,7 +31,7 @@ func TestValidateEnvironmentConfigFields(t *testing.T) { "prod": { GHAEnvironment: "production", RequiredReviewers: []string{"octocat", "team/ops"}, - WaitTimer: 10, + WaitTimer: intPtr(10), BranchPolicy: EnvBranchPolicyCustom, BranchPatterns: []string{"main", "release/*"}, TagPatterns: []string{"v*"}, @@ -44,21 +44,21 @@ func TestValidateEnvironmentConfigFields(t *testing.T) { { name: "wait_timer zero is valid", envConfig: map[string]EnvironmentConfig{ - "prod": {WaitTimer: 0}, + "prod": {WaitTimer: intPtr(0)}, }, wantErr: false, }, { name: "wait_timer at maximum is valid", envConfig: map[string]EnvironmentConfig{ - "prod": {WaitTimer: MaxWaitTimerMinutes}, + "prod": {WaitTimer: intPtr(MaxWaitTimerMinutes)}, }, wantErr: false, }, { name: "wait_timer above maximum is rejected", envConfig: map[string]EnvironmentConfig{ - "prod": {WaitTimer: MaxWaitTimerMinutes + 1}, + "prod": {WaitTimer: intPtr(MaxWaitTimerMinutes + 1)}, }, wantErr: true, errContains: "wait_timer must be between 0 and 43200 minutes", @@ -66,7 +66,7 @@ func TestValidateEnvironmentConfigFields(t *testing.T) { { name: "negative wait_timer is rejected", envConfig: map[string]EnvironmentConfig{ - "prod": {WaitTimer: -1}, + "prod": {WaitTimer: intPtr(-1)}, }, wantErr: true, errContains: "wait_timer must be between 0 and 43200 minutes", @@ -217,8 +217,8 @@ environment_config: if got, want := len(ec.RequiredReviewers), 2; got != want { t.Fatalf("required_reviewers len = %d, want %d", got, want) } - if ec.WaitTimer != 10 { - t.Fatalf("wait_timer = %d, want 10", ec.WaitTimer) + if ec.WaitTimerMinutes() != 10 { + t.Fatalf("wait_timer = %d, want 10", ec.WaitTimerMinutes()) } if ec.BranchPolicy != EnvBranchPolicyCustom { t.Fatalf("branch_policy = %q", ec.BranchPolicy) diff --git a/internal/config/validate_v1.go b/internal/config/validate_v1.go index 8b7bed2e..401f0d5e 100644 --- a/internal/config/validate_v1.go +++ b/internal/config/validate_v1.go @@ -673,7 +673,7 @@ func validateEnvironmentConfig(cfg *TrunkConfig) []string { ec := cfg.EnvironmentConfig[name] prefix := "environment_config." + name - if ec.WaitTimer < 0 || ec.WaitTimer > MaxWaitTimerMinutes { + if ec.WaitTimer != nil && (*ec.WaitTimer < 0 || *ec.WaitTimer > MaxWaitTimerMinutes) { errs = append(errs, fmt.Sprintf("%s.wait_timer must be between 0 and %d minutes", prefix, MaxWaitTimerMinutes)) } diff --git a/internal/environments/payload.go b/internal/environments/payload.go index 480d96e9..a6344b4c 100644 --- a/internal/environments/payload.go +++ b/internal/environments/payload.go @@ -127,7 +127,7 @@ func Build(cfg *config.TrunkConfig) Payload { Name: name, GHAEnvironment: ghaEnv, Environment: EnvironmentBody{ - WaitTimer: ec.WaitTimer, + WaitTimer: ec.WaitTimerMinutes(), DeploymentBranchPolicy: branchPolicy(ec.BranchPolicy), }, OperatorTodo: OperatorTodo{ diff --git a/internal/environments/payload_test.go b/internal/environments/payload_test.go index 9f85c6a0..1d021d96 100644 --- a/internal/environments/payload_test.go +++ b/internal/environments/payload_test.go @@ -20,7 +20,7 @@ func fullConfig() *config.TrunkConfig { "prod": { GHAEnvironment: "production", RequiredReviewers: []string{"octocat", "team/ops"}, - WaitTimer: 10, + WaitTimer: intPtr(10), BranchPolicy: config.EnvBranchPolicyProtected, Secrets: []string{"MY_SECRET", "DB_PASSWORD"}, Variables: []string{"REGION"}, diff --git a/internal/environments/ptr_helpers_test.go b/internal/environments/ptr_helpers_test.go new file mode 100644 index 00000000..59bd2b2e --- /dev/null +++ b/internal/environments/ptr_helpers_test.go @@ -0,0 +1,5 @@ +package environments + +// intPtr builds a pointer to a literal int for tests that populate the +// pointer-typed config.EnvironmentConfig.WaitTimer field. +func intPtr(i int) *int { return &i } diff --git a/internal/generate/cli_validation_test.go b/internal/generate/cli_validation_test.go index d0202761..93162499 100644 --- a/internal/generate/cli_validation_test.go +++ b/internal/generate/cli_validation_test.go @@ -359,7 +359,7 @@ func TestGeneratedWorkflow_ChangelogCommandValid(t *testing.T) { {Name: "app", Workflow: ".github/workflows/deploy.yaml"}, }, Changelog: &config.ChangelogConfig{ - Contributors: true, + Contributors: boolPtr(true), }, } diff --git a/internal/generate/command.go b/internal/generate/command.go index 3812b6f1..34ee8efc 100644 --- a/internal/generate/command.go +++ b/internal/generate/command.go @@ -359,7 +359,7 @@ func runGenerateWorkflow(opts generateOptions) error { // Generate the opt-in read-only PR plan-preview workflow (#40). Absent or // disabled pr_preview emits nothing, so existing manifests are unaffected. - if cfg.PRPreview != nil && cfg.PRPreview.Enabled { + if cfg.PRPreview.IsEnabled() { previewGen := NewPRPreviewGenerator(cfg, baseDir) content, err := previewGen.Generate() if err != nil { diff --git a/internal/generate/custom_changelog_test.go b/internal/generate/custom_changelog_test.go index e5b7d9cb..fca668f8 100644 --- a/internal/generate/custom_changelog_test.go +++ b/internal/generate/custom_changelog_test.go @@ -137,7 +137,7 @@ func TestCustomChangelog_Actionlint(t *testing.T) { cfg := &config.TrunkConfig{ TrunkBranch: "main", Environments: []string{"dev"}, - Changelog: &config.ChangelogConfig{Workflow: ".github/workflows/custom-changelog.yaml", Contributors: true}, + Changelog: &config.ChangelogConfig{Workflow: ".github/workflows/custom-changelog.yaml", Contributors: boolPtr(true)}, Builds: []config.BuildConfig{ {Name: "app", Workflow: ".github/workflows/build.yaml", Triggers: []string{"src/**"}}, }, diff --git a/internal/generate/generator.go b/internal/generate/generator.go index df27778c..1e764f11 100644 --- a/internal/generate/generator.go +++ b/internal/generate/generator.go @@ -750,7 +750,7 @@ func (g *Generator) writeWorkflowTriggers(sb *strings.Builder) { if di.Default != nil { fmt.Fprintf(sb, " default: '%v'\n", di.Default) } - if di.Required { + if di.IsRequired() { sb.WriteString(" required: true\n") } } @@ -1966,7 +1966,7 @@ func (g *Generator) writeChangelogStep(sb *strings.Builder) { sb.WriteString(" --base-sha \"${{ needs.setup.outputs.changelog_base_sha }}\" \\\n") sb.WriteString(" --head-sha \"${{ needs.setup.outputs.head_sha }}\" \\\n") // Add contributors flag if enabled in config - if g.config.Changelog != nil && g.config.Changelog.Contributors { + if g.config.Changelog.IncludesContributors() { sb.WriteString(" --contributors \\\n") } sb.WriteString(" --repo \"${{ github.repository }}\")\n") diff --git a/internal/generate/generator_test.go b/internal/generate/generator_test.go index 3b170fa7..52b841ed 100644 --- a/internal/generate/generator_test.go +++ b/internal/generate/generator_test.go @@ -745,7 +745,7 @@ func TestGenerator_FinalizeJob_ReleaseDisabled(t *testing.T) { cfg := &config.TrunkConfig{ TrunkBranch: "main", Environments: []string{"dev"}, - Release: &config.ReleaseConfig{Disabled: true}, + Release: &config.ReleaseConfig{Disabled: boolPtr(true)}, Builds: []config.BuildConfig{ {Name: "app", Workflow: ".github/workflows/build.yaml", Triggers: []string{"src/**"}}, }, @@ -2246,7 +2246,7 @@ func TestGenerator_DispatchInputs_StringType(t *testing.T) { "deploy_tag": { Type: config.DispatchInputTypeString, Description: "Override image tag", - Required: false, + Required: boolPtr(false), }, }, } @@ -2342,7 +2342,7 @@ func TestGenerator_DispatchInputs_RequiredFlag(t *testing.T) { DispatchInputs: map[string]config.DispatchInput{ "release_notes": { Type: config.DispatchInputTypeString, - Required: true, + Required: boolPtr(true), }, }, } diff --git a/internal/generate/graph.go b/internal/generate/graph.go index 9e94b29e..845ae3c5 100644 --- a/internal/generate/graph.go +++ b/internal/generate/graph.go @@ -89,12 +89,12 @@ func BuildDependencyGraph(cfg *config.TrunkConfig) *DependencyGraph { Workflow: cfg.Validate.Workflow, RunPolicy: defaultString(cfg.Validate.RunPolicy, config.RunPolicyDefault), OnFailure: defaultString(cfg.Validate.OnFailure, config.OnFailureAbort), - Retries: cfg.Validate.Retries, + Retries: cfg.Validate.RetryCount(), TimeoutMinutes: cfg.Validate.TimeoutMinutes, RunsOn: cfg.Validate.RunsOn, Permissions: cfg.Validate.Permissions, Concurrency: cfg.Validate.Concurrency, - SupportsDryRun: cfg.Validate.SupportsDryRun, + SupportsDryRun: cfg.Validate.DryRunSupported(), Secrets: cfg.Validate.Secrets, } g.Edges[jobID] = nil diff --git a/internal/generate/graph_test.go b/internal/generate/graph_test.go index ff09ed55..83c3bce7 100644 --- a/internal/generate/graph_test.go +++ b/internal/generate/graph_test.go @@ -108,7 +108,7 @@ func TestBuildDependencyGraph_WithValidate(t *testing.T) { Workflow: ".github/workflows/validate.yaml", RunPolicy: config.RunPolicyAlways, OnFailure: config.OnFailureAbort, - Retries: 1, + Retries: intPtr(1), }, Builds: []config.BuildConfig{ {Name: "app", DependsOn: []string{}}, diff --git a/internal/generate/least_privilege_permissions_test.go b/internal/generate/least_privilege_permissions_test.go index f31f90bd..7cac6e0f 100644 --- a/internal/generate/least_privilege_permissions_test.go +++ b/internal/generate/least_privilege_permissions_test.go @@ -126,7 +126,7 @@ func TestPromote_NativeDeployments_JobLevelDeploymentsWrite(t *testing.T) { Deploys: []config.DeployConfig{ {Name: "app", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}}, }, - Deployments: &config.DeploymentsConfig{Enabled: true}, + Deployments: &config.DeploymentsConfig{Enabled: boolPtr(true)}, EnvironmentConfig: map[string]config.EnvironmentConfig{ "production": {EnvironmentURL: "https://app.example.com"}, }, diff --git a/internal/generate/native_deployments.go b/internal/generate/native_deployments.go index ad564b14..50f895c4 100644 --- a/internal/generate/native_deployments.go +++ b/internal/generate/native_deployments.go @@ -11,7 +11,7 @@ import ( // nativeDeploymentsEnabled reports whether the manifest opted in to GitHub // Deployments API reporting in the finalize job. func nativeDeploymentsEnabled(cfg *config.TrunkConfig) bool { - return cfg.Deployments != nil && cfg.Deployments.Enabled + return cfg.Deployments.IsEnabled() } // deploymentAutoInactive returns the auto_inactive value sent to the Deployments @@ -19,10 +19,7 @@ func nativeDeploymentsEnabled(cfg *config.TrunkConfig) bool { // deployments for the same environment Active; the default relies on GitHub's // native auto-inactivation (auto_inactive:true). func deploymentAutoInactive(cfg *config.TrunkConfig) bool { - if cfg.Deployments != nil && cfg.Deployments.KeepPriorActive { - return false - } - return true + return !cfg.Deployments.KeepsPriorActive() } // writeNativeDeploymentSteps emits the GitHub Deployments API lifecycle for the diff --git a/internal/generate/native_deployments_test.go b/internal/generate/native_deployments_test.go index a51f50e8..c28efb72 100644 --- a/internal/generate/native_deployments_test.go +++ b/internal/generate/native_deployments_test.go @@ -37,7 +37,7 @@ on: Deploys: []config.DeployConfig{ {Name: "app", Workflow: ".github/workflows/deploy.yaml", Triggers: []string{"src/**"}}, }, - Deployments: &config.DeploymentsConfig{Enabled: true}, + Deployments: &config.DeploymentsConfig{Enabled: boolPtr(true)}, EnvironmentConfig: map[string]config.EnvironmentConfig{ "production": {EnvironmentURL: "https://app.example.com"}, }, diff --git a/internal/generate/plan.go b/internal/generate/plan.go index dc60d9aa..8f397f24 100644 --- a/internal/generate/plan.go +++ b/internal/generate/plan.go @@ -199,7 +199,7 @@ func Plan(opts PlanOptions) ([]PlannedFile, error) { } // 8. pr-preview -> .github/workflows/cascade-pr-preview.yaml when enabled. - if cfg.PRPreview != nil && cfg.PRPreview.Enabled { + if cfg.PRPreview.IsEnabled() { content, err = NewPRPreviewGenerator(cfg, baseDir).Generate() if err != nil { return nil, fmt.Errorf("generating pr-preview workflow: %w", err) diff --git a/internal/generate/pr_preview.go b/internal/generate/pr_preview.go index 9fc9038b..f475cf4d 100644 --- a/internal/generate/pr_preview.go +++ b/internal/generate/pr_preview.go @@ -44,12 +44,12 @@ func (g *PRPreviewGenerator) getCLIRef() string { // commentEnabled reports whether the preview should also post a PR comment. func (g *PRPreviewGenerator) commentEnabled() bool { - return g.config.PRPreview != nil && g.config.PRPreview.Comment + return g.config.PRPreview.HasComment() } // Generate creates the PR plan-preview workflow content. func (g *PRPreviewGenerator) Generate() (string, error) { - if g.config.PRPreview == nil || !g.config.PRPreview.Enabled { + if !g.config.PRPreview.IsEnabled() { return "", fmt.Errorf("cannot generate pr-preview workflow: pr_preview is not enabled") } diff --git a/internal/generate/pr_preview_test.go b/internal/generate/pr_preview_test.go index 415087f2..ad969bbf 100644 --- a/internal/generate/pr_preview_test.go +++ b/internal/generate/pr_preview_test.go @@ -17,8 +17,8 @@ func prPreviewConfig(comment bool) *config.TrunkConfig { TrunkBranch: "main", Environments: []string{"dev"}, PRPreview: &config.PRPreviewConfig{ - Enabled: true, - Comment: comment, + Enabled: boolPtr(true), + Comment: boolPtr(comment), }, } } @@ -98,7 +98,7 @@ func TestPRPreviewGenerator_Disabled(t *testing.T) { // enabled: false gen = NewPRPreviewGenerator(&config.TrunkConfig{ TrunkBranch: "main", - PRPreview: &config.PRPreviewConfig{Enabled: false}, + PRPreview: &config.PRPreviewConfig{Enabled: boolPtr(false)}, }, "") _, err = gen.Generate() require.Error(t, err) diff --git a/internal/generate/promote.go b/internal/generate/promote.go index aef57f58..513a4b35 100644 --- a/internal/generate/promote.go +++ b/internal/generate/promote.go @@ -1164,7 +1164,7 @@ func (g *PromoteGenerator) writeFinalizeJob(sb *strings.Builder) { // Build changelog command with optional --contributors flag changelogCmd := "cascade generate-changelog --base-sha \"$TARGET_SHA\" --head-sha \"$SOURCE_SHA\" --repo \"${{ github.repository }}\"" - if g.config.Changelog != nil && g.config.Changelog.Contributors { + if g.config.Changelog.IncludesContributors() { changelogCmd += " --contributors" } fmt.Fprintf(sb, " RESULT=$(%s)\n", changelogCmd) diff --git a/internal/generate/ptr_helpers_test.go b/internal/generate/ptr_helpers_test.go new file mode 100644 index 00000000..e1aa3de0 --- /dev/null +++ b/internal/generate/ptr_helpers_test.go @@ -0,0 +1,6 @@ +package generate + +// intPtr builds a pointer to a literal int for tests that populate the +// pointer-typed inheritable override fields (e.g. ValidateConfig.Retries). +// boolPtr lives in deploy_strategy_test.go. +func intPtr(i int) *int { return &i } diff --git a/internal/generate/validate_check.go b/internal/generate/validate_check.go index 3fa37b00..845c4f6a 100644 --- a/internal/generate/validate_check.go +++ b/internal/generate/validate_check.go @@ -30,7 +30,7 @@ func NewValidateCheckGenerator(cfg *config.TrunkConfig, baseDir string) *Validat // Enabled reports whether the manifest opts in to the validation check. func (g *ValidateCheckGenerator) Enabled() bool { - return g.config != nil && g.config.ValidateCheck != nil && g.config.ValidateCheck.Enabled + return g.config != nil && g.config.ValidateCheck.IsEnabled() } // getCLIRef mirrors the ref-resolution used by the other generators so the diff --git a/internal/generate/validate_check_test.go b/internal/generate/validate_check_test.go index 17349364..3c123145 100644 --- a/internal/generate/validate_check_test.go +++ b/internal/generate/validate_check_test.go @@ -14,21 +14,21 @@ import ( func TestValidateCheckGenerator_Enabled(t *testing.T) { cfg := &config.TrunkConfig{ TrunkBranch: "main", - ValidateCheck: &config.ValidateCheckConfig{Enabled: true}, + ValidateCheck: &config.ValidateCheckConfig{Enabled: boolPtr(true)}, } gen := NewValidateCheckGenerator(cfg, "") assert.True(t, gen.Enabled(), "validate_check.enabled should report enabled") // Absent block and explicitly-disabled block both report disabled. assert.False(t, NewValidateCheckGenerator(&config.TrunkConfig{TrunkBranch: "main"}, "").Enabled()) - disabled := &config.TrunkConfig{ValidateCheck: &config.ValidateCheckConfig{Enabled: false}} + disabled := &config.TrunkConfig{ValidateCheck: &config.ValidateCheckConfig{Enabled: boolPtr(false)}} assert.False(t, NewValidateCheckGenerator(disabled, "").Enabled()) } func TestValidateCheckGenerator_TriggersAndPermissions(t *testing.T) { cfg := &config.TrunkConfig{ TrunkBranch: "main", - ValidateCheck: &config.ValidateCheckConfig{Enabled: true}, + ValidateCheck: &config.ValidateCheckConfig{Enabled: boolPtr(true)}, } gen := NewValidateCheckGenerator(cfg, "") content, err := gen.Generate() @@ -49,7 +49,7 @@ func TestValidateCheckGenerator_TriggersAndPermissions(t *testing.T) { func TestValidateCheckGenerator_Steps(t *testing.T) { cfg := &config.TrunkConfig{ TrunkBranch: "main", - ValidateCheck: &config.ValidateCheckConfig{Enabled: true}, + ValidateCheck: &config.ValidateCheckConfig{Enabled: boolPtr(true)}, } gen := NewValidateCheckGenerator(cfg, "") content, err := gen.Generate() @@ -78,7 +78,7 @@ func TestValidateCheckGenerator_Steps(t *testing.T) { func TestValidateCheckGenerator_SetupCLIPassesToken(t *testing.T) { cfg := &config.TrunkConfig{ TrunkBranch: "main", - ValidateCheck: &config.ValidateCheckConfig{Enabled: true}, + ValidateCheck: &config.ValidateCheckConfig{Enabled: boolPtr(true)}, } gen := NewValidateCheckGenerator(cfg, "") content, err := gen.Generate() @@ -94,7 +94,7 @@ func TestValidateCheckGenerator_PinModeSHA(t *testing.T) { cfg := &config.TrunkConfig{ TrunkBranch: "main", PinMode: config.PinModeSHA, - ValidateCheck: &config.ValidateCheckConfig{Enabled: true}, + ValidateCheck: &config.ValidateCheckConfig{Enabled: boolPtr(true)}, } gen := NewValidateCheckGenerator(cfg, "") content, err := gen.Generate() @@ -107,7 +107,7 @@ func TestValidateCheckGenerator_PinModeSHA(t *testing.T) { func TestValidateCheckGenerator_ValidYAML(t *testing.T) { cfg := &config.TrunkConfig{ TrunkBranch: "main", - ValidateCheck: &config.ValidateCheckConfig{Enabled: true}, + ValidateCheck: &config.ValidateCheckConfig{Enabled: boolPtr(true)}, } gen := NewValidateCheckGenerator(cfg, "") content, err := gen.Generate() From d50a71e7ae3f3f87428aff5f1600572ac2d2a8f1 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Sat, 11 Jul 2026 19:28:00 -0400 Subject: [PATCH 3/3] fix(config): whole-replace exclusive blocks in deep-merge inheritance 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 --- docs/src/content/docs/reference/manifest.md | 7 ++ .../59-component-secrets-replace-leaf.yaml | 82 +++++++++++++ internal/config/components.go | 44 ++++++- internal/config/components_deepmerge_test.go | 108 ++++++++++++++++++ 4 files changed, 236 insertions(+), 5 deletions(-) create mode 100644 e2e/scenarios/59-component-secrets-replace-leaf.yaml diff --git a/docs/src/content/docs/reference/manifest.md b/docs/src/content/docs/reference/manifest.md index 03793f44..66d52a32 100644 --- a/docs/src/content/docs/reference/manifest.md +++ b/docs/src/content/docs/reference/manifest.md @@ -831,6 +831,13 @@ a partial override never drops the shared siblings it did not mention: back to `false` (for example `deployments.enabled: false` under a shared `deployments.enabled: true`), or an inherited number to `0`, overrides the shared value rather than inheriting it. +- A few **exclusive blocks whole-replace** instead of field-merging, because their + fields are not independently composable: `secrets` (either `inherit` or an + explicit allow-list, never both), `runs_on` (a label, a list, or a `{group, + labels}` object, one form only), and the `release_token_app` / `state_token_app` + credential blocks. A component that sets one of these replaces the inherited + block outright. This keeps a component narrowing `secrets: inherit` to an + explicit allow-list from being silently broadened back to inherit-all. The one exception to replace-a-list is the additive path set: a component's `extra_paths` unions with the top-level `shared_paths` rather than replacing it. diff --git a/e2e/scenarios/59-component-secrets-replace-leaf.yaml b/e2e/scenarios/59-component-secrets-replace-leaf.yaml new file mode 100644 index 00000000..95a60143 --- /dev/null +++ b/e2e/scenarios/59-component-secrets-replace-leaf.yaml @@ -0,0 +1,82 @@ +name: "Per-Component Secrets Replace-Leaf" +description: | + Proves the secrets block is a replace-leaf in the inheritance deep merge, + end to end. The shared validate gate opts into inheriting all caller secrets + (validate.secrets: inherit). One component (api) narrows to an explicit + allow-list (validate.secrets: {NPM_TOKEN: NPM_TOKEN}); the other (web) inherits. + + The secrets block is XOR-exclusive: inherit-all OR an explicit allow-list, never + both. A field-merge of the inherited inherit with the component allow-list would + set both, and generation gives inherit precedence, silently broadening api back + to inherit-all and discarding its least-privilege allow-list. Treating secrets + as a replace-leaf keeps the component's allow-list intact. The scenario generates + the per-component set, proves the roundtrip is drift-free, and asserts api's + orchestrate workflow emits the explicit NPM_TOKEN mapping (not secrets: inherit) + while web inherits secrets: inherit. + +config: + trunk_branch: main + environments: [dev, prod] + validate: + workflow: validate.yaml + secrets: inherit + builds: + - name: app + workflow: build.yaml + triggers: ["services/**"] + deploys: + - name: app + workflow: deploy.yaml + triggers: ["services/**"] + components: + api: + path: services/api + tag_prefix: api- + validate: + workflow: validate.yaml + secrets: + NPM_TOKEN: NPM_TOKEN + web: + path: services/web + tag_prefix: web- + +steps: + - name: "Seed both component subtrees" + action: commit + commit: + message: "seed component sources" + files: + services/api/main.go: | + package main + + func main() {} + services/web/main.go: | + package main + + func main() {} + + - name: "Regenerate the per-component set and confirm no drift" + action: verify + verify: + regenerate: true + expect_exit: 0 + + - name: "The narrowing component keeps its allow-list; the sibling inherits inherit" + action: verify + verify: + regenerate: true + expect_exit: 0 + # api narrowed validate.secrets to an explicit allow-list, so its orchestrate + # workflow emits the NPM_TOKEN mapping and never secrets: inherit. web inherited + # the shared inherit-all block. A field-merge would have broadened api back to + # secrets: inherit, discarding its least-privilege scope. + expect: + workflow_files: + - path: ".github/workflows/orchestrate-api.yaml" + contains: + - "NPM_TOKEN: ${{ secrets.NPM_TOKEN }}" + not_contains: + - "secrets: inherit" + - path: ".github/workflows/orchestrate-web.yaml" + contains: + - "secrets: inherit" diff --git a/internal/config/components.go b/internal/config/components.go index d6261210..4011c1d4 100644 --- a/internal/config/components.go +++ b/internal/config/components.go @@ -256,21 +256,55 @@ func deepMergeComponentOverrides(base *TrunkConfig, override ComponentConfig) er return nil } +// mergeReplaceLeafKeys names the manifest blocks that must WHOLE-REPLACE on +// override rather than field-merge, even though they marshal to a JSON object. +// These are exclusive or atomic blocks whose fields are not independently +// composable, so recursively merging one component's fields onto the inherited +// block's fields corrupts it: +// +// - secrets (SecretsConfig): XOR-exclusive - either "inherit" (all caller +// secrets) OR an explicit {name: source} allow-list, never both. A +// field-merge of an inherited "inherit" with a component allow-list yields +// both set, and generation gives "inherit" precedence, silently BROADENING +// the component's least-privilege allow-list. It has a custom UnmarshalYAML. +// - runs_on (RunsOn): polymorphic - a scalar label, a list of labels, or a +// {group, labels} object, exactly one form. Field-merging two forms leaves a +// stale field from the inherited form set. It has a custom UnmarshalYAML. +// - release_token_app / state_token_app (AppTokenSource): an atomic credential +// identity (app_id + private_key). Field-merging would splice an app_id from +// one source with a private_key from another; whole-replace makes an +// incomplete override a clean validation error instead of a bad credential. +// +// A new exclusive, polymorphic, atomic, or custom-decoded block MUST be added +// here, or the tree-merge will silently corrupt it. The keys are matched by name +// at any depth because each of these names maps to exactly one such type in the +// schema (for example secrets under validate and under each build/deploy). +var mergeReplaceLeafKeys = map[string]struct{}{ + "secrets": {}, + "runs_on": {}, + "release_token_app": {}, + "state_token_app": {}, +} + // mergeJSONTrees recursively merges the override tree onto the base tree in // place, implementing the deep-merge inheritance rule at the generic-JSON level: // when a key holds an object on both sides the objects merge recursively; a null // override is treated as unset and leaves the base untouched; anything else -// (scalar or array) replaces the base value. It is the merge core behind +// (scalar or array) replaces the base value. A key in mergeReplaceLeafKeys is a +// replace-leaf: it whole-replaces rather than recursing, so an exclusive or +// atomic block is never corrupted by a field-merge. It is the merge core behind // deepMergeComponentOverrides. func mergeJSONTrees(base, override map[string]any) { for key, ov := range override { if ov == nil { continue // an explicit null is treated as unset: inherit the base } - if ovMap, ok := ov.(map[string]any); ok { - if baseMap, ok := base[key].(map[string]any); ok { - mergeJSONTrees(baseMap, ovMap) - continue + if _, replaceLeaf := mergeReplaceLeafKeys[key]; !replaceLeaf { + if ovMap, ok := ov.(map[string]any); ok { + if baseMap, ok := base[key].(map[string]any); ok { + mergeJSONTrees(baseMap, ovMap) + continue + } } } base[key] = ov diff --git a/internal/config/components_deepmerge_test.go b/internal/config/components_deepmerge_test.go index aea46bd0..273fd57a 100644 --- a/internal/config/components_deepmerge_test.go +++ b/internal/config/components_deepmerge_test.go @@ -170,6 +170,114 @@ components: } } +// TestResolveComponent_SecretsBlockWholeReplaces proves the secrets block is a +// replace-leaf: it is XOR-exclusive (inherit ALL, or an explicit allow-list, +// never both), so a component override whole-replaces the inherited block rather +// than field-merging. A field-merge would combine an inherited "inherit" with a +// component allow-list, and generation gives inherit precedence, silently +// broadening the component's least-privilege scope. +func TestResolveComponent_SecretsBlockWholeReplaces(t *testing.T) { + // Shared inherit-all, component narrows to an explicit allow-list. + cfg := parseInline(t, ` +trunk_branch: main +environments: [dev, prod] +validate: + workflow: .github/workflows/validate.yaml + secrets: inherit +components: + api: + path: services/api + tag_prefix: api- + validate: + secrets: + NPM_TOKEN: NPM_TOKEN + web: + path: services/web + tag_prefix: web- +`) + api, err := cfg.ResolveComponent("api") + if err != nil { + t.Fatalf("ResolveComponent(api): %v", err) + } + sec := api.Config.Validate.Secrets + if sec == nil { + t.Fatal("api validate.secrets is nil, want the explicit allow-list") + } + if sec.Inherit { + t.Error("api narrowed secrets to an allow-list but Inherit stayed true (least-privilege broadened)") + } + if len(sec.Map) != 1 || sec.Map["NPM_TOKEN"] != "NPM_TOKEN" { + t.Errorf("api secrets map = %v, want only NPM_TOKEN", sec.Map) + } + // The sibling with no override inherits the shared inherit-all block intact. + web, err := cfg.ResolveComponent("web") + if err != nil { + t.Fatalf("ResolveComponent(web): %v", err) + } + if web.Config.Validate.Secrets == nil || !web.Config.Validate.Secrets.Inherit { + t.Errorf("web should inherit the shared secrets: inherit, got %#v", web.Config.Validate.Secrets) + } + + // Reverse: shared allow-list, component broadens back to inherit. + cfg2 := parseInline(t, ` +trunk_branch: main +environments: [dev, prod] +validate: + workflow: .github/workflows/validate.yaml + secrets: + SHARED_TOKEN: SHARED_TOKEN +components: + api: + path: services/api + tag_prefix: api- + validate: + secrets: inherit +`) + api2, err := cfg2.ResolveComponent("api") + if err != nil { + t.Fatalf("ResolveComponent(api) reverse: %v", err) + } + sec2 := api2.Config.Validate.Secrets + if sec2 == nil || !sec2.Inherit { + t.Errorf("api should override to secrets: inherit, got %#v", sec2) + } + if len(sec2.Map) != 0 { + t.Errorf("api secrets map should be empty after overriding to inherit, got %v", sec2.Map) + } +} + +// TestResolveComponent_RunsOnBlockWholeReplaces proves runs_on is a replace-leaf: +// a component that overrides the polymorphic runs_on block replaces it outright, +// so no field from the inherited form (a stale label or group) lingers. +func TestResolveComponent_RunsOnBlockWholeReplaces(t *testing.T) { + cfg := parseInline(t, ` +trunk_branch: main +environments: [dev, prod] +runs_on: + group: shared-group + labels: [self-hosted, linux] +components: + api: + path: services/api + tag_prefix: api- + runs_on: ubuntu-latest +`) + api, err := cfg.ResolveComponent("api") + if err != nil { + t.Fatalf("ResolveComponent(api): %v", err) + } + ro := api.Config.RunsOn + if ro == nil { + t.Fatal("api runs_on is nil, want the scalar override") + } + if ro.Label != "ubuntu-latest" { + t.Errorf("api runs_on label = %q, want ubuntu-latest", ro.Label) + } + if ro.Group != "" || len(ro.Labels) != 0 { + t.Errorf("api runs_on retained inherited fields (group=%q labels=%v); the block must whole-replace", ro.Group, ro.Labels) + } +} + // TestResolveComponent_FullBlockOverrideStillWins proves that when a component // sets every field of a nested block, the component's values win outright: deep // merge reduces to replacement when nothing is left to inherit.