From 33fa971c34a4e1cc60a8a27f8e2f24ff507190a6 Mon Sep 17 00:00:00 2001 From: Mohammad Abdolirad Date: Wed, 7 Oct 2026 01:05:49 +0200 Subject: [PATCH 1/3] build semantic plans in one constructor - derive WillRun inside semantic.New - remove stamp and attach helpers - drop view task-graph checks --- internal/plan/doc.go | 7 +- internal/plan/helm_action_test.go | 30 +--- internal/plan/semantic/chart_crd_test.go | 28 ++-- internal/plan/semantic/doc.go | 4 + internal/plan/semantic/drift.go | 38 ++--- internal/plan/semantic/drift_test.go | 40 +++-- internal/plan/semantic/order_test.go | 12 +- internal/plan/semantic/plan.go | 174 ++++++++++++-------- internal/plan/semantic/plan_test.go | 129 +++++++++++---- internal/plan/semantic/task_test.go | 52 ++---- internal/plan/semantic/types.go | 7 +- internal/plan/semantic_build.go | 18 +-- internal/plan/semantic_build_test.go | 102 +++++------- internal/plan/tasks.go | 26 +-- internal/plan/tasks_test.go | 94 +++-------- internal/plan/view/helpers_test.go | 38 ++++- internal/plan/view/human_chart_crd_test.go | 8 +- internal/plan/view/human_semantic_test.go | 2 - internal/plan/view/human_style_test.go | 6 +- internal/plan/view/human_test.go | 21 +-- internal/plan/view/json_test.go | 178 +++------------------ internal/plan/view/prepare.go | 38 ----- internal/plan/view/schema_test.go | 30 +--- 23 files changed, 445 insertions(+), 637 deletions(-) diff --git a/internal/plan/doc.go b/internal/plan/doc.go index 3947448..2dff509 100644 --- a/internal/plan/doc.go +++ b/internal/plan/doc.go @@ -24,8 +24,11 @@ // existing release reads Live through [LiveReader] with GET and LIST, // so Drift is Previous to Live. Chart CRD lifecycle comes from the // loaded documents, not from object diffs. The result is a -// [deployah.dev/deployah/internal/plan/semantic.Plan]. Those types -// live in plan/semantic. Their rendering lives in plan/view. +// [deployah.dev/deployah/internal/plan/semantic.Plan] from +// [deployah.dev/deployah/internal/plan/semantic.New]. That constructor +// derives whether each task will run. Task assembly in this package +// leaves WillRun false. Those types live in plan/semantic. Their +// rendering lives in plan/view. // // [LastSuccessfulRelease] remains for deploy hostname and workload // guards. [BuildSemanticPlan] does not use it. diff --git a/internal/plan/helm_action_test.go b/internal/plan/helm_action_test.go index 06c208e..45e768b 100644 --- a/internal/plan/helm_action_test.go +++ b/internal/plan/helm_action_test.go @@ -99,33 +99,7 @@ func configMapManifest(name, data string) string { return "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: " + name + "\n namespace: prod\ndata:\n key: " + data + "\n" } -func TestStampTaskWillRun_UnchangedHook(t *testing.T) { - t.Parallel() - unchanged := []semantic.TaskPlan{{ - Name: "seed", - Phase: semantic.TaskPreDeploy, - Action: semantic.TaskUnchanged, - }} - - tests := []struct { - name string - action semantic.HelmAction - wantRun bool - }{ - {name: "none leaves idle", action: semantic.HelmNone}, - {name: "upgrade will run", action: semantic.HelmUpgrade, wantRun: true}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - tasks := append([]semantic.TaskPlan(nil), unchanged...) - stampTaskWillRun(tasks, tt.action) - assert.Equal(t, tt.wantRun, tasks[0].WillRun) - }) - } -} - -func TestAssembleTasks_DoesNotStampWillRun(t *testing.T) { +func TestAssembleTasks_LeavesWillRunFalse(t *testing.T) { t.Parallel() resolved := resolvedWithTasks(map[string]spec.ResolvedTask{ "migrate": hookResolved(spec.TaskOnPreDeploy, 1), @@ -136,6 +110,4 @@ func TestAssembleTasks_DoesNotStampWillRun(t *testing.T) { require.NoError(t, err) require.Len(t, tasks, 1) assert.False(t, tasks[0].WillRun) - stampTaskWillRun(tasks, semantic.HelmInstall) - assert.True(t, tasks[0].WillRun) } diff --git a/internal/plan/semantic/chart_crd_test.go b/internal/plan/semantic/chart_crd_test.go index 7a77282..fd93e71 100644 --- a/internal/plan/semantic/chart_crd_test.go +++ b/internal/plan/semantic/chart_crd_test.go @@ -15,7 +15,6 @@ package semantic_test import ( - "reflect" "testing" "github.com/stretchr/testify/assert" @@ -26,7 +25,7 @@ import ( func TestAttachChartCRDs_Lifecycle(t *testing.T) { t.Parallel() - base, err := semantic.New(semantic.Header{FreshInstall: true}, semantic.HelmInstall, nil, nil) + base, err := semantic.New(semantic.Input{Header: semantic.Header{FreshInstall: true}, HelmAction: semantic.HelmInstall, Changes: nil, Tasks: nil}) require.NoError(t, err) changesBefore := len(base.Changes) tests := []struct { @@ -41,7 +40,7 @@ func TestAttachChartCRDs_Lifecycle(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - p, attachErr := semantic.AttachChartCRDs(base, []semantic.ChartCRD{{ + p, attachErr := withChartCRDs(base, []semantic.ChartCRD{{ Source: ".deployah/crds/widget.yaml", Kind: "CustomResourceDefinition", Name: "widgets.example.com", @@ -62,9 +61,9 @@ func TestAttachChartCRDs_Lifecycle(t *testing.T) { func TestAttachChartCRDs_PreservesOrder(t *testing.T) { t.Parallel() - base, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, nil, nil) + base, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: nil, Tasks: nil}) require.NoError(t, err) - p, err := semantic.AttachChartCRDs(base, []semantic.ChartCRD{ + p, err := withChartCRDs(base, []semantic.ChartCRD{ {Kind: "CustomResourceDefinition", Name: "one.example.com", Lifecycle: semantic.ChartCRDUpgrade}, {Kind: "CustomResourceDefinition", Name: "two.example.com", Index: 1, Lifecycle: semantic.ChartCRDUpgrade}, }) @@ -78,9 +77,9 @@ func TestAttachChartCRDs_PreservesOrder(t *testing.T) { func TestAttachChartCRDs_HasEffectsFollowsWillProcess(t *testing.T) { t.Parallel() - fresh, err := semantic.New(semantic.Header{FreshInstall: true}, semantic.HelmInstall, nil, nil) + fresh, err := semantic.New(semantic.Input{Header: semantic.Header{FreshInstall: true}, HelmAction: semantic.HelmInstall, Changes: nil, Tasks: nil}) require.NoError(t, err) - idle, err := semantic.New(semantic.Header{}, semantic.HelmNone, nil, nil) + idle, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmNone, Changes: nil, Tasks: nil}) require.NoError(t, err) require.True(t, idle.IsNoOp()) @@ -114,7 +113,7 @@ func TestAttachChartCRDs_HasEffectsFollowsWillProcess(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - p, attachErr := semantic.AttachChartCRDs(tc.base, []semantic.ChartCRD{{ + p, attachErr := withChartCRDs(tc.base, []semantic.ChartCRD{{ Kind: "CustomResourceDefinition", Name: "widgets.example.com", Lifecycle: tc.lifecycle, @@ -131,7 +130,7 @@ func TestAttachChartCRDs_HasEffectsFollowsWillProcess(t *testing.T) { func TestAttachChartCRDs_Validation(t *testing.T) { t.Parallel() - base, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, nil, nil) + base, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: nil, Tasks: nil}) require.NoError(t, err) tests := []struct { name string @@ -148,18 +147,9 @@ func TestAttachChartCRDs_Validation(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - _, attachErr := semantic.AttachChartCRDs(base, []semantic.ChartCRD{tc.crd}) + _, attachErr := withChartCRDs(base, []semantic.ChartCRD{tc.crd}) require.Error(t, attachErr) assert.ErrorContains(t, attachErr, tc.wantErr) }) } } - -func TestChartCRD_HasNoMutationFields(t *testing.T) { - t.Parallel() - rt := reflect.TypeFor[semantic.ChartCRD]() - for _, name := range []string{"APIVersion", "Action", "Before", "After", "Fields", "Namespace", "Scope", "Spec"} { - _, has := rt.FieldByName(name) - assert.False(t, has, name) - } -} diff --git a/internal/plan/semantic/doc.go b/internal/plan/semantic/doc.go index 35e62ee..6f227d0 100644 --- a/internal/plan/semantic/doc.go +++ b/internal/plan/semantic/doc.go @@ -14,6 +14,10 @@ // Package semantic is Deployah's unredacted semantic plan model. // +// [New] is the only constructor. It derives [TaskPlan.WillRun] from +// the Helm action and the task phase and action. This package does +// not read the cluster and does not render charts. +// // This package is not the deployah plan command and is not a JSON API. // Snapshots and field values stay complete. Serialization and secret // redaction belong to deployah.dev/deployah/internal/plan/view. diff --git a/internal/plan/semantic/drift.go b/internal/plan/semantic/drift.go index 9bc76d7..668d755 100644 --- a/internal/plan/semantic/drift.go +++ b/internal/plan/semantic/drift.go @@ -74,7 +74,7 @@ func driftActionRank(a DriftAction) int { } // DriftChange is one difference between Previous and Live. -// [AttachDrift] checks and stores it. It is cluster state, separate +// [New] checks and stores it. It is cluster state, separate // from a release change. // // Name is required, and GenerateName must be empty. Modified and @@ -85,7 +85,7 @@ func driftActionRank(a DriftAction) int { // Previous is unset for Unexpected and is never rewritten. Live is // unset for Missing. Modified Live keeps only the declared surface. // Unexpected Live has server bookkeeping removed. Only Modified has -// Fields. [AttachDrift] keeps them and does not recompute them. +// Fields. [New] keeps them and does not recompute them. type DriftChange struct { Resource ResourceRef Action DriftAction @@ -100,33 +100,25 @@ func (p Plan) HasDrift() bool { return len(p.Drift) > 0 } -// AttachDrift returns a copy of p with drift checked and sorted. A -// fresh install ([Header.FreshInstall]) accepts only an empty -// collection. Other plan fields stay as they are, including whether -// the plan has effects or is a no-op. -// -// Entries sort by group, kind, namespace, and name, then by apiVersion -// and action. Group comes from apiVersion. The same group, kind, -// namespace, and name twice is an error. -func AttachDrift(p Plan, drift []DriftChange) (Plan, error) { - copied := copyDrift(drift) - if p.Header.FreshInstall && len(copied) > 0 { - return Plan{}, fmt.Errorf("fresh install must not include drift") +// validateDriftSet checks drift before [New] sorts it. A fresh install +// accepts only an empty collection. Entries with the same group, kind, +// namespace, and name are an error. Group comes from apiVersion. +func validateDriftSet(header Header, drift []DriftChange) error { + if header.FreshInstall && len(drift) > 0 { + return fmt.Errorf("fresh install must not include drift") } - seen := make(map[string]struct{}, len(copied)) - for i := range copied { - if err := validateDrift(copied[i]); err != nil { - return Plan{}, fmt.Errorf("drift %d: %w", i, err) + seen := make(map[string]struct{}, len(drift)) + for i := range drift { + if err := validateDrift(drift[i]); err != nil { + return fmt.Errorf("drift %d: %w", i, err) } - key := driftIdentityKey(copied[i].Resource) + key := driftIdentityKey(drift[i].Resource) if _, dup := seen[key]; dup { - return Plan{}, fmt.Errorf("drift %s: duplicate logical identity", copied[i].Resource) + return fmt.Errorf("drift %s: duplicate logical identity", drift[i].Resource) } seen[key] = struct{}{} } - sortDrift(copied) - p.Drift = copied - return p, nil + return nil } func validateDrift(d DriftChange) error { diff --git a/internal/plan/semantic/drift_test.go b/internal/plan/semantic/drift_test.go index 1bc5075..786743c 100644 --- a/internal/plan/semantic/drift_test.go +++ b/internal/plan/semantic/drift_test.go @@ -25,7 +25,7 @@ import ( func TestNew_DriftStartsEmpty(t *testing.T) { t.Parallel() - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, nil, nil) + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: nil, Tasks: nil}) require.NoError(t, err) require.NotNil(t, p.Drift) assert.Empty(t, p.Drift) @@ -48,7 +48,7 @@ func TestAttachDrift_FreshInstallRejectsDrift(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got, err := semantic.AttachDrift(base, tt.drift) + got, err := withDrift(base, tt.drift) if tt.wantErr != "" { require.Error(t, err) assert.ErrorContains(t, err, tt.wantErr) @@ -145,7 +145,7 @@ func TestAttachDrift_Validation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - _, err := semantic.AttachDrift(base, []semantic.DriftChange{tt.drift}) + _, err := withDrift(base, []semantic.DriftChange{tt.drift}) require.Error(t, err) assert.ErrorContains(t, err, tt.wantErr) }) @@ -158,7 +158,7 @@ func TestAttachDrift_DuplicateLogicalIdentity(t *testing.T) { first := missingDrift("app") second := missingDrift("app") second.Resource.APIVersion = "core/v1" - _, err := semantic.AttachDrift(base, []semantic.DriftChange{first, second}) + _, err := withDrift(base, []semantic.DriftChange{first, second}) require.Error(t, err) assert.ErrorContains(t, err, "duplicate logical identity") } @@ -166,7 +166,7 @@ func TestAttachDrift_DuplicateLogicalIdentity(t *testing.T) { func TestAttachDrift_SortsAndCopies(t *testing.T) { t.Parallel() change := updateChange("app", "v1", "v2") - base, err := semantic.New(semantic.Header{Release: "web"}, semantic.HelmUpgrade, []semantic.ResourceChange{change}, nil) + base, err := semantic.New(semantic.Input{Header: semantic.Header{Release: "web"}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{change}, Tasks: nil}) require.NoError(t, err) summary := base.Summary @@ -180,7 +180,7 @@ func TestAttachDrift_SortsAndCopies(t *testing.T) { {Path: "/a", Op: semantic.FieldReplace, Before: "1", After: "2"}, } input := []semantic.DriftChange{zeta, alpha, mod} - got, err := semantic.AttachDrift(base, input) + got, err := withDrift(base, input) require.NoError(t, err) require.Len(t, got.Drift, 3) // Group sorts before kind, so core ConfigMaps precede apps/Deployment. @@ -210,9 +210,9 @@ func TestAttachDrift_SortsAndCopies(t *testing.T) { func TestAttachDrift_DoesNotChangeReleaseIntent(t *testing.T) { t.Parallel() - base, err := semantic.New(semantic.Header{Release: "web"}, semantic.HelmNone, nil, nil) + base, err := semantic.New(semantic.Input{Header: semantic.Header{Release: "web"}, HelmAction: semantic.HelmNone, Changes: nil, Tasks: nil}) require.NoError(t, err) - got, err := semantic.AttachDrift(base, []semantic.DriftChange{missingDrift("app")}) + got, err := withDrift(base, []semantic.DriftChange{missingDrift("app")}) require.NoError(t, err) assert.Equal(t, semantic.HelmNone, got.HelmAction) assert.Empty(t, got.Changes) @@ -243,11 +243,33 @@ func TestDriftAction_String(t *testing.T) { func mustPlan(t *testing.T, header semantic.Header, action semantic.HelmAction) semantic.Plan { t.Helper() - p, err := semantic.New(header, action, nil, nil) + p, err := semantic.New(semantic.Input{Header: header, HelmAction: action, Changes: nil, Tasks: nil}) require.NoError(t, err) return p } +func withDrift(base semantic.Plan, drift []semantic.DriftChange) (semantic.Plan, error) { + return semantic.New(semantic.Input{ + Header: base.Header, + HelmAction: base.HelmAction, + Changes: base.Changes, + Tasks: base.Tasks, + Drift: drift, + ChartCRDs: base.ChartCRDs, + }) +} + +func withChartCRDs(base semantic.Plan, crds []semantic.ChartCRD) (semantic.Plan, error) { + return semantic.New(semantic.Input{ + Header: base.Header, + HelmAction: base.HelmAction, + Changes: base.Changes, + Tasks: base.Tasks, + Drift: base.Drift, + ChartCRDs: crds, + }) +} + func missingDrift(name string) semantic.DriftChange { return semantic.DriftChange{ Resource: ref("ConfigMap", name), diff --git a/internal/plan/semantic/order_test.go b/internal/plan/semantic/order_test.go index 3ae0a58..d16a234 100644 --- a/internal/plan/semantic/order_test.go +++ b/internal/plan/semantic/order_test.go @@ -89,7 +89,7 @@ func TestNew_OrderTieBreakers(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, tt.changes, nil) + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: tt.changes, Tasks: nil}) require.NoError(t, err) require.Len(t, p.Changes, len(tt.want)) got := make([]semantic.ResourceRef, 0, len(p.Changes)) @@ -104,7 +104,7 @@ func TestNew_OrderTieBreakers(t *testing.T) { func TestNew_ActionRank(t *testing.T) { t.Parallel() res := ref("ConfigMap", "app") - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, []semantic.ResourceChange{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{ { Resource: res, Action: semantic.Delete, @@ -122,7 +122,7 @@ func TestNew_ActionRank(t *testing.T) { Action: semantic.Create, After: &semantic.ResourceSnapshot{Object: cm("app", "v1")}, }, - }, nil) + }, Tasks: nil}) require.NoError(t, err) require.Len(t, p.Changes, 3) assert.Equal(t, []semantic.Action{ @@ -134,7 +134,7 @@ func TestNew_ActionRank(t *testing.T) { func TestNew_FieldChangeOrder(t *testing.T) { t.Parallel() - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, []semantic.ResourceChange{{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{{ Resource: ref("ConfigMap", "app"), Action: semantic.Update, Before: &semantic.ResourceSnapshot{Object: map[string]any{"z": "1", "a": "1", "m": "1"}}, @@ -144,7 +144,7 @@ func TestNew_FieldChangeOrder(t *testing.T) { {Path: "/a", Op: semantic.FieldReplace, Before: "1", After: "2"}, {Path: "/m", Op: semantic.FieldReplace, Before: "1", After: "2"}, }, - }}, nil) + }}, Tasks: nil}) require.NoError(t, err) require.Len(t, p.Changes[0].Fields, 3) assert.Equal(t, []string{"/a", "/m", "/z"}, []string{ @@ -162,7 +162,7 @@ func TestNew_HelmOrderThenRef(t *testing.T) { tiedHigh.HelmOrder = 1 tiedLow := createChange("a", "v1") tiedLow.HelmOrder = 1 - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, []semantic.ResourceChange{later, tiedHigh, tiedLow}, nil) + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{later, tiedHigh, tiedLow}, Tasks: nil}) require.NoError(t, err) require.Len(t, p.Changes, 3) assert.Equal(t, "a", p.Changes[0].Resource.Name) diff --git a/internal/plan/semantic/plan.go b/internal/plan/semantic/plan.go index 272a750..5147eb4 100644 --- a/internal/plan/semantic/plan.go +++ b/internal/plan/semantic/plan.go @@ -21,9 +21,8 @@ import ( // Plan is one run's Helm release intent, its Previous-to-Desired // resource changes, and its Previous-to-Live drift. Build it with -// [New]. Add chart CRDs with [AttachChartCRDs] and drift with -// [AttachDrift]. Snapshots still contain secrets, and this type is -// not the plan JSON document. +// [New]. Snapshots still contain secrets, and this type is not the +// plan JSON document. type Plan struct { Header Header HelmAction HelmAction @@ -34,78 +33,128 @@ type Plan struct { Summary Summary } -// New checks and sorts its inputs, fills [Summary], and returns -// non-nil slices. Chart CRDs and drift stay empty. It returns an -// error if a task reference is missing, ambiguous, or shared. -// helmAction and field changes are stored as given. Drift is -// observed cluster state and is not an input here. -func New(header Header, helmAction HelmAction, changes []ResourceChange, tasks []TaskPlan) (Plan, error) { - if !helmAction.valid() { - return Plan{}, fmt.Errorf("invalid helm action %s", helmAction) - } - if err := validateHelmAction(header, helmAction); err != nil { - return Plan{}, err - } +// Input is the caller data for [New]. Task WillRun values in Tasks +// are ignored. [New] derives them. +type Input struct { + Header Header + HelmAction HelmAction + Changes []ResourceChange + Tasks []TaskPlan + Drift []DriftChange + ChartCRDs []ChartCRD +} - copiedChanges := slices.Clone(changes) - if copiedChanges == nil { - copiedChanges = []ResourceChange{} +// New copies in and returns a complete plan, or a zero Plan and an +// error. Nil slices become empty slices. +// +// Order is: copy and normalize, enum and shape checks, derived +// WillRun, Helm and content checks, drift and chart CRD checks, sort, +// then [Summary]. Summary counts resource changes only. Chart CRDs +// stay in the order given. Changes, tasks, and drift are sorted. +func New(in Input) (Plan, error) { + changes := normalizeChanges(in.Changes) + tasks, normErr := normalizeTasks(in.Tasks) + if normErr != nil { + return Plan{}, normErr + } + drift := copyDrift(in.Drift) + crds := slices.Clone(in.ChartCRDs) + if crds == nil { + crds = []ChartCRD{} } - copiedTasks := copyTasks(tasks) - for i := range copiedChanges { - if err := validateChange(copiedChanges[i]); err != nil { - return Plan{}, fmt.Errorf("resource %s: %w", copiedChanges[i].Resource, err) - } - normalized, nerr := normalizeChange(copiedChanges[i]) - if nerr != nil { - return Plan{}, fmt.Errorf("resource %s: %w", copiedChanges[i].Resource, nerr) - } - copiedChanges[i] = normalized + if !in.HelmAction.valid() { + return Plan{}, fmt.Errorf("invalid helm action %s", in.HelmAction) + } + if err := validateHelmAction(in.Header, in.HelmAction); err != nil { + return Plan{}, err } - for i := range copiedTasks { - normalized, nerr := normalizeTask(copiedTasks[i]) - if nerr != nil { - return Plan{}, fmt.Errorf("task %s: %w", copiedTasks[i].Name, nerr) + for i := range changes { + if err := validateChange(changes[i]); err != nil { + return Plan{}, fmt.Errorf("resource %s: %w", changes[i].Resource, err) } - copiedTasks[i] = normalized } - if err := validateTasks(copiedTasks, copiedChanges); err != nil { + if err := validateTasks(tasks, changes); err != nil { + return Plan{}, err + } + + deriveWillRun(tasks, in.HelmAction) + + if err := validateHelmContent(in.HelmAction, changes, tasks); err != nil { return Plan{}, err } - if err := validateHelmContent(helmAction, copiedChanges, copiedTasks); err != nil { + if err := validateDriftSet(in.Header, drift); err != nil { + return Plan{}, err + } + if err := validateChartCRDs(crds); err != nil { return Plan{}, err } - sortChanges(copiedChanges) - sortTasks(copiedTasks, copiedChanges) + sortChanges(changes) + sortTasks(tasks, changes) + sortDrift(drift) return Plan{ - Header: header, - HelmAction: helmAction, - Changes: copiedChanges, - Tasks: copiedTasks, - Drift: []DriftChange{}, - ChartCRDs: []ChartCRD{}, - Summary: Summarize(copiedChanges), + Header: in.Header, + HelmAction: in.HelmAction, + Changes: changes, + Tasks: tasks, + Drift: drift, + ChartCRDs: crds, + Summary: Summarize(changes), }, nil } -// AttachChartCRDs returns a copy of p with crds attached in order. -// [Plan.Changes] and [Plan.Summary] stay the same. -func AttachChartCRDs(p Plan, crds []ChartCRD) (Plan, error) { - copied := slices.Clone(crds) - if copied == nil { - copied = []ChartCRD{} - } - for i, c := range copied { +func normalizeChanges(in []ResourceChange) []ResourceChange { + out := slices.Clone(in) + if out == nil { + out = []ResourceChange{} + } + for i := range out { + out[i] = normalizeChange(out[i]) + } + return out +} + +func normalizeTasks(in []TaskPlan) ([]TaskPlan, error) { + out := copyTasks(in) + for i := range out { + normalized, err := normalizeTask(out[i]) + if err != nil { + return nil, fmt.Errorf("task %s: %w", out[i].Name, err) + } + out[i] = normalized + } + return out, nil +} + +// deriveWillRun overwrites caller WillRun. Schedule and delete tasks +// do not run. Other preDeploy and postDeploy tasks run only when the +// Helm action is install or upgrade. +func deriveWillRun(tasks []TaskPlan, helmAction HelmAction) { + transition := helmAction == HelmInstall || helmAction == HelmUpgrade + for i := range tasks { + tasks[i].WillRun = false + if !transition { + continue + } + if tasks[i].Phase != TaskPreDeploy && tasks[i].Phase != TaskPostDeploy { + continue + } + if tasks[i].Action == TaskDelete { + continue + } + tasks[i].WillRun = true + } +} + +func validateChartCRDs(crds []ChartCRD) error { + for i, c := range crds { if err := validateChartCRD(c); err != nil { - return Plan{}, fmt.Errorf("chart crd %d: %w", i, err) + return fmt.Errorf("chart crd %d: %w", i, err) } } - p.ChartCRDs = copied - // WillProcess still counts in HasEffects. It is not a resource change. - return p, nil + return nil } func validateChartCRD(c ChartCRD) error { @@ -153,14 +202,14 @@ func (p Plan) IsNoOp() bool { !p.HasEffects() } -func normalizeChange(c ResourceChange) (ResourceChange, error) { +func normalizeChange(c ResourceChange) ResourceChange { c.Before = copySnapshot(c.Before) c.After = copySnapshot(c.After) c.Fields = copyFields(c.Fields) if c.Fields == nil { c.Fields = []FieldChange{} } - return c, nil + return c } func normalizeTask(t TaskPlan) (TaskPlan, error) { @@ -225,17 +274,11 @@ func validateTask(t TaskPlan, changeIndex map[string]int, owned map[string]strin if !t.Action.valid() { return fmt.Errorf("invalid action %s", t.Action) } - if t.Action == TaskDelete && t.WillRun { - return fmt.Errorf("delete must not will run") - } switch t.Phase { case TaskSchedule: if len(t.Definitions) > 0 { return fmt.Errorf("schedule must not have hook definitions") } - if t.WillRun { - return fmt.Errorf("schedule must not will run") - } for _, ref := range t.Resources { key := ref.refKey() idx, ok := changeIndex[key] @@ -339,9 +382,6 @@ func validateHelmContent(helmAction HelmAction, changes []ResourceChange, tasks if t.Action != TaskUnchanged { return fmt.Errorf("helm none must not include changed %s task %s", t.Phase, t.Name) } - if t.WillRun { - return fmt.Errorf("helm none must not will run %s task %s", t.Phase, t.Name) - } } return nil } diff --git a/internal/plan/semantic/plan_test.go b/internal/plan/semantic/plan_test.go index 4b4924d..eae0799 100644 --- a/internal/plan/semantic/plan_test.go +++ b/internal/plan/semantic/plan_test.go @@ -62,7 +62,7 @@ func TestNew_SnapshotInvariants(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - p, err := semantic.New(header, semantic.HelmUpgrade, []semantic.ResourceChange{tt.change}, nil) + p, err := semantic.New(semantic.Input{Header: header, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{tt.change}, Tasks: nil}) require.NoError(t, err) require.Len(t, p.Changes, 1) c := p.Changes[0] @@ -92,7 +92,7 @@ func TestNew_UpdateRequiresAfterAndFields(t *testing.T) { Before: &semantic.ResourceSnapshot{Object: cm("app", "v1")}, Fields: []semantic.FieldChange{{Path: "/data/key", Op: semantic.FieldReplace, Before: "v1", After: "v2"}}, } - _, err := semantic.New(header, semantic.HelmUpgrade, []semantic.ResourceChange{missingAfter}, nil) + _, err := semantic.New(semantic.Input{Header: header, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{missingAfter}, Tasks: nil}) require.Error(t, err) assert.ErrorContains(t, err, "update requires an after snapshot") @@ -102,14 +102,14 @@ func TestNew_UpdateRequiresAfterAndFields(t *testing.T) { Before: &semantic.ResourceSnapshot{Object: cm("app", "v1")}, After: &semantic.ResourceSnapshot{Object: cm("app", "v2")}, } - _, err = semantic.New(header, semantic.HelmUpgrade, []semantic.ResourceChange{missingFields}, nil) + _, err = semantic.New(semantic.Input{Header: header, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{missingFields}, Tasks: nil}) require.Error(t, err) assert.ErrorContains(t, err, "update requires a field change") } func TestNew_TasksAlwaysNonNil(t *testing.T) { t.Parallel() - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, nil, nil) + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: nil, Tasks: nil}) require.NoError(t, err) require.NotNil(t, p.Tasks) assert.Empty(t, p.Tasks) @@ -123,11 +123,11 @@ func TestNew_TasksAlwaysNonNil(t *testing.T) { func TestNew_SummaryDerived(t *testing.T) { t.Parallel() - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, []semantic.ResourceChange{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{ createChange("a", "1"), updateChange("b", "1", "2"), deleteChange("c", "1"), - }, nil) + }, Tasks: nil}) require.NoError(t, err) assert.Equal(t, semantic.Summary{Create: 1, Update: 1, Delete: 1}, p.Summary) assert.Equal(t, 3, p.Summary.Total()) @@ -135,11 +135,11 @@ func TestNew_SummaryDerived(t *testing.T) { func TestNew_DeterministicOrder(t *testing.T) { t.Parallel() - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, []semantic.ResourceChange{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{ deleteChange("z", "1"), createChange("a", "1"), updateChange("m", "1", "2"), - }, nil) + }, Tasks: nil}) require.NoError(t, err) require.Len(t, p.Changes, 3) assert.Equal(t, "a", p.Changes[0].Resource.Name) @@ -156,11 +156,11 @@ func TestNew_GoIntInSnapshot(t *testing.T) { "metadata": map[string]any{"name": "web", "namespace": "prod"}, "spec": map[string]any{"replicas": 1}, } - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, []semantic.ResourceChange{{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{{ Resource: ref("Deployment", "web"), Action: semantic.Create, After: &semantic.ResourceSnapshot{Object: obj}, - }}, nil) + }}, Tasks: nil}) require.NoError(t, err) require.NotNil(t, p.Changes[0].After) spec, ok := p.Changes[0].After.Object["spec"].(map[string]any) @@ -183,7 +183,7 @@ func TestNew_DoesNotMutateCallerSnapshots(t *testing.T) { Action: semantic.Create, After: &semantic.ResourceSnapshot{Object: obj}, } - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, []semantic.ResourceChange{change}, nil) + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{change}, Tasks: nil}) require.NoError(t, err) setObjectString(t, obj, "mutated", "data", "key") assert.Equal(t, "v1", objectString(t, p.Changes[0].After.Object, "data", "key")) @@ -197,13 +197,13 @@ func TestNew_KeepsCallerFields(t *testing.T) { {Path: "/z", Op: semantic.FieldReplace, Before: "1", After: "9"}, {Path: "/a", Op: semantic.FieldReplace, Before: "1", After: "2"}, } - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, []semantic.ResourceChange{{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{{ Resource: ref("ConfigMap", "app"), Action: semantic.Update, Before: &semantic.ResourceSnapshot{Object: before}, After: &semantic.ResourceSnapshot{Object: after}, Fields: fields, - }}, nil) + }}, Tasks: nil}) require.NoError(t, err) require.Len(t, p.Changes[0].Fields, 2) assert.Equal(t, "/a", p.Changes[0].Fields[0].Path) @@ -334,7 +334,7 @@ func TestNew_RejectsInvalid(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - _, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, []semantic.ResourceChange{tt.change}, nil) + _, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{tt.change}, Tasks: nil}) require.Error(t, err) assert.ErrorContains(t, err, tt.wantErr) assert.ErrorContains(t, err, res.String()) @@ -354,11 +354,11 @@ func TestNew_CopiesNestedSnapshotShapes(t *testing.T) { "metadata": map[string]any{"name": "app", "namespace": "prod", "labels": labels}, "data": map[string]any{"args": args, "nested": nested, "inner": inner, "empty": map[string]any(nil)}, } - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, []semantic.ResourceChange{{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{{ Resource: ref("ConfigMap", "app"), Action: semantic.Create, After: &semantic.ResourceSnapshot{Object: obj}, - }}, nil) + }}, Tasks: nil}) require.NoError(t, err) assert.Empty(t, p.Changes[0].Fields) @@ -381,13 +381,13 @@ func TestNew_CopiesNestedSnapshotShapes(t *testing.T) { func TestNew_UpdateEmptyBeforeObject(t *testing.T) { t.Parallel() - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, []semantic.ResourceChange{{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{{ Resource: ref("ConfigMap", "app"), Action: semantic.Update, Before: &semantic.ResourceSnapshot{}, After: &semantic.ResourceSnapshot{Object: cm("app", "v1")}, Fields: []semantic.FieldChange{{Path: "/data/key", Op: semantic.FieldReplace, Before: "v1", After: "v2"}}, - }}, nil) + }}, Tasks: nil}) require.NoError(t, err) require.Len(t, p.Changes[0].Fields, 1) assert.Equal(t, "/data/key", p.Changes[0].Fields[0].Path) @@ -395,11 +395,11 @@ func TestNew_UpdateEmptyBeforeObject(t *testing.T) { func TestNew_NilSnapshotObject(t *testing.T) { t.Parallel() - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, []semantic.ResourceChange{{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{{ Resource: ref("ConfigMap", "app"), Action: semantic.Create, After: &semantic.ResourceSnapshot{}, - }}, nil) + }}, Tasks: nil}) require.NoError(t, err) require.NotNil(t, p.Changes[0].After) assert.Nil(t, p.Changes[0].After.Object) @@ -442,7 +442,7 @@ func TestNew_HelmActionHeader(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - p, err := semantic.New(tt.header, tt.helmAction, nil, nil) + p, err := semantic.New(semantic.Input{Header: tt.header, HelmAction: tt.helmAction, Changes: nil, Tasks: nil}) if tt.wantErr != "" { require.Error(t, err) assert.ErrorContains(t, err, tt.wantErr) @@ -478,14 +478,13 @@ func TestNew_HelmNoneContent(t *testing.T) { wantErr: "helm none must not include changed preDeploy task migrate", }, { - name: "will run preDeploy", + name: "incoming will run is ignored", tasks: []semantic.TaskPlan{{ Name: "migrate", Phase: semantic.TaskPreDeploy, Action: semantic.TaskUnchanged, WillRun: true, }}, - wantErr: "helm none must not will run preDeploy task migrate", }, { name: "unchanged idle hook", @@ -499,13 +498,17 @@ func TestNew_HelmNoneContent(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - _, err := semantic.New(semantic.Header{}, semantic.HelmNone, tt.changes, tt.tasks) + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmNone, Changes: tt.changes, Tasks: tt.tasks}) if tt.wantErr != "" { require.Error(t, err) assert.ErrorContains(t, err, tt.wantErr) return } require.NoError(t, err) + if tt.name == "incoming will run is ignored" { + require.Len(t, p.Tasks, 1) + assert.False(t, p.Tasks[0].WillRun) + } }) } } @@ -595,12 +598,14 @@ func TestPlan_HasEffects(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - p, err := semantic.New(tt.header, tt.helmAction, tt.changes, tt.tasks) + p, err := semantic.New(semantic.Input{ + Header: tt.header, + HelmAction: tt.helmAction, + Changes: tt.changes, + Tasks: tt.tasks, + ChartCRDs: tt.crds, + }) require.NoError(t, err) - if len(tt.crds) > 0 { - p, err = semantic.AttachChartCRDs(p, tt.crds) - require.NoError(t, err) - } assert.Equal(t, tt.want, p.HasEffects()) }) } @@ -633,9 +638,73 @@ func TestPlan_IsNoOp(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - p, err := semantic.New(tt.header, tt.helmAction, tt.changes, tt.tasks) + p, err := semantic.New(semantic.Input{Header: tt.header, HelmAction: tt.helmAction, Changes: tt.changes, Tasks: tt.tasks}) require.NoError(t, err) assert.Equal(t, tt.want, p.IsNoOp()) }) } } + +func TestNew_DerivesWillRun(t *testing.T) { + t.Parallel() + schedule := semantic.TaskPlan{Name: "cleanup", Phase: semantic.TaskSchedule, Action: semantic.TaskUnchanged, WillRun: true} + deleteHook := semantic.TaskPlan{Name: "old", Phase: semantic.TaskPreDeploy, Action: semantic.TaskDelete, WillRun: true} + idle := semantic.TaskPlan{Name: "migrate", Phase: semantic.TaskPreDeploy, Action: semantic.TaskUnchanged, WillRun: true} + tests := []struct { + name string + header semantic.Header + action semantic.HelmAction + task semantic.TaskPlan + want bool + }{ + {name: "schedule on install", header: semantic.Header{FreshInstall: true}, action: semantic.HelmInstall, task: schedule}, + {name: "delete on upgrade", action: semantic.HelmUpgrade, task: deleteHook}, + {name: "install hook", header: semantic.Header{FreshInstall: true}, action: semantic.HelmInstall, task: idle, want: true}, + {name: "upgrade hook", action: semantic.HelmUpgrade, task: idle, want: true}, + {name: "helm none hook", action: semantic.HelmNone, task: idle}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + p, err := semantic.New(semantic.Input{ + Header: tt.header, + HelmAction: tt.action, + Tasks: []semantic.TaskPlan{tt.task}, + }) + require.NoError(t, err) + require.Len(t, p.Tasks, 1) + assert.Equal(t, tt.want, p.Tasks[0].WillRun) + }) + } +} + +func TestNew_ChartCRDAndDriftIgnoreSummary(t *testing.T) { + t.Parallel() + p, err := semantic.New(semantic.Input{ + Header: semantic.Header{}, + HelmAction: semantic.HelmUpgrade, + Changes: []semantic.ResourceChange{createChange("app", "v1")}, + Drift: []semantic.DriftChange{missingDrift("other")}, + ChartCRDs: []semantic.ChartCRD{{ + Kind: "CustomResourceDefinition", + Name: "widgets.example.com", + Lifecycle: semantic.ChartCRDProcess, + WillProcess: true, + }}, + }) + require.NoError(t, err) + assert.Equal(t, semantic.Summary{Create: 1}, p.Summary) + assert.True(t, p.HasEffects()) + assert.True(t, p.HasDrift()) + assert.False(t, p.IsNoOp()) + + without, err := semantic.New(semantic.Input{ + Header: semantic.Header{}, + HelmAction: semantic.HelmUpgrade, + Drift: []semantic.DriftChange{missingDrift("other")}, + }) + require.NoError(t, err) + assert.Equal(t, semantic.Summary{}, without.Summary) + assert.False(t, without.HasEffects()) + assert.True(t, without.HasDrift()) +} diff --git a/internal/plan/semantic/task_test.go b/internal/plan/semantic/task_test.go index 35d8ea6..e529fb0 100644 --- a/internal/plan/semantic/task_test.go +++ b/internal/plan/semantic/task_test.go @@ -101,26 +101,6 @@ func TestNew_TaskValidation(t *testing.T) { }}, wantErr: "schedule must not have hook definitions", }, - { - name: "schedule with WillRun", - tasks: []semantic.TaskPlan{{ - Name: "cleanup", - Phase: semantic.TaskSchedule, - Action: semantic.TaskUnchanged, - WillRun: true, - }}, - wantErr: "schedule must not will run", - }, - { - name: "delete with WillRun", - tasks: []semantic.TaskPlan{{ - Name: "migrate", - Phase: semantic.TaskPreDeploy, - Action: semantic.TaskDelete, - WillRun: true, - }}, - wantErr: "delete must not will run", - }, { name: "duplicate task name", tasks: []semantic.TaskPlan{ @@ -172,7 +152,7 @@ func TestNew_TaskValidation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - _, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, tt.changes, tt.tasks) + _, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: tt.changes, Tasks: tt.tasks}) require.Error(t, err) assert.ErrorContains(t, err, tt.wantErr) }) @@ -218,7 +198,7 @@ func TestNew_HookDefinitionFields(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - _, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, nil, task(tt.def)) + _, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: nil, Tasks: task(tt.def)}) require.Error(t, err) assert.ErrorContains(t, err, tt.wantErr) }) @@ -226,13 +206,13 @@ func TestNew_HookDefinitionFields(t *testing.T) { t.Run("update keeps caller fields", func(t *testing.T) { t.Parallel() - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, nil, task(semantic.HookDefinition{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: nil, Tasks: task(semantic.HookDefinition{ Resource: app, Action: semantic.Update, Before: before, After: after, Fields: fields, - })) + })}) require.NoError(t, err) require.Len(t, p.Tasks[0].Definitions[0].Fields, 1) assert.Equal(t, "/data/key", p.Tasks[0].Definitions[0].Fields[0].Path) @@ -244,12 +224,12 @@ func TestNew_HookDefinitionFields(t *testing.T) { func TestNew_ValidScheduleReference(t *testing.T) { t.Parallel() app := ref("ConfigMap", "app") - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, []semantic.ResourceChange{createChange("app", "v1")}, []semantic.TaskPlan{{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{createChange("app", "v1")}, Tasks: []semantic.TaskPlan{{ Name: "cleanup", Phase: semantic.TaskSchedule, Action: semantic.TaskCreate, Resources: []semantic.ResourceRef{app}, - }}) + }}}) require.NoError(t, err) require.Len(t, p.Tasks, 1) require.Len(t, p.Tasks[0].Resources, 1) @@ -260,7 +240,7 @@ func TestNew_ValidScheduleReference(t *testing.T) { func TestNew_SummaryIgnoresHookDefinitions(t *testing.T) { t.Parallel() - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, nil, []semantic.TaskPlan{{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: nil, Tasks: []semantic.TaskPlan{{ Name: "migrate", Phase: semantic.TaskPreDeploy, Action: semantic.TaskCreate, @@ -270,7 +250,7 @@ func TestNew_SummaryIgnoresHookDefinitions(t *testing.T) { Action: semantic.Create, After: &semantic.ResourceSnapshot{Object: cm("migrate", "v1")}, }}, - }}) + }}}) require.NoError(t, err) assert.Equal(t, 0, p.Summary.Total()) require.Len(t, p.Tasks, 1) @@ -280,13 +260,13 @@ func TestNew_SummaryIgnoresHookDefinitions(t *testing.T) { func TestNew_TaskSort(t *testing.T) { t.Parallel() - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, nil, []semantic.TaskPlan{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: nil, Tasks: []semantic.TaskPlan{ {Name: "z", Phase: semantic.TaskPreDeploy, Action: semantic.TaskUnchanged, WillRun: true, HookWeight: 2}, {Name: "cleanup", Phase: semantic.TaskSchedule, Action: semantic.TaskUnchanged}, {Name: "a", Phase: semantic.TaskPreDeploy, Action: semantic.TaskUnchanged, WillRun: true, HookWeight: 2}, {Name: "smoke", Phase: semantic.TaskPostDeploy, Action: semantic.TaskCreate, WillRun: true}, {Name: "migrate", Phase: semantic.TaskPreDeploy, Action: semantic.TaskUnchanged, WillRun: true, HookWeight: 1}, - }) + }}) require.NoError(t, err) require.Len(t, p.Tasks, 5) assert.Equal(t, []string{"migrate", "a", "z", "smoke", "cleanup"}, []string{ @@ -348,7 +328,7 @@ func TestNew_NestedTaskOrder(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, tt.changes, []semantic.TaskPlan{tt.task}) + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: tt.changes, Tasks: []semantic.TaskPlan{tt.task}}) require.NoError(t, err) require.Len(t, p.Tasks, 1) gotDefs := make([]string, 0, len(p.Tasks[0].Definitions)) @@ -371,7 +351,7 @@ func TestNew_HelmOrderSortsChanges(t *testing.T) { a.HelmOrder = 2 z := createChange("z", "1") z.HelmOrder = 1 - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, []semantic.ResourceChange{a, z}, nil) + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{a, z}, Tasks: nil}) require.NoError(t, err) require.Len(t, p.Changes, 2) assert.Equal(t, "z", p.Changes[0].Resource.Name) @@ -380,12 +360,12 @@ func TestNew_HelmOrderSortsChanges(t *testing.T) { func TestNew_NonNilNestedTaskSlices(t *testing.T) { t.Parallel() - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, nil, []semantic.TaskPlan{{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: nil, Tasks: []semantic.TaskPlan{{ Name: "seed", Phase: semantic.TaskPreDeploy, Action: semantic.TaskUnchanged, WillRun: true, - }}) + }}}) require.NoError(t, err) require.NotNil(t, p.Tasks[0].Definitions) assert.Empty(t, p.Tasks[0].Definitions) @@ -403,13 +383,13 @@ func TestNew_DoesNotAliasCallerTasks(t *testing.T) { After: &semantic.ResourceSnapshot{Object: cm("env", "v2")}, Fields: fields, }} - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, nil, []semantic.TaskPlan{{ + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: nil, Tasks: []semantic.TaskPlan{{ Name: "migrate", Phase: semantic.TaskPreDeploy, Action: semantic.TaskUpdate, WillRun: true, Definitions: defs, - }}) + }}}) require.NoError(t, err) defs[0].Resource.Name = "mutated" fields[0].After = "mutated" diff --git a/internal/plan/semantic/types.go b/internal/plan/semantic/types.go index 4440d1d..45e4c98 100644 --- a/internal/plan/semantic/types.go +++ b/internal/plan/semantic/types.go @@ -266,8 +266,11 @@ type TaskPlan struct { Name string Phase TaskPhase Action TaskAction - // WillRun reports whether the task's hook takes part in the Helm - // transition the plan describes. It does not predict whether a later + // WillRun reports whether this task's hook takes part in the Helm + // transition. [New] sets it and ignores a caller value. Schedule + // and delete tasks do not run. Other preDeploy and postDeploy + // tasks run for [HelmInstall] and [HelmUpgrade], and do not run + // for [HelmNone]. It does not predict whether a later // `deployah deploy` skips the hook. WillRun bool Definitions []HookDefinition diff --git a/internal/plan/semantic_build.go b/internal/plan/semantic_build.go index 588dd8f..e33542d 100644 --- a/internal/plan/semantic_build.go +++ b/internal/plan/semantic_build.go @@ -156,20 +156,18 @@ func BuildSemanticPlan( if err != nil { return semantic.Plan{}, nil, cleanup, fmt.Errorf("assemble semantic plan: %w", err) } - stampTaskWillRun(tasks, helmAction) - p, err := semantic.New(header, helmAction, changes, tasks) - if err != nil { - return semantic.Plan{}, nil, cleanup, fmt.Errorf("assemble semantic plan: %w", err) - } - p, err = semantic.AttachChartCRDs(p, chartCRDsFromDocs(input.CRDDocs, prep.Operation, input.SkipCRDs)) - if err != nil { - return semantic.Plan{}, nil, cleanup, fmt.Errorf("assemble semantic plan: %w", err) - } drift, err := observeDrift(ctx, live, previous, header) if err != nil { return semantic.Plan{}, nil, cleanup, fmt.Errorf("observe drift: %w", err) } - p, err = semantic.AttachDrift(p, drift) + p, err := semantic.New(semantic.Input{ + Header: header, + HelmAction: helmAction, + Changes: changes, + Tasks: tasks, + Drift: drift, + ChartCRDs: chartCRDsFromDocs(input.CRDDocs, prep.Operation, input.SkipCRDs), + }) if err != nil { return semantic.Plan{}, nil, cleanup, fmt.Errorf("assemble semantic plan: %w", err) } diff --git a/internal/plan/semantic_build_test.go b/internal/plan/semantic_build_test.go index f283160..99ae3f8 100644 --- a/internal/plan/semantic_build_test.go +++ b/internal/plan/semantic_build_test.go @@ -354,46 +354,34 @@ func TestBuildSemanticPlan_NoResourceChange(t *testing.T) { func TestBuildSemanticPlan_FieldChangeIsUpdate(t *testing.T) { t.Parallel() - tests := []struct { - name string - revision int - }{ - {name: "records the field change", revision: 7}, - {name: "previous change is an update", revision: 4}, - {name: "failed-only history is still an upgrade", revision: 2}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - client := upgradeClient(configMapYAML("app", "prod", "old"), configMapYAML("app", "prod", "new"), tt.revision) - p := mustBuild(t, client, newMapper()) - assert.Equal(t, semantic.HelmUpgrade, p.HelmAction) - assert.Equal(t, tt.revision, p.Header.Revision) - assert.False(t, p.Header.FreshInstall) - require.Len(t, p.Changes, 1) - c := p.Changes[0] - assert.Equal(t, semantic.Update, c.Action) - assert.Equal(t, "v1", c.Resource.APIVersion) - require.NotNil(t, c.Before) - require.NotNil(t, c.After) - beforeKey, found, err := unstructured.NestedString(c.Before.Object, "data", "key") - require.NoError(t, err) - require.True(t, found) - assert.Equal(t, "old", beforeKey) - afterKey, found, err := unstructured.NestedString(c.After.Object, "data", "key") - require.NoError(t, err) - require.True(t, found) - assert.Equal(t, "new", afterKey) - require.Len(t, c.Fields, 1) - assert.Equal(t, "/data/key", c.Fields[0].Path) - assert.Equal(t, semantic.FieldReplace, c.Fields[0].Op) - assert.Equal(t, "old", c.Fields[0].Before) - assert.Equal(t, "new", c.Fields[0].After) + const revision = 7 + client := upgradeClient(configMapYAML("app", "prod", "old"), configMapYAML("app", "prod", "new"), revision) + p := mustBuild(t, client, newMapper()) + assert.Equal(t, semantic.HelmUpgrade, p.HelmAction) + assert.Equal(t, revision, p.Header.Revision) + assert.False(t, p.Header.FreshInstall) + require.Len(t, p.Changes, 1) + c := p.Changes[0] + assert.Equal(t, semantic.Update, c.Action) + assert.Equal(t, "v1", c.Resource.APIVersion) + require.NotNil(t, c.Before) + require.NotNil(t, c.After) + beforeKey, found, err := unstructured.NestedString(c.Before.Object, "data", "key") + require.NoError(t, err) + require.True(t, found) + assert.Equal(t, "old", beforeKey) + afterKey, found, err := unstructured.NestedString(c.After.Object, "data", "key") + require.NoError(t, err) + require.True(t, found) + assert.Equal(t, "new", afterKey) + require.Len(t, c.Fields, 1) + assert.Equal(t, "/data/key", c.Fields[0].Path) + assert.Equal(t, semantic.FieldReplace, c.Fields[0].Op) + assert.Equal(t, "old", c.Fields[0].Before) + assert.Equal(t, "new", c.Fields[0].After) - again := mustBuild(t, upgradeClient(configMapYAML("app", "prod", "old"), configMapYAML("app", "prod", "new"), tt.revision), newMapper()) - assert.Equal(t, p.Changes[0].Fields, again.Changes[0].Fields) - }) - } + again := mustBuild(t, upgradeClient(configMapYAML("app", "prod", "old"), configMapYAML("app", "prod", "new"), revision), newMapper()) + assert.Equal(t, p.Changes[0].Fields, again.Changes[0].Fields) } func TestBuildSemanticPlan_AddedAndRemoved(t *testing.T) { @@ -884,6 +872,7 @@ func TestBuildSemanticPlan_PrepRenderMismatch(t *testing.T) { name string prep helm.ReleasePrep result *render.RenderResult + prefix string want string }{ { @@ -924,6 +913,16 @@ func TestBuildSemanticPlan_PrepRenderMismatch(t *testing.T) { result: nil, want: "render result is required", }, + { + name: "upgrade without current release", + prep: helm.ReleasePrep{ + Operation: helm.OperationUpgrade, + NextRevision: 2, + }, + result: upgradeResult(configMapYAML("app", "prod", "new"), 2), + prefix: "determine helm release intent:", + want: "upgrade prep requires a current release", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -932,7 +931,11 @@ func TestBuildSemanticPlan_PrepRenderMismatch(t *testing.T) { p, result, cleanup, err := plan.BuildSemanticPlan(t.Context(), client, newMapper(), liveFor(client), buildInput("ctx", resolvedSpec(), nil)) t.Cleanup(cleanup) require.Error(t, err) - assert.ErrorContains(t, err, "render preparation:") + prefix := tt.prefix + if prefix == "" { + prefix = "render preparation:" + } + assert.ErrorContains(t, err, prefix) assert.ErrorContains(t, err, tt.want) assert.Zero(t, p) assert.Nil(t, result) @@ -940,25 +943,6 @@ func TestBuildSemanticPlan_PrepRenderMismatch(t *testing.T) { } } -func TestBuildSemanticPlan_InvalidPrep(t *testing.T) { - t.Parallel() - client := &fakeBuildClient{ - result: upgradeResult(configMapYAML("app", "prod", "new"), 2), - prep: helm.ReleasePrep{ - Operation: helm.OperationUpgrade, - NextRevision: 2, - }, - cleanup: func() {}, - } - p, result, cleanup, err := plan.BuildSemanticPlan(t.Context(), client, newMapper(), liveFor(client), buildInput("ctx", resolvedSpec(), nil)) - t.Cleanup(cleanup) - require.Error(t, err) - assert.ErrorContains(t, err, "determine helm release intent:") - assert.ErrorContains(t, err, "upgrade prep requires a current release") - assert.Zero(t, p) - assert.Nil(t, result) -} - func TestBuildSemanticPlan_MappingFailureLeavesCleanup(t *testing.T) { t.Parallel() var cleanups int diff --git a/internal/plan/tasks.go b/internal/plan/tasks.go index 018fa93..5d16725 100644 --- a/internal/plan/tasks.go +++ b/internal/plan/tasks.go @@ -47,9 +47,7 @@ func assembleTasks( desiredHooks []*v1.Hook, changes []semantic.ResourceChange, ) ([]semantic.TaskPlan, error) { - // WillRun stays false. Callers stamp it with stampTaskWillRun after - // deriveHelmAction. HelmAction is the source of truth for whether - // the plan has a Helm transition. + // WillRun stays false. semantic.New derives it from the Helm action. if resolved == nil { return []semantic.TaskPlan{}, nil } @@ -272,28 +270,6 @@ func hookTask( }, nil } -func stampTaskWillRun(tasks []semantic.TaskPlan, helmAction semantic.HelmAction) { - helmTransition := helmAction == semantic.HelmInstall || helmAction == semantic.HelmUpgrade - for i, t := range tasks { - if t.Phase == semantic.TaskSchedule { - tasks[i].WillRun = false - continue - } - if !hookPhase(t.Phase) { - continue - } - if t.Action == semantic.TaskDelete { - tasks[i].WillRun = false - continue - } - tasks[i].WillRun = helmTransition - } -} - -func hookPhase(p semantic.TaskPhase) bool { - return p == semantic.TaskPreDeploy || p == semantic.TaskPostDeploy -} - func diffHookDocs(desired, previous map[string]hookDoc) ([]semantic.HookDefinition, error) { if desired == nil { desired = map[string]hookDoc{} diff --git a/internal/plan/tasks_test.go b/internal/plan/tasks_test.go index d75c75c..902c71b 100644 --- a/internal/plan/tasks_test.go +++ b/internal/plan/tasks_test.go @@ -42,8 +42,7 @@ func TestAssembleTasks_FreshInstallPrePost(t *testing.T) { testHook("Job", "smoke", "smoke", "v1", 2), }, nil) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmInstall) - byName := taskByName(t, tasks) + byName := taskByName(t, tasksWithDerivedWillRun(t, semantic.HelmInstall, nil, tasks)) require.Contains(t, byName, "migrate") require.Contains(t, byName, "smoke") assert.NotContains(t, byName, "run") @@ -65,8 +64,7 @@ func TestAssembleTasks_ManifestChangeUnchangedHookWillRun(t *testing.T) { prep := upgradePrep(map[string]any{"seed": map[string]any{"on": "preDeploy", "hookWeight": 1}}, []*v1.Hook{hook}, nil) tasks, err := assembleTasks(resolved, prep, []*v1.Hook{hook}, []semantic.ResourceChange{labeledCreate("api", "api")}) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmUpgrade) - byName := taskByName(t, tasks) + byName := taskByName(t, tasksWithDerivedWillRun(t, semantic.HelmUpgrade, nil, tasks)) require.Contains(t, byName, "seed") assert.Equal(t, semantic.TaskUnchanged, byName["seed"].Action) assert.True(t, byName["seed"].WillRun) @@ -83,8 +81,7 @@ func TestAssembleTasks_HookDefinitionOnly(t *testing.T) { prep := upgradePrep(map[string]any{"migrate": map[string]any{"on": "preDeploy", "hookWeight": 1}}, []*v1.Hook{prev}, nil) tasks, err := assembleTasks(resolved, prep, []*v1.Hook{desired}, nil) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmUpgrade) - byName := taskByName(t, tasks) + byName := taskByName(t, tasksWithDerivedWillRun(t, semantic.HelmUpgrade, nil, tasks)) require.Contains(t, byName, "migrate") assert.Equal(t, semantic.TaskUpdate, byName["migrate"].Action) assert.True(t, byName["migrate"].WillRun) @@ -104,8 +101,7 @@ func TestAssembleTasks_HookCreateAndDelete(t *testing.T) { }, []*v1.Hook{prev}, nil) tasks, err := assembleTasks(resolved, prep, []*v1.Hook{desired}, nil) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmUpgrade) - byName := taskByName(t, tasks) + byName := taskByName(t, tasksWithDerivedWillRun(t, semantic.HelmUpgrade, nil, tasks)) require.Contains(t, byName, "smoke") require.Contains(t, byName, "old") assert.Equal(t, semantic.TaskCreate, byName["smoke"].Action) @@ -131,8 +127,7 @@ func TestAssembleTasks_AllCurrentHooksRunOnHelmChange(t *testing.T) { changes := []semantic.ResourceChange{labeledCreate("api", "api")} tasks, err := assembleTasks(resolved, prep, []*v1.Hook{migrate, seed}, changes) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmUpgrade) - byName := taskByName(t, tasks) + byName := taskByName(t, tasksWithDerivedWillRun(t, semantic.HelmUpgrade, nil, tasks)) assert.True(t, byName["migrate"].WillRun) assert.True(t, byName["seed"].WillRun) assert.Equal(t, semantic.TaskUnchanged, byName["migrate"].Action) @@ -151,7 +146,6 @@ func TestAssembleTasks_NewestHooksIgnored(t *testing.T) { prep := upgradePrep(map[string]any{"migrate": map[string]any{"on": "preDeploy", "hookWeight": 1}}, []*v1.Hook{currentHook}, newest) tasks, err := assembleTasks(resolved, prep, []*v1.Hook{desired}, nil) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmNone) byName := taskByName(t, tasks) assert.Equal(t, semantic.TaskUnchanged, byName["migrate"].Action) assert.Empty(t, byName["migrate"].Definitions) @@ -169,7 +163,6 @@ func TestAssembleTasks_MultiResourceBundleOnlyChangedDefinition(t *testing.T) { prep := upgradePrep(map[string]any{"migrate": map[string]any{"on": "preDeploy", "hookWeight": 1}}, []*v1.Hook{prevCM, prevJob}, nil) tasks, err := assembleTasks(resolved, prep, []*v1.Hook{desiredCM, desiredJob}, nil) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmUpgrade) byName := taskByName(t, tasks) require.Len(t, byName["migrate"].Definitions, 1) assert.Equal(t, "migrate-env", byName["migrate"].Definitions[0].Resource.Name) @@ -201,7 +194,6 @@ func TestAssembleTasks_LeftoverHookWithoutConfig(t *testing.T) { prep := upgradePrep(map[string]any{}, []*v1.Hook{hook}, nil) tasks, err := assembleTasks(resolvedWithTasks(nil), prep, nil, nil) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmUpgrade) byName := taskByName(t, tasks) require.Contains(t, byName, "orphan") assert.Equal(t, semantic.TaskDelete, byName["orphan"].Action) @@ -237,7 +229,6 @@ spec: prep := upgradePrep(map[string]any{}, []*v1.Hook{hook}, nil) tasks, err := assembleTasks(resolvedWithTasks(nil), prep, nil, nil) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmUpgrade) byName := taskByName(t, tasks) require.Contains(t, byName, "orphan") assert.Equal(t, semantic.TaskDelete, byName["orphan"].Action) @@ -388,8 +379,7 @@ func TestAssembleTasks_HookChangeRunsAllCurrentHooks(t *testing.T) { prep := upgradePrep(tt.previous, tt.prev, nil) tasks, err := assembleTasks(resolvedWithTasks(tt.resolved), prep, tt.desired, nil) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmUpgrade) - byName := taskByName(t, tasks) + byName := taskByName(t, tasksWithDerivedWillRun(t, semantic.HelmUpgrade, nil, tasks)) require.Len(t, byName, len(tt.want)) for name, want := range tt.want { got, ok := byName[name] @@ -413,7 +403,6 @@ func TestAssembleTasks_RemovedScheduleDoesNotCaptureComponent(t *testing.T) { changes := []semantic.ResourceChange{cron, comp} tasks, err := assembleTasks(resolved, prep, nil, changes) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmUpgrade) byName := taskByName(t, tasks) require.Contains(t, byName, "cleanup") assert.Equal(t, semantic.TaskDelete, byName["cleanup"].Action) @@ -477,7 +466,6 @@ func TestAssembleTasks_TransitionToManual(t *testing.T) { prep := upgradePrep(tt.previous, tt.prevHooks, nil) tasks, err := assembleTasks(resolved, prep, nil, tt.changes) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmUpgrade) byName := taskByName(t, tasks) require.Contains(t, byName, tt.task) got := byName[tt.task] @@ -528,8 +516,7 @@ func TestAssembleTasks_HookPhaseChange(t *testing.T) { }, []*v1.Hook{tt.prev}, nil) tasks, err := assembleTasks(resolved, prep, []*v1.Hook{tt.desired}, nil) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmUpgrade) - got := taskByName(t, tasks)["migrate"] + got := taskByName(t, tasksWithDerivedWillRun(t, semantic.HelmUpgrade, nil, tasks))["migrate"] assert.Equal(t, tt.wantPhase, got.Phase) assert.Equal(t, semantic.TaskUpdate, got.Action) assert.True(t, got.WillRun) @@ -648,8 +635,7 @@ func TestAssembleTasks_HookScheduleTransition(t *testing.T) { prep := upgradePrep(tt.previous, tt.prevHooks, nil) tasks, err := assembleTasks(resolved, prep, tt.desired, tt.changes) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmUpgrade) - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, tt.changes, tasks) + p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: tt.changes, Tasks: tasks}) require.NoError(t, err) byName := taskByName(t, p.Tasks) require.Contains(t, byName, "work") @@ -678,7 +664,6 @@ func TestAssembleTasks_ScheduleReferencesAndOmitsUnchanged(t *testing.T) { changes := []semantic.ResourceChange{cron, labeledCreate("api", "api")} tasks, err := assembleTasks(resolved, prep, nil, changes) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmUpgrade) byName := taskByName(t, tasks) require.Contains(t, byName, "cleanup") assert.NotContains(t, byName, "keep") @@ -734,7 +719,6 @@ func TestAssembleTasks_ScheduleProvenance(t *testing.T) { prep := upgradePrep(tt.previous, nil, nil) tasks, err := assembleTasks(resolvedWithTasks(tt.resolved), prep, nil, tt.changes) require.NoError(t, err) - stampTaskWillRun(tasks, semantic.HelmUpgrade) byName := taskByName(t, tasks) require.Contains(t, byName, "cleanup") got := byName["cleanup"] @@ -744,52 +728,6 @@ func TestAssembleTasks_ScheduleProvenance(t *testing.T) { } } -func TestStampHelmOrder_ConfigMapBeforeDeployment(t *testing.T) { - t.Parallel() - changes := []semantic.ResourceChange{ - { - Resource: semantic.ResourceRef{APIVersion: "apps/v1", Kind: "Deployment", Namespace: "prod", Name: "api"}, - Action: semantic.Create, - After: snapObj(map[string]any{"apiVersion": "apps/v1", "kind": "Deployment", "metadata": map[string]any{"name": "api"}}), - }, - { - Resource: semantic.ResourceRef{APIVersion: "v1", Kind: "ConfigMap", Namespace: "prod", Name: "api"}, - Action: semantic.Create, - After: snapObj(map[string]any{"apiVersion": "v1", "kind": "ConfigMap", "metadata": map[string]any{"name": "api"}}), - }, - } - require.NoError(t, stampHelmOrder(changes)) - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, changes, nil) - require.NoError(t, err) - require.Len(t, p.Changes, 2) - assert.Equal(t, "ConfigMap", p.Changes[0].Resource.Kind) - assert.Equal(t, "Deployment", p.Changes[1].Resource.Kind) -} - -func TestStampHelmOrder_SameKindPreservesInputOrder(t *testing.T) { - t.Parallel() - const n = 12 - changes := make([]semantic.ResourceChange, 0, n) - for i := range n { - name := fmt.Sprintf("n%d", n-1-i) - changes = append(changes, semantic.ResourceChange{ - Resource: semantic.ResourceRef{APIVersion: "v1", Kind: "ConfigMap", Namespace: "prod", Name: name}, - Action: semantic.Create, - After: snapObj(map[string]any{"apiVersion": "v1", "kind": "ConfigMap", "metadata": map[string]any{"name": name}}), - }) - } - require.NoError(t, stampHelmOrder(changes)) - for i, c := range changes { - assert.Equal(t, i+1, c.HelmOrder, "input index %d", i) - } - p, err := semantic.New(semantic.Header{}, semantic.HelmUpgrade, changes, nil) - require.NoError(t, err) - require.Len(t, p.Changes, n) - for i, c := range p.Changes { - assert.Equal(t, fmt.Sprintf("n%d", n-1-i), c.Resource.Name) - } -} - func resolvedWithTasks(tasks map[string]spec.ResolvedTask) *spec.ResolvedSpec { return &spec.ResolvedSpec{ Spec: &spec.Spec{Project: "shop"}, @@ -979,6 +917,22 @@ func snapObj(obj map[string]any) *semantic.ResourceSnapshot { return &semantic.ResourceSnapshot{Object: obj} } +func tasksWithDerivedWillRun(t *testing.T, action semantic.HelmAction, changes []semantic.ResourceChange, tasks []semantic.TaskPlan) []semantic.TaskPlan { + t.Helper() + header := semantic.Header{} + if action == semantic.HelmInstall { + header.FreshInstall = true + } + p, err := semantic.New(semantic.Input{ + Header: header, + HelmAction: action, + Changes: changes, + Tasks: tasks, + }) + require.NoError(t, err) + return p.Tasks +} + func taskByName(t *testing.T, tasks []semantic.TaskPlan) map[string]semantic.TaskPlan { t.Helper() out := make(map[string]semantic.TaskPlan, len(tasks)) diff --git a/internal/plan/view/helpers_test.go b/internal/plan/view/helpers_test.go index df95bb3..940f1f9 100644 --- a/internal/plan/view/helpers_test.go +++ b/internal/plan/view/helpers_test.go @@ -242,11 +242,47 @@ func mustPlanWithTasks(tb testing.TB, helmAction semantic.HelmAction, changes [] func mustPlanWithHeader(tb testing.TB, header semantic.Header, helmAction semantic.HelmAction, changes []semantic.ResourceChange, tasks []semantic.TaskPlan) semantic.Plan { tb.Helper() - p, err := semantic.New(header, helmAction, fillUpdateFields(changes), fillTaskFields(tasks)) + return mustPlanInput(tb, semantic.Input{ + Header: header, + HelmAction: helmAction, + Changes: changes, + Tasks: tasks, + }) +} + +func mustPlanInput(tb testing.TB, in semantic.Input) semantic.Plan { + tb.Helper() + in.Changes = fillUpdateFields(in.Changes) + in.Tasks = fillTaskFields(in.Tasks) + p, err := semantic.New(in) require.NoError(tb, err) return p } +func rebuildWithDrift(tb testing.TB, base semantic.Plan, drift []semantic.DriftChange) (semantic.Plan, error) { + tb.Helper() + return semantic.New(semantic.Input{ + Header: base.Header, + HelmAction: base.HelmAction, + Changes: base.Changes, + Tasks: base.Tasks, + Drift: drift, + ChartCRDs: base.ChartCRDs, + }) +} + +func rebuildWithChartCRDs(tb testing.TB, base semantic.Plan, crds []semantic.ChartCRD) (semantic.Plan, error) { + tb.Helper() + return semantic.New(semantic.Input{ + Header: base.Header, + HelmAction: base.HelmAction, + Changes: base.Changes, + Tasks: base.Tasks, + Drift: base.Drift, + ChartCRDs: crds, + }) +} + func fillUpdateFields(changes []semantic.ResourceChange) []semantic.ResourceChange { out := append([]semantic.ResourceChange(nil), changes...) for i, c := range out { diff --git a/internal/plan/view/human_chart_crd_test.go b/internal/plan/view/human_chart_crd_test.go index 3541532..c9cbc8c 100644 --- a/internal/plan/view/human_chart_crd_test.go +++ b/internal/plan/view/human_chart_crd_test.go @@ -160,11 +160,11 @@ func TestWriteHuman_ChartCRDPlacement(t *testing.T) { WillRun: true, }}) var err error - p, err = semantic.AttachChartCRDs(p, []semantic.ChartCRD{ + p, err = rebuildWithChartCRDs(t, p, []semantic.ChartCRD{ chartCRD(".deployah/crds/widget.yaml", "widgets.example.com", 0, semantic.ChartCRDUpgrade), }) require.NoError(t, err) - p, err = semantic.AttachDrift(p, []semantic.DriftChange{{ + p, err = rebuildWithDrift(t, p, []semantic.DriftChange{{ Resource: ref("ConfigMap", "extra"), Action: semantic.DriftUnexpected, Live: snap(cm("extra", "live")), @@ -216,7 +216,7 @@ func TestWriteHuman_ChartCRDWriteError(t *testing.T) { After: snap(cm("app", "v1")), }}, nil) var err error - p, err = semantic.AttachChartCRDs(p, []semantic.ChartCRD{ + p, err = rebuildWithChartCRDs(t, p, []semantic.ChartCRD{ chartCRD(".deployah/crds/widget.yaml", "widgets.example.com", 0, semantic.ChartCRDUpgrade), chartCRD(".deployah/crds/widget.yaml", "gadgets.example.com", 1, semantic.ChartCRDUpgrade), }) @@ -306,7 +306,7 @@ func chartCRD(source, name string, index int, lc semantic.ChartCRDLifecycle) sem func planWithChartCRDs(tb testing.TB, header semantic.Header, action semantic.HelmAction, crds []semantic.ChartCRD) semantic.Plan { tb.Helper() - p, err := semantic.AttachChartCRDs(mustPlanWithHeader(tb, header, action, nil, nil), crds) + p, err := rebuildWithChartCRDs(tb, mustPlanWithHeader(tb, header, action, nil, nil), crds) require.NoError(tb, err) return p } diff --git a/internal/plan/view/human_semantic_test.go b/internal/plan/view/human_semantic_test.go index 98c394d..8a3a556 100644 --- a/internal/plan/view/human_semantic_test.go +++ b/internal/plan/view/human_semantic_test.go @@ -65,9 +65,7 @@ func TestWriteHuman_SemanticPlan(t *testing.T) { assert.Equal(t, "web-prod", p.Header.Release) assert.Equal(t, productNamespace, p.Header.Namespace) assert.False(t, p.Header.FreshInstall) - assert.NotContains(t, text, "~ cleanup changed, will run") assertCronJobOnlyUnderTasks(t, text, "web-prod-cleanup") - assert.NotContains(t, text, `create v1/Namespace`) assert.NotContains(t, text, "-/+") assert.NotContains(t, strings.ToLower(text), "recreate") assert.NotContains(t, text, "Actions:") diff --git a/internal/plan/view/human_style_test.go b/internal/plan/view/human_style_test.go index eff211b..8584666 100644 --- a/internal/plan/view/human_style_test.go +++ b/internal/plan/view/human_style_test.go @@ -75,19 +75,19 @@ func TestWriteHuman_StylerPreservesLayout(t *testing.T) { func TestWriteHuman_StyleRoles(t *testing.T) { t.Parallel() header := humanHeader() - unexpected, err := semantic.AttachDrift(mustPlanWithHeader(t, header, semantic.HelmNone, nil, nil), []semantic.DriftChange{{ + unexpected, err := rebuildWithDrift(t, mustPlanWithHeader(t, header, semantic.HelmNone, nil, nil), []semantic.DriftChange{{ Resource: ref("ConfigMap", "extra"), Action: semantic.DriftUnexpected, Live: snap(cm("extra", "live")), }}) require.NoError(t, err) - missing, err := semantic.AttachDrift(mustPlanWithHeader(t, header, semantic.HelmNone, nil, nil), []semantic.DriftChange{{ + missing, err := rebuildWithDrift(t, mustPlanWithHeader(t, header, semantic.HelmNone, nil, nil), []semantic.DriftChange{{ Resource: ref("ConfigMap", "other"), Action: semantic.DriftMissing, Previous: snap(cm("other", "gone")), }}) require.NoError(t, err) - modified, err := semantic.AttachDrift(mustPlanWithHeader(t, header, semantic.HelmNone, nil, nil), []semantic.DriftChange{{ + modified, err := rebuildWithDrift(t, mustPlanWithHeader(t, header, semantic.HelmNone, nil, nil), []semantic.DriftChange{{ Resource: ref("ConfigMap", "app"), Action: semantic.DriftModified, Previous: snap(cm("app", "old")), diff --git a/internal/plan/view/human_test.go b/internal/plan/view/human_test.go index ab94741..ae3e78b 100644 --- a/internal/plan/view/human_test.go +++ b/internal/plan/view/human_test.go @@ -32,24 +32,7 @@ func TestWriteHuman_CreateUpdateDelete(t *testing.T) { t.Parallel() text := writeHuman(t, allActionsPlan(t)) assertGolden(t, "human_all_actions", text) - assert.NotContains(t, text, "~ cleanup changed, will run") - assert.NotContains(t, text, "jobTemplate:") - assert.Equal(t, 1, strings.Count(text, `batch/v1/CronJob "web-cleanup"`)) assert.NotContains(t, text, "~ ConfigMap/prod/rs") - assert.NotContains(t, text, "-/+") - assert.NotContains(t, strings.ToLower(text), "recreate") - assert.NotContains(t, text, "Actions:") - assert.NotContains(t, text, "create helm") - assert.NotContains(t, text, "completeness:") - assert.NotContains(t, text, "write=") - assert.NotContains(t, text, "field_manager=") - assert.NotContains(t, text, "force_conflicts=") - assert.NotContains(t, text, "delete=") - assert.NotContains(t, text, "before:") - assert.NotContains(t, text, "after:") - assert.NotContains(t, text, "--- before") - assert.NotContains(t, text, "+++ after") - assert.NotContains(t, text, "@@") } func TestWriteHuman_DeterministicMapOrder(t *testing.T) { @@ -891,7 +874,7 @@ func allActionsInputs() (semantic.Header, []semantic.ResourceChange, []semantic. func humanDriftPlan(t *testing.T) semantic.Plan { t.Helper() - p, err := semantic.AttachDrift(mustPlanWithHeader(t, humanHeader(), semantic.HelmNone, nil, nil), []semantic.DriftChange{ + p, err := rebuildWithDrift(t, mustPlanWithHeader(t, humanHeader(), semantic.HelmNone, nil, nil), []semantic.DriftChange{ { Resource: ref("ConfigMap", "app"), Action: semantic.DriftModified, @@ -936,7 +919,7 @@ func TestWriteHuman_EmptyDriftHasNoSection(t *testing.T) { func TestWriteHuman_DriftSecretNormalizedPath(t *testing.T) { t.Parallel() - p, err := semantic.AttachDrift(mustPlan(t, semantic.HelmNone, nil), []semantic.DriftChange{{ + p, err := rebuildWithDrift(t, mustPlan(t, semantic.HelmNone, nil), []semantic.DriftChange{{ Resource: ref("Secret", "db"), Action: semantic.DriftModified, Previous: snap(map[string]any{ diff --git a/internal/plan/view/json_test.go b/internal/plan/view/json_test.go index 9b1f8b0..fa7fc23 100644 --- a/internal/plan/view/json_test.go +++ b/internal/plan/view/json_test.go @@ -43,13 +43,6 @@ func TestWriteJSON_SchemaAndTasks(t *testing.T) { var buf bytes.Buffer require.NoError(t, view.WriteJSON(&buf, p, view.Options{})) assertJSONGolden(t, "json_empty", buf.String()) - var doc map[string]any - require.NoError(t, json.Unmarshal(buf.Bytes(), &doc)) - assert.NotContains(t, doc, "completeness") - summary, ok := doc["summary"].(map[string]any) - require.True(t, ok) - assert.NotContains(t, summary, "replace") - assertNoJSONKeysFromBytes(t, buf.Bytes()) validatePlanSchema(t, buf.Bytes()) } @@ -71,6 +64,24 @@ func TestWriteJSON_CreateChangeShape(t *testing.T) { validatePlanSchema(t, buf.Bytes()) } +func TestWriteJSON_DeleteChangeShape(t *testing.T) { + t.Parallel() + p := mustPlan(t, semantic.HelmUpgrade, []semantic.ResourceChange{{ + Resource: ref("ConfigMap", "app"), + Action: semantic.Delete, + Before: snap(cm("app", "v1")), + }}) + var buf bytes.Buffer + require.NoError(t, view.WriteJSON(&buf, p, view.Options{})) + change := jsonObjects(t, jsonObject(mustJSON(t, buf.Bytes()))["changes"])[0] + assert.Equal(t, []string{"action", "after", "before", "fields", "resource"}, sortedKeys(change)) + assert.Equal(t, "delete", change["action"]) + assert.NotNil(t, change["before"]) + assert.Nil(t, change["after"]) + assert.Empty(t, change["fields"]) + validatePlanSchema(t, buf.Bytes()) +} + func TestWriteJSON_TasksContract(t *testing.T) { t.Parallel() p := mustPlanWithTasks(t, semantic.HelmUpgrade, nil, []semantic.TaskPlan{{ @@ -88,7 +99,6 @@ func TestWriteJSON_TasksContract(t *testing.T) { require.NoError(t, view.WriteJSON(&buf, p, view.Options{})) assertJSONGolden(t, "json_tasks", buf.String()) validatePlanSchema(t, buf.Bytes()) - assertNoJSONKeysFromBytes(t, buf.Bytes()) } func TestWriteJSON_ChartCRDs(t *testing.T) { @@ -109,7 +119,7 @@ func TestWriteJSON_ChartCRDs(t *testing.T) { t.Parallel() p := mustPlanWithHeader(t, tc.header, tc.action, nil, nil) var err error - p, err = semantic.AttachChartCRDs(p, []semantic.ChartCRD{{ + p, err = rebuildWithChartCRDs(t, p, []semantic.ChartCRD{{ Source: ".deployah/crds/widgets.yaml", Kind: "CustomResourceDefinition", Name: "widgets.example.com", @@ -131,12 +141,8 @@ func TestWriteJSON_ChartCRDs(t *testing.T) { assert.Equal(t, tc.lifecycle.String(), entry["lifecycle"]) assert.Equal(t, tc.willProcess, entry["willProcess"]) assert.Equal(t, float64(0), entry["index"]) - for _, banned := range []string{"action", "origin", "before", "after", "apply", "fields", "namespace", "apiVersion"} { - assert.NotContains(t, entry, banned) - } assert.NotContains(t, buf.String(), "Helm install will process") validatePlanSchema(t, buf.Bytes()) - assertNoJSONKeysFromBytes(t, buf.Bytes()) }) } } @@ -154,7 +160,6 @@ func TestWriteJSON_Deterministic(t *testing.T) { require.NoError(t, view.WriteJSON(&b2, p, view.Options{})) assert.Equal(t, b1.String(), b2.String()) assertJSONGolden(t, "json_update", b1.String()) - assertNoJSONKeysFromBytes(t, b1.Bytes()) validatePlanSchema(t, b1.Bytes()) } @@ -197,7 +202,6 @@ func TestWriteJSON_CamelCasePropertyNames(t *testing.T) { change := jsonObjects(t, jsonObject(mustJSON(t, raw))["changes"])[0] assert.Equal(t, "create", change["action"]) assert.Empty(t, change["fields"]) - assertNoJSONKeysFromBytes(t, raw) validatePlanSchema(t, raw) } @@ -304,68 +308,6 @@ func TestWriteJSON_ExplicitNullFields(t *testing.T) { validatePlanSchema(t, buf.Bytes()) } -func TestWriteJSON_MatchesSchemaForRepresentativePlans(t *testing.T) { - t.Parallel() - res := ref("ConfigMap", "app") - tests := []struct { - name string - plan semantic.Plan - }{ - {name: "empty", plan: mustPlan(t, semantic.HelmNone, nil)}, - {name: "create", plan: mustPlan(t, semantic.HelmUpgrade, []semantic.ResourceChange{{ - Resource: res, - Action: semantic.Create, - After: snap(cm("app", "v1")), - }})}, - {name: "delete", plan: mustPlan(t, semantic.HelmUpgrade, []semantic.ResourceChange{{ - Resource: res, - Action: semantic.Delete, - Before: snap(cm("app", "v1")), - }})}, - {name: "update", plan: mustPlan(t, semantic.HelmUpgrade, []semantic.ResourceChange{{ - Resource: res, - Action: semantic.Update, - Before: snap(cm("app", "v1")), - After: snap(cm("app", "v2")), - }})}, - {name: "namespace create", plan: mustPlanWithHeader(t, semantic.Header{ - Project: "web", - Environment: "prod", - Release: "web", - Namespace: "prod", - FreshInstall: true, - }, semantic.HelmInstall, []semantic.ResourceChange{{ - Resource: semantic.ResourceRef{APIVersion: "v1", Kind: "Namespace", Name: "prod"}, - Action: semantic.Create, - After: snap(map[string]any{ - "apiVersion": "v1", - "kind": "Namespace", - "metadata": map[string]any{"name": "prod"}, - }), - }}, nil)}, - {name: "task create", plan: mustPlanWithTasks(t, semantic.HelmUpgrade, nil, []semantic.TaskPlan{{ - Name: "migrate", - Phase: semantic.TaskPreDeploy, - Action: semantic.TaskCreate, - WillRun: true, - Definitions: []semantic.HookDefinition{{ - Resource: ref("Job", "migrate"), - Action: semantic.Create, - After: snap(cm("migrate", "v1")), - }}, - }})}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - var buf bytes.Buffer - require.NoError(t, view.WriteJSON(&buf, tt.plan, view.Options{})) - validatePlanSchema(t, buf.Bytes()) - assertNoJSONKeysFromBytes(t, buf.Bytes()) - }) - } -} - func TestWriteJSON_SnapshotMayUseProtocolKeyNames(t *testing.T) { t.Parallel() obj := cm("app", "v1") @@ -386,33 +328,9 @@ func TestWriteJSON_SnapshotMayUseProtocolKeyNames(t *testing.T) { "api_version": "1", "will_run": "yes" }`, "changes", 0, "after", "data") - assertNoJSONKeysFromBytes(t, buf.Bytes()) validatePlanSchema(t, buf.Bytes()) } -func assertNoJSONKeysFromBytes(t *testing.T, raw []byte) { - t.Helper() - var doc map[string]any - require.NoError(t, json.Unmarshal(raw, &doc)) - assertNoJSONKeys(t, doc) - assert.Contains(t, doc, "helmAction") - if header := jsonObject(doc["header"]); header != nil { - assertNoJSONKeys(t, header) - } - if summary := jsonObject(doc["summary"]); summary != nil { - assertNoJSONKeys(t, summary) - } - for _, item := range jsonObjects(t, doc["changes"]) { - assertNoJSONChangeKeys(t, item) - } - for _, item := range jsonObjects(t, doc["tasks"]) { - assertNoJSONTaskKeys(t, item) - } - for _, item := range jsonObjects(t, doc["chartCRDs"]) { - assertNoJSONKeys(t, item) - } -} - func mustJSON(t *testing.T, raw []byte) map[string]any { t.Helper() var doc map[string]any @@ -453,13 +371,6 @@ func jsonObjects(t *testing.T, v any) []map[string]any { return out } -func assertNoJSONChangeKeys(t *testing.T, change map[string]any) { - t.Helper() - assertNoJSONKeys(t, change) - assertNoJSONResourceKeys(t, change["resource"]) - assertNoJSONFieldKeys(t, change["fields"]) -} - func TestWriteJSON_DriftShapes(t *testing.T) { t.Parallel() var buf bytes.Buffer @@ -468,8 +379,6 @@ func TestWriteJSON_DriftShapes(t *testing.T) { validatePlanSchema(t, raw) var doc map[string]any require.NoError(t, json.Unmarshal(raw, &doc)) - _, hasChecked := doc["driftChecked"] - assert.False(t, hasChecked) entries := jsonObjects(t, doc["drift"]) require.Len(t, entries, 3) @@ -490,7 +399,6 @@ func TestWriteJSON_DriftShapes(t *testing.T) { assert.NotNil(t, missing["previous"]) assert.Nil(t, missing["live"]) assert.Empty(t, missing["fields"]) - assertNoJSONKeysFromBytes(t, raw) } func TestWriteJSON_DriftSecretRedaction(t *testing.T) { @@ -542,7 +450,7 @@ func TestWriteJSON_DriftSecretRedaction(t *testing.T) { func secretDriftPlan(t *testing.T) semantic.Plan { t.Helper() - p, err := semantic.AttachDrift(mustPlan(t, semantic.HelmNone, nil), []semantic.DriftChange{ + p, err := rebuildWithDrift(t, mustPlan(t, semantic.HelmNone, nil), []semantic.DriftChange{ { Resource: ref("Secret", "db"), Action: semantic.DriftModified, @@ -580,33 +488,6 @@ func secretDriftPlan(t *testing.T) semantic.Plan { return p } -func assertNoJSONTaskKeys(t *testing.T, task map[string]any) { - t.Helper() - assertNoJSONKeys(t, task) - for _, item := range jsonObjects(t, task["resources"]) { - assertNoJSONKeys(t, item) - } - for _, def := range jsonObjects(t, task["definitions"]) { - assertNoJSONKeys(t, def) - assertNoJSONResourceKeys(t, def["resource"]) - assertNoJSONFieldKeys(t, def["fields"]) - } -} - -func assertNoJSONResourceKeys(t *testing.T, raw any) { - t.Helper() - res, isObject := raw.(map[string]any) - require.True(t, isObject) - assertNoJSONKeys(t, res) -} - -func assertNoJSONFieldKeys(t *testing.T, raw any) { - t.Helper() - for _, field := range jsonObjects(t, raw) { - assertNoJSONKeys(t, field) - } -} - func TestWriteRenderers_ManualSnapshotShapes(t *testing.T) { t.Parallel() p := semantic.Plan{ @@ -656,22 +537,3 @@ func TestWriteRenderers_NilTasks(t *testing.T) { require.NoError(t, view.WriteJSON(&jsonBuf, p, view.Options{})) assertJSONAt(t, jsonBuf.Bytes(), `[]`, "tasks") } - -func assertNoJSONKeys(t *testing.T, obj map[string]any) { - t.Helper() - require.NotNil(t, obj) - for _, name := range []string{ - "fresh_install", - "api_version", - "generate_name", - "field_manager", - "force_conflicts", - "will_run", - "marker", - "displayAction", - "executions", - "helm_action", - } { - assert.NotContains(t, obj, name) - } -} diff --git a/internal/plan/view/prepare.go b/internal/plan/view/prepare.go index 4fa0012..1669b87 100644 --- a/internal/plan/view/prepare.go +++ b/internal/plan/view/prepare.go @@ -62,16 +62,6 @@ func validateRenderable(p semantic.Plan) error { } } } - owned := make(map[string]string) - changeKeys := make(map[string]int, len(p.Changes)) - for i, c := range p.Changes { - key := refKey(c.Resource) - if _, exists := changeKeys[key]; exists { - changeKeys[key] = -1 - continue - } - changeKeys[key] = i - } for i, t := range p.Tasks { if err := requireEnum(fmt.Sprintf("task %d phase", i), t.Phase.String(), semantic.TaskPreDeploy.String(), semantic.TaskPostDeploy.String(), semantic.TaskSchedule.String()); err != nil { @@ -82,34 +72,6 @@ func validateRenderable(p semantic.Plan) error { semantic.TaskUpdate.String(), semantic.TaskDelete.String()); err != nil { return err } - if t.Phase == semantic.TaskSchedule { - if len(t.Definitions) > 0 { - return fmt.Errorf("task %s: schedule must not have hook definitions", t.Name) - } - if t.WillRun { - return fmt.Errorf("task %s: schedule must not will run", t.Name) - } - for _, ref := range t.Resources { - key := refKey(ref) - idx, ok := changeKeys[key] - if !ok { - return fmt.Errorf("task %s: dangling resource %s", t.Name, ref) - } - if idx < 0 { - return fmt.Errorf("task %s: resource %s matches more than one change", t.Name, ref) - } - if prev, taken := owned[key]; taken { - return fmt.Errorf("task %s: resource %s already referenced by task %s", t.Name, ref, prev) - } - owned[key] = t.Name - } - } - if (t.Phase == semantic.TaskPreDeploy || t.Phase == semantic.TaskPostDeploy) && len(t.Resources) > 0 { - return fmt.Errorf("task %s: %s must not reference resource changes", t.Name, t.Phase) - } - if t.Action == semantic.TaskDelete && t.WillRun { - return fmt.Errorf("task %s: delete must not will run", t.Name) - } for j, d := range t.Definitions { if err := requireEnum(fmt.Sprintf("task %d definition %d action", i, j), d.Action.String(), semantic.Create.String(), semantic.Update.String(), semantic.Delete.String()); err != nil { diff --git a/internal/plan/view/schema_test.go b/internal/plan/view/schema_test.go index 9db8b23..3870628 100644 --- a/internal/plan/view/schema_test.go +++ b/internal/plan/view/schema_test.go @@ -40,16 +40,9 @@ func TestSchemaV1ID_MatchesEmbeddedAndRendered(t *testing.T) { required, ok := sch["required"].([]any) require.True(t, ok) assert.Contains(t, required, "chartCRDs") - assert.NotContains(t, required, "completeness") - assert.NotContains(t, props, "completeness") defs, ok := sch["$defs"].(map[string]any) require.True(t, ok) - summary, ok := defs["Summary"].(map[string]any) - require.True(t, ok) - summaryProps, ok := summary["properties"].(map[string]any) - require.True(t, ok) - assert.NotContains(t, summaryProps, "replace") change, ok := defs["ResourceChange"].(map[string]any) require.True(t, ok) assert.Equal(t, []any{"resource", "action", "before", "after", "fields"}, change["required"]) @@ -138,14 +131,8 @@ func TestSchemaV1_RejectsMalformedDocuments(t *testing.T) { {name: "delete with fields", raw: patched(t, deleteDoc, func(d map[string]any) { firstChange(t, d)["fields"] = []any{addField} })}, - {name: "top-level completeness", raw: patched(t, update, func(d map[string]any) { - d["completeness"] = "complete" - })}, - {name: "action replace", raw: patched(t, update, func(d map[string]any) { - firstChange(t, d)["action"] = "replace" - })}, - {name: "unknown top-level executions", raw: patched(t, update, func(d map[string]any) { - d["executions"] = []any{} + {name: "action nope", raw: patched(t, update, func(d map[string]any) { + firstChange(t, d)["action"] = "nope" })}, {name: "unknown top-level field", raw: patched(t, update, func(d map[string]any) { d["unknown"] = true @@ -247,7 +234,7 @@ func TestSchemaV1_ChartCRDs(t *testing.T) { t.Parallel() p := mustPlanWithHeader(t, tc.header, tc.action, nil, nil) var attachErr error - p, attachErr = semantic.AttachChartCRDs(p, []semantic.ChartCRD{{ + p, attachErr = rebuildWithChartCRDs(t, p, []semantic.ChartCRD{{ Source: ".deployah/crds/widgets.yaml", Kind: "CustomResourceDefinition", Name: "widgets.example.com", @@ -263,7 +250,7 @@ func TestSchemaV1_ChartCRDs(t *testing.T) { p := mustPlan(t, semantic.HelmUpgrade, nil) var err error - p, err = semantic.AttachChartCRDs(p, []semantic.ChartCRD{{ + p, err = rebuildWithChartCRDs(t, p, []semantic.ChartCRD{{ Source: ".deployah/crds/widgets.yaml", Kind: "CustomResourceDefinition", Name: "widgets.example.com", @@ -300,13 +287,6 @@ func TestSchemaV1_ChartCRDs(t *testing.T) { } } -func TestSchemaV1_AcceptsHelmNoneWithDrift(t *testing.T) { - t.Parallel() - var buf bytes.Buffer - require.NoError(t, view.WriteJSON(&buf, sampleDriftPlan(t), view.Options{})) - validatePlanSchema(t, buf.Bytes()) -} - func TestSchemaV1_RejectsMalformedDrift(t *testing.T) { t.Parallel() base := mustPlanDoc(t, sampleDriftPlan(t)) @@ -358,7 +338,7 @@ func TestSchemaV1_RejectsMalformedDrift(t *testing.T) { func sampleDriftPlan(t *testing.T) semantic.Plan { t.Helper() - p, err := semantic.AttachDrift(mustPlan(t, semantic.HelmNone, nil), []semantic.DriftChange{ + p, err := rebuildWithDrift(t, mustPlan(t, semantic.HelmNone, nil), []semantic.DriftChange{ { Resource: ref("ConfigMap", "app"), Action: semantic.DriftModified, From ecc7dbcf4e5e7a25c641b1e1e9abaf962350c3d9 Mon Sep 17 00:00:00 2001 From: Mohammad Abdolirad Date: Wed, 7 Oct 2026 01:31:55 +0200 Subject: [PATCH 2/3] simplify plan constructor docs and tests - state package boundaries instead of the build steps - call semantic.New directly in drift and chart CRD tests - drop leftover historical view assertions --- internal/plan/doc.go | 23 ++--- internal/plan/semantic/chart_crd_test.go | 89 +++++++++-------- internal/plan/semantic/doc.go | 14 ++- internal/plan/semantic/drift.go | 6 +- internal/plan/semantic/drift_test.go | 82 +++++++-------- internal/plan/semantic/plan.go | 15 ++- internal/plan/tasks_test.go | 96 +++++++----------- internal/plan/view/helpers_test.go | 24 ----- internal/plan/view/human_chart_crd_test.go | 72 +++++++------- internal/plan/view/human_semantic_test.go | 6 -- internal/plan/view/human_style_test.go | 57 ++++++----- internal/plan/view/human_test.go | 110 +++++++++++---------- internal/plan/view/json_test.go | 95 +++++++++--------- internal/plan/view/schema_test.go | 98 ++++++++++-------- 14 files changed, 377 insertions(+), 410 deletions(-) diff --git a/internal/plan/doc.go b/internal/plan/doc.go index 2dff509..af82041 100644 --- a/internal/plan/doc.go +++ b/internal/plan/doc.go @@ -12,23 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package plan computes and renders read-only deployment plans. -// Planning looks at release changes. It does not decide whether -// `deployah deploy` runs Helm. +// Package plan builds a read-only deployment plan from a rendered +// release and, on upgrade, from live cluster objects. // -// [BuildSemanticPlan] renders with -// [SemanticBuildClient.RenderManifestsWithPrep]. It picks install, -// upgrade, or none from the Helm operation and from comparing the -// previous release with the render and its hooks. Resource changes -// are Previous to Desired. [RESTMapper] supplies scope only. An -// existing release reads Live through [LiveReader] with GET and LIST, -// so Drift is Previous to Live. Chart CRD lifecycle comes from the -// loaded documents, not from object diffs. The result is a -// [deployah.dev/deployah/internal/plan/semantic.Plan] from -// [deployah.dev/deployah/internal/plan/semantic.New]. That constructor -// derives whether each task will run. Task assembly in this package -// leaves WillRun false. Those types live in plan/semantic. Their -// rendering lives in plan/view. +// [BuildSemanticPlan] returns a +// [deployah.dev/deployah/internal/plan/semantic.Plan]. Task assembly +// records what changed. [deployah.dev/deployah/internal/plan/semantic.New] +// decides whether each task will run. Human text and JSON live in +// plan/view. // // [LastSuccessfulRelease] remains for deploy hostname and workload // guards. [BuildSemanticPlan] does not use it. diff --git a/internal/plan/semantic/chart_crd_test.go b/internal/plan/semantic/chart_crd_test.go index fd93e71..0016c59 100644 --- a/internal/plan/semantic/chart_crd_test.go +++ b/internal/plan/semantic/chart_crd_test.go @@ -23,11 +23,8 @@ import ( "deployah.dev/deployah/internal/plan/semantic" ) -func TestAttachChartCRDs_Lifecycle(t *testing.T) { +func TestNew_ChartCRDLifecycle(t *testing.T) { t.Parallel() - base, err := semantic.New(semantic.Input{Header: semantic.Header{FreshInstall: true}, HelmAction: semantic.HelmInstall, Changes: nil, Tasks: nil}) - require.NoError(t, err) - changesBefore := len(base.Changes) tests := []struct { name string lifecycle semantic.ChartCRDLifecycle @@ -40,32 +37,36 @@ func TestAttachChartCRDs_Lifecycle(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - p, attachErr := withChartCRDs(base, []semantic.ChartCRD{{ - Source: ".deployah/crds/widget.yaml", - Kind: "CustomResourceDefinition", - Name: "widgets.example.com", - Lifecycle: tc.lifecycle, - WillProcess: tc.willProcess, - }}) - require.NoError(t, attachErr) + p, err := semantic.New(semantic.Input{ + Header: semantic.Header{FreshInstall: true}, + HelmAction: semantic.HelmInstall, + ChartCRDs: []semantic.ChartCRD{{ + Source: ".deployah/crds/widget.yaml", + Kind: "CustomResourceDefinition", + Name: "widgets.example.com", + Lifecycle: tc.lifecycle, + WillProcess: tc.willProcess, + }}, + }) + require.NoError(t, err) require.Len(t, p.ChartCRDs, 1) assert.Equal(t, "CustomResourceDefinition", p.ChartCRDs[0].Kind) assert.Equal(t, "widgets.example.com", p.ChartCRDs[0].Name) assert.Equal(t, tc.lifecycle, p.ChartCRDs[0].Lifecycle) assert.Equal(t, tc.willProcess, p.ChartCRDs[0].WillProcess) - assert.Equal(t, changesBefore, len(p.Changes)) assert.Empty(t, p.Changes) }) } } -func TestAttachChartCRDs_PreservesOrder(t *testing.T) { +func TestNew_ChartCRDPreservesOrder(t *testing.T) { t.Parallel() - base, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: nil, Tasks: nil}) - require.NoError(t, err) - p, err := withChartCRDs(base, []semantic.ChartCRD{ - {Kind: "CustomResourceDefinition", Name: "one.example.com", Lifecycle: semantic.ChartCRDUpgrade}, - {Kind: "CustomResourceDefinition", Name: "two.example.com", Index: 1, Lifecycle: semantic.ChartCRDUpgrade}, + p, err := semantic.New(semantic.Input{ + HelmAction: semantic.HelmUpgrade, + ChartCRDs: []semantic.ChartCRD{ + {Kind: "CustomResourceDefinition", Name: "one.example.com", Lifecycle: semantic.ChartCRDUpgrade}, + {Kind: "CustomResourceDefinition", Name: "two.example.com", Index: 1, Lifecycle: semantic.ChartCRDUpgrade}, + }, }) require.NoError(t, err) require.Len(t, p.ChartCRDs, 2) @@ -75,17 +76,12 @@ func TestAttachChartCRDs_PreservesOrder(t *testing.T) { assert.Equal(t, 1, p.ChartCRDs[1].Index) } -func TestAttachChartCRDs_HasEffectsFollowsWillProcess(t *testing.T) { +func TestNew_ChartCRDHasEffectsFollowsWillProcess(t *testing.T) { t.Parallel() - fresh, err := semantic.New(semantic.Input{Header: semantic.Header{FreshInstall: true}, HelmAction: semantic.HelmInstall, Changes: nil, Tasks: nil}) - require.NoError(t, err) - idle, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmNone, Changes: nil, Tasks: nil}) - require.NoError(t, err) - require.True(t, idle.IsNoOp()) - tests := []struct { name string - base semantic.Plan + header semantic.Header + action semantic.HelmAction lifecycle semantic.ChartCRDLifecycle willProcess bool wantEffects bool @@ -93,19 +89,21 @@ func TestAttachChartCRDs_HasEffectsFollowsWillProcess(t *testing.T) { }{ { name: "process", - base: fresh, + header: semantic.Header{FreshInstall: true}, + action: semantic.HelmInstall, lifecycle: semantic.ChartCRDProcess, willProcess: true, wantEffects: true, }, { name: "skip", - base: fresh, + header: semantic.Header{FreshInstall: true}, + action: semantic.HelmInstall, lifecycle: semantic.ChartCRDSkip, }, { name: "upgrade", - base: idle, + action: semantic.HelmNone, lifecycle: semantic.ChartCRDUpgrade, wantNoOp: true, }, @@ -113,13 +111,17 @@ func TestAttachChartCRDs_HasEffectsFollowsWillProcess(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - p, attachErr := withChartCRDs(tc.base, []semantic.ChartCRD{{ - Kind: "CustomResourceDefinition", - Name: "widgets.example.com", - Lifecycle: tc.lifecycle, - WillProcess: tc.willProcess, - }}) - require.NoError(t, attachErr) + p, err := semantic.New(semantic.Input{ + Header: tc.header, + HelmAction: tc.action, + ChartCRDs: []semantic.ChartCRD{{ + Kind: "CustomResourceDefinition", + Name: "widgets.example.com", + Lifecycle: tc.lifecycle, + WillProcess: tc.willProcess, + }}, + }) + require.NoError(t, err) assert.Equal(t, tc.wantEffects, p.HasEffects()) assert.Equal(t, tc.wantNoOp, p.IsNoOp()) assert.Empty(t, p.Changes) @@ -128,10 +130,8 @@ func TestAttachChartCRDs_HasEffectsFollowsWillProcess(t *testing.T) { } } -func TestAttachChartCRDs_Validation(t *testing.T) { +func TestNew_ChartCRDValidation(t *testing.T) { t.Parallel() - base, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: nil, Tasks: nil}) - require.NoError(t, err) tests := []struct { name string crd semantic.ChartCRD @@ -147,9 +147,12 @@ func TestAttachChartCRDs_Validation(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - _, attachErr := withChartCRDs(base, []semantic.ChartCRD{tc.crd}) - require.Error(t, attachErr) - assert.ErrorContains(t, attachErr, tc.wantErr) + _, err := semantic.New(semantic.Input{ + HelmAction: semantic.HelmUpgrade, + ChartCRDs: []semantic.ChartCRD{tc.crd}, + }) + require.Error(t, err) + assert.ErrorContains(t, err, tc.wantErr) }) } } diff --git a/internal/plan/semantic/doc.go b/internal/plan/semantic/doc.go index 6f227d0..5d9ce0d 100644 --- a/internal/plan/semantic/doc.go +++ b/internal/plan/semantic/doc.go @@ -12,17 +12,15 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package semantic is Deployah's unredacted semantic plan model. +// Package semantic is the unredacted model of one deployment plan. // -// [New] is the only constructor. It derives [TaskPlan.WillRun] from -// the Helm action and the task phase and action. This package does -// not read the cluster and does not render charts. +// [New] is the only constructor. It ignores a caller's [TaskPlan.WillRun] +// and derives that flag from the Helm action and the task. This +// package does not render charts and does not read the cluster. // -// This package is not the deployah plan command and is not a JSON API. // Snapshots and field values stay complete. Serialization and secret // redaction belong to deployah.dev/deployah/internal/plan/view. // [deployah.dev/deployah/internal/plan/view.WriteJSON] is the only -// machine-readable output contract. -// -// Do not treat encoding/json of these types as a supported output path. +// machine-readable output. encoding/json of these types is not a +// supported output path. package semantic diff --git a/internal/plan/semantic/drift.go b/internal/plan/semantic/drift.go index 668d755..2c4866a 100644 --- a/internal/plan/semantic/drift.go +++ b/internal/plan/semantic/drift.go @@ -100,9 +100,9 @@ func (p Plan) HasDrift() bool { return len(p.Drift) > 0 } -// validateDriftSet checks drift before [New] sorts it. A fresh install -// accepts only an empty collection. Entries with the same group, kind, -// namespace, and name are an error. Group comes from apiVersion. +// validateDriftSet checks drift. A fresh install accepts only an empty +// collection. The same group, kind, namespace, and name twice is an +// error. Group comes from apiVersion. func validateDriftSet(header Header, drift []DriftChange) error { if header.FreshInstall && len(drift) > 0 { return fmt.Errorf("fresh install must not include drift") diff --git a/internal/plan/semantic/drift_test.go b/internal/plan/semantic/drift_test.go index 786743c..660bce9 100644 --- a/internal/plan/semantic/drift_test.go +++ b/internal/plan/semantic/drift_test.go @@ -32,9 +32,8 @@ func TestNew_DriftStartsEmpty(t *testing.T) { assert.False(t, p.HasDrift()) } -func TestAttachDrift_FreshInstallRejectsDrift(t *testing.T) { +func TestNew_DriftRejectsFreshInstall(t *testing.T) { t.Parallel() - base := mustPlan(t, semantic.Header{FreshInstall: true}, semantic.HelmInstall) tests := []struct { name string drift []semantic.DriftChange @@ -48,7 +47,11 @@ func TestAttachDrift_FreshInstallRejectsDrift(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got, err := withDrift(base, tt.drift) + got, err := semantic.New(semantic.Input{ + Header: semantic.Header{FreshInstall: true}, + HelmAction: semantic.HelmInstall, + Drift: tt.drift, + }) if tt.wantErr != "" { require.Error(t, err) assert.ErrorContains(t, err, tt.wantErr) @@ -62,9 +65,8 @@ func TestAttachDrift_FreshInstallRejectsDrift(t *testing.T) { } } -func TestAttachDrift_Validation(t *testing.T) { +func TestNew_DriftValidation(t *testing.T) { t.Parallel() - base := mustPlan(t, semantic.Header{}, semantic.HelmUpgrade) live := &semantic.ResourceSnapshot{Object: cm("app", "live")} prev := &semantic.ResourceSnapshot{Object: cm("app", "prev")} fields := []semantic.FieldChange{{Path: "/data/key", Op: semantic.FieldReplace, Before: "prev", After: "live"}} @@ -145,30 +147,38 @@ func TestAttachDrift_Validation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - _, err := withDrift(base, []semantic.DriftChange{tt.drift}) + _, err := semantic.New(semantic.Input{ + HelmAction: semantic.HelmUpgrade, + Drift: []semantic.DriftChange{tt.drift}, + }) require.Error(t, err) assert.ErrorContains(t, err, tt.wantErr) }) } } -func TestAttachDrift_DuplicateLogicalIdentity(t *testing.T) { +func TestNew_DriftDuplicateLogicalIdentity(t *testing.T) { t.Parallel() - base := mustPlan(t, semantic.Header{}, semantic.HelmUpgrade) first := missingDrift("app") second := missingDrift("app") second.Resource.APIVersion = "core/v1" - _, err := withDrift(base, []semantic.DriftChange{first, second}) + _, err := semantic.New(semantic.Input{ + HelmAction: semantic.HelmUpgrade, + Drift: []semantic.DriftChange{first, second}, + }) require.Error(t, err) assert.ErrorContains(t, err, "duplicate logical identity") } -func TestAttachDrift_SortsAndCopies(t *testing.T) { +func TestNew_DriftSortsAndCopies(t *testing.T) { t.Parallel() change := updateChange("app", "v1", "v2") - base, err := semantic.New(semantic.Input{Header: semantic.Header{Release: "web"}, HelmAction: semantic.HelmUpgrade, Changes: []semantic.ResourceChange{change}, Tasks: nil}) + without, err := semantic.New(semantic.Input{ + Header: semantic.Header{Release: "web"}, + HelmAction: semantic.HelmUpgrade, + Changes: []semantic.ResourceChange{change}, + }) require.NoError(t, err) - summary := base.Summary zeta := missingDrift("zeta") alpha := unexpectedDrift("alpha") @@ -180,7 +190,12 @@ func TestAttachDrift_SortsAndCopies(t *testing.T) { {Path: "/a", Op: semantic.FieldReplace, Before: "1", After: "2"}, } input := []semantic.DriftChange{zeta, alpha, mod} - got, err := withDrift(base, input) + got, err := semantic.New(semantic.Input{ + Header: semantic.Header{Release: "web"}, + HelmAction: semantic.HelmUpgrade, + Changes: []semantic.ResourceChange{change}, + Drift: input, + }) require.NoError(t, err) require.Len(t, got.Drift, 3) // Group sorts before kind, so core ConfigMaps precede apps/Deployment. @@ -199,7 +214,7 @@ func TestAttachDrift_SortsAndCopies(t *testing.T) { assert.Equal(t, "prev", inputData["key"]) assert.Equal(t, "1", input[2].Fields[0].Before) - assert.Equal(t, summary, got.Summary) + assert.Equal(t, without.Summary, got.Summary) assert.Equal(t, semantic.HelmUpgrade, got.HelmAction) require.Len(t, got.Changes, 1) assert.Equal(t, semantic.Update, got.Changes[0].Action) @@ -208,11 +223,13 @@ func TestAttachDrift_SortsAndCopies(t *testing.T) { assert.True(t, got.HasDrift()) } -func TestAttachDrift_DoesNotChangeReleaseIntent(t *testing.T) { +func TestNew_DriftLeavesReleaseIntent(t *testing.T) { t.Parallel() - base, err := semantic.New(semantic.Input{Header: semantic.Header{Release: "web"}, HelmAction: semantic.HelmNone, Changes: nil, Tasks: nil}) - require.NoError(t, err) - got, err := withDrift(base, []semantic.DriftChange{missingDrift("app")}) + got, err := semantic.New(semantic.Input{ + Header: semantic.Header{Release: "web"}, + HelmAction: semantic.HelmNone, + Drift: []semantic.DriftChange{missingDrift("app")}, + }) require.NoError(t, err) assert.Equal(t, semantic.HelmNone, got.HelmAction) assert.Empty(t, got.Changes) @@ -241,35 +258,6 @@ func TestDriftAction_String(t *testing.T) { } } -func mustPlan(t *testing.T, header semantic.Header, action semantic.HelmAction) semantic.Plan { - t.Helper() - p, err := semantic.New(semantic.Input{Header: header, HelmAction: action, Changes: nil, Tasks: nil}) - require.NoError(t, err) - return p -} - -func withDrift(base semantic.Plan, drift []semantic.DriftChange) (semantic.Plan, error) { - return semantic.New(semantic.Input{ - Header: base.Header, - HelmAction: base.HelmAction, - Changes: base.Changes, - Tasks: base.Tasks, - Drift: drift, - ChartCRDs: base.ChartCRDs, - }) -} - -func withChartCRDs(base semantic.Plan, crds []semantic.ChartCRD) (semantic.Plan, error) { - return semantic.New(semantic.Input{ - Header: base.Header, - HelmAction: base.HelmAction, - Changes: base.Changes, - Tasks: base.Tasks, - Drift: base.Drift, - ChartCRDs: crds, - }) -} - func missingDrift(name string) semantic.DriftChange { return semantic.DriftChange{ Resource: ref("ConfigMap", name), diff --git a/internal/plan/semantic/plan.go b/internal/plan/semantic/plan.go index 5147eb4..9b71e10 100644 --- a/internal/plan/semantic/plan.go +++ b/internal/plan/semantic/plan.go @@ -33,8 +33,8 @@ type Plan struct { Summary Summary } -// Input is the caller data for [New]. Task WillRun values in Tasks -// are ignored. [New] derives them. +// Input is the caller data for [New]. [TaskPlan.WillRun] on Tasks is +// ignored. type Input struct { Header Header HelmAction HelmAction @@ -44,13 +44,12 @@ type Input struct { ChartCRDs []ChartCRD } -// New copies in and returns a complete plan, or a zero Plan and an -// error. Nil slices become empty slices. +// New copies in and returns a complete [Plan], or a zero Plan and an +// error. Nil slices become empty slices. Incoming [TaskPlan.WillRun] +// values are ignored. // -// Order is: copy and normalize, enum and shape checks, derived -// WillRun, Helm and content checks, drift and chart CRD checks, sort, -// then [Summary]. Summary counts resource changes only. Chart CRDs -// stay in the order given. Changes, tasks, and drift are sorted. +// [Summary] counts resource changes only. Chart CRDs stay in the +// given order. Changes, tasks, and drift are sorted. func New(in Input) (Plan, error) { changes := normalizeChanges(in.Changes) tasks, normErr := normalizeTasks(in.Tasks) diff --git a/internal/plan/tasks_test.go b/internal/plan/tasks_test.go index 902c71b..25d0aba 100644 --- a/internal/plan/tasks_test.go +++ b/internal/plan/tasks_test.go @@ -42,20 +42,20 @@ func TestAssembleTasks_FreshInstallPrePost(t *testing.T) { testHook("Job", "smoke", "smoke", "v1", 2), }, nil) require.NoError(t, err) - byName := taskByName(t, tasksWithDerivedWillRun(t, semantic.HelmInstall, nil, tasks)) + byName := taskByName(t, tasks) require.Contains(t, byName, "migrate") require.Contains(t, byName, "smoke") assert.NotContains(t, byName, "run") assert.Equal(t, semantic.TaskCreate, byName["migrate"].Action) - assert.True(t, byName["migrate"].WillRun) + assert.False(t, byName["migrate"].WillRun) assert.Equal(t, semantic.TaskPreDeploy, byName["migrate"].Phase) assert.Len(t, byName["migrate"].Definitions, 2) assert.Equal(t, semantic.TaskCreate, byName["smoke"].Action) - assert.True(t, byName["smoke"].WillRun) + assert.False(t, byName["smoke"].WillRun) assert.Empty(t, byName["migrate"].Resources) } -func TestAssembleTasks_ManifestChangeUnchangedHookWillRun(t *testing.T) { +func TestAssembleTasks_ManifestChangeLeavesHookUnchanged(t *testing.T) { t.Parallel() resolved := resolvedWithTasks(map[string]spec.ResolvedTask{ "seed": hookResolved(spec.TaskOnPreDeploy, 1), @@ -64,10 +64,10 @@ func TestAssembleTasks_ManifestChangeUnchangedHookWillRun(t *testing.T) { prep := upgradePrep(map[string]any{"seed": map[string]any{"on": "preDeploy", "hookWeight": 1}}, []*v1.Hook{hook}, nil) tasks, err := assembleTasks(resolved, prep, []*v1.Hook{hook}, []semantic.ResourceChange{labeledCreate("api", "api")}) require.NoError(t, err) - byName := taskByName(t, tasksWithDerivedWillRun(t, semantic.HelmUpgrade, nil, tasks)) + byName := taskByName(t, tasks) require.Contains(t, byName, "seed") assert.Equal(t, semantic.TaskUnchanged, byName["seed"].Action) - assert.True(t, byName["seed"].WillRun) + assert.False(t, byName["seed"].WillRun) assert.Empty(t, byName["seed"].Definitions) } @@ -81,10 +81,10 @@ func TestAssembleTasks_HookDefinitionOnly(t *testing.T) { prep := upgradePrep(map[string]any{"migrate": map[string]any{"on": "preDeploy", "hookWeight": 1}}, []*v1.Hook{prev}, nil) tasks, err := assembleTasks(resolved, prep, []*v1.Hook{desired}, nil) require.NoError(t, err) - byName := taskByName(t, tasksWithDerivedWillRun(t, semantic.HelmUpgrade, nil, tasks)) + byName := taskByName(t, tasks) require.Contains(t, byName, "migrate") assert.Equal(t, semantic.TaskUpdate, byName["migrate"].Action) - assert.True(t, byName["migrate"].WillRun) + assert.False(t, byName["migrate"].WillRun) require.Len(t, byName["migrate"].Definitions, 1) assert.Equal(t, semantic.Update, byName["migrate"].Definitions[0].Action) } @@ -101,18 +101,18 @@ func TestAssembleTasks_HookCreateAndDelete(t *testing.T) { }, []*v1.Hook{prev}, nil) tasks, err := assembleTasks(resolved, prep, []*v1.Hook{desired}, nil) require.NoError(t, err) - byName := taskByName(t, tasksWithDerivedWillRun(t, semantic.HelmUpgrade, nil, tasks)) + byName := taskByName(t, tasks) require.Contains(t, byName, "smoke") require.Contains(t, byName, "old") assert.Equal(t, semantic.TaskCreate, byName["smoke"].Action) - assert.True(t, byName["smoke"].WillRun) + assert.False(t, byName["smoke"].WillRun) assert.Equal(t, semantic.TaskDelete, byName["old"].Action) assert.False(t, byName["old"].WillRun) require.Len(t, byName["old"].Definitions, 1) assert.Equal(t, semantic.Delete, byName["old"].Definitions[0].Action) } -func TestAssembleTasks_AllCurrentHooksRunOnHelmChange(t *testing.T) { +func TestAssembleTasks_ResourceChangeKeepsHooks(t *testing.T) { t.Parallel() resolved := resolvedWithTasks(map[string]spec.ResolvedTask{ "migrate": hookResolved(spec.TaskOnPreDeploy, 1), @@ -127,9 +127,9 @@ func TestAssembleTasks_AllCurrentHooksRunOnHelmChange(t *testing.T) { changes := []semantic.ResourceChange{labeledCreate("api", "api")} tasks, err := assembleTasks(resolved, prep, []*v1.Hook{migrate, seed}, changes) require.NoError(t, err) - byName := taskByName(t, tasksWithDerivedWillRun(t, semantic.HelmUpgrade, nil, tasks)) - assert.True(t, byName["migrate"].WillRun) - assert.True(t, byName["seed"].WillRun) + byName := taskByName(t, tasks) + assert.False(t, byName["migrate"].WillRun) + assert.False(t, byName["seed"].WillRun) assert.Equal(t, semantic.TaskUnchanged, byName["migrate"].Action) assert.Equal(t, semantic.TaskUnchanged, byName["seed"].Action) } @@ -303,12 +303,11 @@ func TestAssembleTasks_FailClosed(t *testing.T) { } } -func TestAssembleTasks_HookChangeRunsAllCurrentHooks(t *testing.T) { +func TestAssembleTasks_HookChangeKeepsSiblings(t *testing.T) { t.Parallel() type want struct { - Action semantic.TaskAction - Phase semantic.TaskPhase - WillRun bool + Action semantic.TaskAction + Phase semantic.TaskPhase } seed := testHook("Job", "seed", "seed", "same", 1) migrateV1 := testHook("Job", "migrate", "migrate", "v1", 1) @@ -324,7 +323,7 @@ func TestAssembleTasks_HookChangeRunsAllCurrentHooks(t *testing.T) { want map[string]want }{ { - name: "definition change runs unchanged sibling", + name: "definition change keeps unchanged sibling", resolved: map[string]spec.ResolvedTask{ "migrate": hookResolved(spec.TaskOnPreDeploy, 1), "seed": hookResolved(spec.TaskOnPreDeploy, 2), @@ -336,12 +335,12 @@ func TestAssembleTasks_HookChangeRunsAllCurrentHooks(t *testing.T) { prev: []*v1.Hook{migrateV1, seed}, desired: []*v1.Hook{migrateV2, seed}, want: map[string]want{ - "migrate": {Action: semantic.TaskUpdate, Phase: semantic.TaskPreDeploy, WillRun: true}, - "seed": {Action: semantic.TaskUnchanged, Phase: semantic.TaskPreDeploy, WillRun: true}, + "migrate": {Action: semantic.TaskUpdate, Phase: semantic.TaskPreDeploy}, + "seed": {Action: semantic.TaskUnchanged, Phase: semantic.TaskPreDeploy}, }, }, { - name: "new hook runs unchanged sibling", + name: "new hook keeps unchanged sibling", resolved: map[string]spec.ResolvedTask{ "seed": hookResolved(spec.TaskOnPreDeploy, 1), "smoke": hookResolved(spec.TaskOnPostDeploy, 2), @@ -352,12 +351,12 @@ func TestAssembleTasks_HookChangeRunsAllCurrentHooks(t *testing.T) { prev: []*v1.Hook{seed}, desired: []*v1.Hook{seed, smoke}, want: map[string]want{ - "seed": {Action: semantic.TaskUnchanged, Phase: semantic.TaskPreDeploy, WillRun: true}, - "smoke": {Action: semantic.TaskCreate, Phase: semantic.TaskPostDeploy, WillRun: true}, + "seed": {Action: semantic.TaskUnchanged, Phase: semantic.TaskPreDeploy}, + "smoke": {Action: semantic.TaskCreate, Phase: semantic.TaskPostDeploy}, }, }, { - name: "removed hook runs remaining sibling", + name: "removed hook keeps remaining sibling", resolved: map[string]spec.ResolvedTask{ "seed": hookResolved(spec.TaskOnPreDeploy, 1), }, @@ -368,8 +367,8 @@ func TestAssembleTasks_HookChangeRunsAllCurrentHooks(t *testing.T) { prev: []*v1.Hook{old, seed}, desired: []*v1.Hook{seed}, want: map[string]want{ - "old": {Action: semantic.TaskDelete, Phase: semantic.TaskPreDeploy, WillRun: false}, - "seed": {Action: semantic.TaskUnchanged, Phase: semantic.TaskPreDeploy, WillRun: true}, + "old": {Action: semantic.TaskDelete, Phase: semantic.TaskPreDeploy}, + "seed": {Action: semantic.TaskUnchanged, Phase: semantic.TaskPreDeploy}, }, }, } @@ -379,14 +378,14 @@ func TestAssembleTasks_HookChangeRunsAllCurrentHooks(t *testing.T) { prep := upgradePrep(tt.previous, tt.prev, nil) tasks, err := assembleTasks(resolvedWithTasks(tt.resolved), prep, tt.desired, nil) require.NoError(t, err) - byName := taskByName(t, tasksWithDerivedWillRun(t, semantic.HelmUpgrade, nil, tasks)) + byName := taskByName(t, tasks) require.Len(t, byName, len(tt.want)) for name, want := range tt.want { got, ok := byName[name] require.True(t, ok, "missing task %s", name) assert.Equal(t, want.Action, got.Action, name) assert.Equal(t, want.Phase, got.Phase, name) - assert.Equal(t, want.WillRun, got.WillRun, name) + assert.False(t, got.WillRun, name) } }) } @@ -516,10 +515,10 @@ func TestAssembleTasks_HookPhaseChange(t *testing.T) { }, []*v1.Hook{tt.prev}, nil) tasks, err := assembleTasks(resolved, prep, []*v1.Hook{tt.desired}, nil) require.NoError(t, err) - got := taskByName(t, tasksWithDerivedWillRun(t, semantic.HelmUpgrade, nil, tasks))["migrate"] + got := taskByName(t, tasks)["migrate"] assert.Equal(t, tt.wantPhase, got.Phase) assert.Equal(t, semantic.TaskUpdate, got.Action) - assert.True(t, got.WillRun) + assert.False(t, got.WillRun) }) } } @@ -542,7 +541,6 @@ func TestAssembleTasks_HookScheduleTransition(t *testing.T) { changes []semantic.ResourceChange wantPhase semantic.TaskPhase wantAction semantic.TaskAction - wantWillRun bool wantDefActions []semantic.Action wantOwned []string wantChangeNames []string @@ -555,7 +553,6 @@ func TestAssembleTasks_HookScheduleTransition(t *testing.T) { changes: []semantic.ResourceChange{cronDelete}, wantPhase: semantic.TaskPreDeploy, wantAction: semantic.TaskUpdate, - wantWillRun: true, wantDefActions: []semantic.Action{semantic.Create}, wantOwned: []string{}, wantChangeNames: []string{"work"}, @@ -568,7 +565,6 @@ func TestAssembleTasks_HookScheduleTransition(t *testing.T) { changes: []semantic.ResourceChange{cronDelete}, wantPhase: semantic.TaskPostDeploy, wantAction: semantic.TaskUpdate, - wantWillRun: true, wantDefActions: []semantic.Action{semantic.Create}, wantOwned: []string{}, wantChangeNames: []string{"work"}, @@ -581,7 +577,6 @@ func TestAssembleTasks_HookScheduleTransition(t *testing.T) { changes: []semantic.ResourceChange{cronCreate}, wantPhase: semantic.TaskSchedule, wantAction: semantic.TaskUpdate, - wantWillRun: false, wantDefActions: []semantic.Action{}, wantOwned: []string{"work"}, wantChangeNames: []string{"work"}, @@ -594,7 +589,6 @@ func TestAssembleTasks_HookScheduleTransition(t *testing.T) { changes: []semantic.ResourceChange{cronCreate}, wantPhase: semantic.TaskSchedule, wantAction: semantic.TaskUpdate, - wantWillRun: false, wantDefActions: []semantic.Action{}, wantOwned: []string{"work"}, wantChangeNames: []string{"work"}, @@ -607,7 +601,6 @@ func TestAssembleTasks_HookScheduleTransition(t *testing.T) { changes: []semantic.ResourceChange{cronCreate}, wantPhase: semantic.TaskSchedule, wantAction: semantic.TaskCreate, - wantWillRun: false, wantDefActions: []semantic.Action{}, wantOwned: []string{"work"}, wantChangeNames: []string{"work"}, @@ -620,7 +613,6 @@ func TestAssembleTasks_HookScheduleTransition(t *testing.T) { changes: []semantic.ResourceChange{cronCreate}, wantPhase: semantic.TaskSchedule, wantAction: semantic.TaskCreate, - wantWillRun: false, wantDefActions: []semantic.Action{}, wantOwned: []string{"work"}, wantChangeNames: []string{"work"}, @@ -635,16 +627,20 @@ func TestAssembleTasks_HookScheduleTransition(t *testing.T) { prep := upgradePrep(tt.previous, tt.prevHooks, nil) tasks, err := assembleTasks(resolved, prep, tt.desired, tt.changes) require.NoError(t, err) - p, err := semantic.New(semantic.Input{Header: semantic.Header{}, HelmAction: semantic.HelmUpgrade, Changes: tt.changes, Tasks: tasks}) - require.NoError(t, err) - byName := taskByName(t, p.Tasks) + byName := taskByName(t, tasks) require.Contains(t, byName, "work") got := byName["work"] assert.Equal(t, tt.wantPhase, got.Phase) assert.Equal(t, tt.wantAction, got.Action) - assert.Equal(t, tt.wantWillRun, got.WillRun) + assert.False(t, got.WillRun) assert.Equal(t, tt.wantDefActions, definitionActions(got.Definitions)) assert.Equal(t, tt.wantOwned, resourceNames(got.Resources)) + p, err := semantic.New(semantic.Input{ + HelmAction: semantic.HelmUpgrade, + Changes: tt.changes, + Tasks: tasks, + }) + require.NoError(t, err) assert.Equal(t, tt.wantChangeNames, resourceNames(changeResources(p.Changes))) }) } @@ -917,22 +913,6 @@ func snapObj(obj map[string]any) *semantic.ResourceSnapshot { return &semantic.ResourceSnapshot{Object: obj} } -func tasksWithDerivedWillRun(t *testing.T, action semantic.HelmAction, changes []semantic.ResourceChange, tasks []semantic.TaskPlan) []semantic.TaskPlan { - t.Helper() - header := semantic.Header{} - if action == semantic.HelmInstall { - header.FreshInstall = true - } - p, err := semantic.New(semantic.Input{ - Header: header, - HelmAction: action, - Changes: changes, - Tasks: tasks, - }) - require.NoError(t, err) - return p.Tasks -} - func taskByName(t *testing.T, tasks []semantic.TaskPlan) map[string]semantic.TaskPlan { t.Helper() out := make(map[string]semantic.TaskPlan, len(tasks)) diff --git a/internal/plan/view/helpers_test.go b/internal/plan/view/helpers_test.go index 940f1f9..a1b1556 100644 --- a/internal/plan/view/helpers_test.go +++ b/internal/plan/view/helpers_test.go @@ -259,30 +259,6 @@ func mustPlanInput(tb testing.TB, in semantic.Input) semantic.Plan { return p } -func rebuildWithDrift(tb testing.TB, base semantic.Plan, drift []semantic.DriftChange) (semantic.Plan, error) { - tb.Helper() - return semantic.New(semantic.Input{ - Header: base.Header, - HelmAction: base.HelmAction, - Changes: base.Changes, - Tasks: base.Tasks, - Drift: drift, - ChartCRDs: base.ChartCRDs, - }) -} - -func rebuildWithChartCRDs(tb testing.TB, base semantic.Plan, crds []semantic.ChartCRD) (semantic.Plan, error) { - tb.Helper() - return semantic.New(semantic.Input{ - Header: base.Header, - HelmAction: base.HelmAction, - Changes: base.Changes, - Tasks: base.Tasks, - Drift: base.Drift, - ChartCRDs: crds, - }) -} - func fillUpdateFields(changes []semantic.ResourceChange) []semantic.ResourceChange { out := append([]semantic.ResourceChange(nil), changes...) for i, c := range out { diff --git a/internal/plan/view/human_chart_crd_test.go b/internal/plan/view/human_chart_crd_test.go index c9cbc8c..0efc743 100644 --- a/internal/plan/view/human_chart_crd_test.go +++ b/internal/plan/view/human_chart_crd_test.go @@ -149,27 +149,29 @@ func TestWriteHuman_ChartCRDOrder(t *testing.T) { func TestWriteHuman_ChartCRDPlacement(t *testing.T) { t.Parallel() - p := mustPlanWithHeader(t, humanHeader(), semantic.HelmUpgrade, []semantic.ResourceChange{{ - Resource: ref("ConfigMap", "app"), - Action: semantic.Create, - After: snap(cm("app", "v1")), - }}, []semantic.TaskPlan{{ - Name: "migrate", - Phase: semantic.TaskPreDeploy, - Action: semantic.TaskUnchanged, - WillRun: true, - }}) - var err error - p, err = rebuildWithChartCRDs(t, p, []semantic.ChartCRD{ - chartCRD(".deployah/crds/widget.yaml", "widgets.example.com", 0, semantic.ChartCRDUpgrade), + p := mustPlanInput(t, semantic.Input{ + Header: humanHeader(), + HelmAction: semantic.HelmUpgrade, + Changes: []semantic.ResourceChange{{ + Resource: ref("ConfigMap", "app"), + Action: semantic.Create, + After: snap(cm("app", "v1")), + }}, + Tasks: []semantic.TaskPlan{{ + Name: "migrate", + Phase: semantic.TaskPreDeploy, + Action: semantic.TaskUnchanged, + WillRun: true, + }}, + ChartCRDs: []semantic.ChartCRD{ + chartCRD(".deployah/crds/widget.yaml", "widgets.example.com", 0, semantic.ChartCRDUpgrade), + }, + Drift: []semantic.DriftChange{{ + Resource: ref("ConfigMap", "extra"), + Action: semantic.DriftUnexpected, + Live: snap(cm("extra", "live")), + }}, }) - require.NoError(t, err) - p, err = rebuildWithDrift(t, p, []semantic.DriftChange{{ - Resource: ref("ConfigMap", "extra"), - Action: semantic.DriftUnexpected, - Live: snap(cm("extra", "live")), - }}) - require.NoError(t, err) text := writeHuman(t, p) assertHumanLayout(t, text) @@ -210,17 +212,19 @@ func TestWriteHuman_ChartCRDSectionUsesNoDiffRole(t *testing.T) { func TestWriteHuman_ChartCRDWriteError(t *testing.T) { t.Parallel() - p := mustPlanWithHeader(t, humanHeader(), semantic.HelmUpgrade, []semantic.ResourceChange{{ - Resource: ref("ConfigMap", "app"), - Action: semantic.Create, - After: snap(cm("app", "v1")), - }}, nil) - var err error - p, err = rebuildWithChartCRDs(t, p, []semantic.ChartCRD{ - chartCRD(".deployah/crds/widget.yaml", "widgets.example.com", 0, semantic.ChartCRDUpgrade), - chartCRD(".deployah/crds/widget.yaml", "gadgets.example.com", 1, semantic.ChartCRDUpgrade), + p := mustPlanInput(t, semantic.Input{ + Header: humanHeader(), + HelmAction: semantic.HelmUpgrade, + Changes: []semantic.ResourceChange{{ + Resource: ref("ConfigMap", "app"), + Action: semantic.Create, + After: snap(cm("app", "v1")), + }}, + ChartCRDs: []semantic.ChartCRD{ + chartCRD(".deployah/crds/widget.yaml", "widgets.example.com", 0, semantic.ChartCRDUpgrade), + chartCRD(".deployah/crds/widget.yaml", "gadgets.example.com", 1, semantic.ChartCRDUpgrade), + }, }) - require.NoError(t, err) var plain captureWriter require.NoError(t, view.WriteHuman(&plain, p, view.Options{})) @@ -306,9 +310,11 @@ func chartCRD(source, name string, index int, lc semantic.ChartCRDLifecycle) sem func planWithChartCRDs(tb testing.TB, header semantic.Header, action semantic.HelmAction, crds []semantic.ChartCRD) semantic.Plan { tb.Helper() - p, err := rebuildWithChartCRDs(tb, mustPlanWithHeader(tb, header, action, nil, nil), crds) - require.NoError(tb, err) - return p + return mustPlanInput(tb, semantic.Input{ + Header: header, + HelmAction: action, + ChartCRDs: crds, + }) } // chartCRDSection returns the Chart CRDs block, from its title through diff --git a/internal/plan/view/human_semantic_test.go b/internal/plan/view/human_semantic_test.go index 8a3a556..52e2df5 100644 --- a/internal/plan/view/human_semantic_test.go +++ b/internal/plan/view/human_semantic_test.go @@ -66,10 +66,6 @@ func TestWriteHuman_SemanticPlan(t *testing.T) { assert.Equal(t, productNamespace, p.Header.Namespace) assert.False(t, p.Header.FreshInstall) assertCronJobOnlyUnderTasks(t, text, "web-prod-cleanup") - assert.NotContains(t, text, "-/+") - assert.NotContains(t, strings.ToLower(text), "recreate") - assert.NotContains(t, text, "Actions:") - assert.NotContains(t, text, "completeness:") } func TestWriteHuman_SemanticScheduleCreate(t *testing.T) { @@ -95,7 +91,6 @@ func TestWriteHuman_SemanticScheduleCreate(t *testing.T) { assertSemanticCronJobBody(t, text) assert.NotContains(t, text, "helm.sh/hook:") assert.NotContains(t, text, `create v1/Namespace`) - assert.NotContains(t, text, "-/+") } func TestWriteHuman_SemanticScheduleDelete(t *testing.T) { @@ -120,7 +115,6 @@ func TestWriteHuman_SemanticScheduleDelete(t *testing.T) { assertCronJobOnlyUnderTasks(t, text, "web-prod-cleanup") assertSemanticCronJobBody(t, text) assert.NotContains(t, text, `create v1/Namespace`) - assert.NotContains(t, text, "-/+") } func assertCronJobOnlyUnderTasks(t *testing.T, text, name string) { diff --git a/internal/plan/view/human_style_test.go b/internal/plan/view/human_style_test.go index 8584666..71a49ae 100644 --- a/internal/plan/view/human_style_test.go +++ b/internal/plan/view/human_style_test.go @@ -75,31 +75,40 @@ func TestWriteHuman_StylerPreservesLayout(t *testing.T) { func TestWriteHuman_StyleRoles(t *testing.T) { t.Parallel() header := humanHeader() - unexpected, err := rebuildWithDrift(t, mustPlanWithHeader(t, header, semantic.HelmNone, nil, nil), []semantic.DriftChange{{ - Resource: ref("ConfigMap", "extra"), - Action: semantic.DriftUnexpected, - Live: snap(cm("extra", "live")), - }}) - require.NoError(t, err) - missing, err := rebuildWithDrift(t, mustPlanWithHeader(t, header, semantic.HelmNone, nil, nil), []semantic.DriftChange{{ - Resource: ref("ConfigMap", "other"), - Action: semantic.DriftMissing, - Previous: snap(cm("other", "gone")), - }}) - require.NoError(t, err) - modified, err := rebuildWithDrift(t, mustPlanWithHeader(t, header, semantic.HelmNone, nil, nil), []semantic.DriftChange{{ - Resource: ref("ConfigMap", "app"), - Action: semantic.DriftModified, - Previous: snap(cm("app", "old")), - Live: snap(cm("app", "new")), - Fields: []semantic.FieldChange{{ - Path: "/data/key", - Op: semantic.FieldReplace, - Before: "old", - After: "new", + unexpected := mustPlanInput(t, semantic.Input{ + Header: header, + HelmAction: semantic.HelmNone, + Drift: []semantic.DriftChange{{ + Resource: ref("ConfigMap", "extra"), + Action: semantic.DriftUnexpected, + Live: snap(cm("extra", "live")), }}, - }}) - require.NoError(t, err) + }) + missing := mustPlanInput(t, semantic.Input{ + Header: header, + HelmAction: semantic.HelmNone, + Drift: []semantic.DriftChange{{ + Resource: ref("ConfigMap", "other"), + Action: semantic.DriftMissing, + Previous: snap(cm("other", "gone")), + }}, + }) + modified := mustPlanInput(t, semantic.Input{ + Header: header, + HelmAction: semantic.HelmNone, + Drift: []semantic.DriftChange{{ + Resource: ref("ConfigMap", "app"), + Action: semantic.DriftModified, + Previous: snap(cm("app", "old")), + Live: snap(cm("app", "new")), + Fields: []semantic.FieldChange{{ + Path: "/data/key", + Op: semantic.FieldReplace, + Before: "old", + After: "new", + }}, + }}, + }) tasks := mustPlanWithHeader(t, header, semantic.HelmUpgrade, nil, []semantic.TaskPlan{ { Name: "migrate", diff --git a/internal/plan/view/human_test.go b/internal/plan/view/human_test.go index ae3e78b..38e51e1 100644 --- a/internal/plan/view/human_test.go +++ b/internal/plan/view/human_test.go @@ -220,10 +220,6 @@ func TestWriteHuman_HeaderMetadata(t *testing.T) { assertHumanHeaderMetadata(t, text, tt.context, tt.revision) assert.Equal(t, tt.context, p.Header.Context) assert.Equal(t, tt.revision, p.Header.Revision) - assert.NotContains(t, text, "fresh_install") - assert.NotContains(t, text, "FreshInstall") - assert.NotContains(t, text, "completeness:") - assert.NotContains(t, text, "Executions:") }) } } @@ -233,13 +229,9 @@ func TestWriteHuman_EmptyPlan(t *testing.T) { p := mustPlan(t, semantic.HelmNone, nil) text := writeHuman(t, p) assert.Contains(t, text, `Plan for project "web" on environment "prod"`) - assert.NotContains(t, text, "completeness:") - assert.NotContains(t, text, "Executions:") assertHumanLayout(t, text) assert.Contains(t, text, " Resources: 0 create, 0 update, 0 delete") assert.NotContains(t, text, " Tasks:") - assert.NotContains(t, text, "create helm") - assert.NotContains(t, text, "Actions:") } func TestWriteHuman_WriterError(t *testing.T) { @@ -874,32 +866,34 @@ func allActionsInputs() (semantic.Header, []semantic.ResourceChange, []semantic. func humanDriftPlan(t *testing.T) semantic.Plan { t.Helper() - p, err := rebuildWithDrift(t, mustPlanWithHeader(t, humanHeader(), semantic.HelmNone, nil, nil), []semantic.DriftChange{ - { - Resource: ref("ConfigMap", "app"), - Action: semantic.DriftModified, - Previous: snap(cm("app", "old")), - Live: snap(cm("app", "new")), - Fields: []semantic.FieldChange{{ - Path: "/data/key", - Op: semantic.FieldReplace, - Before: "old", - After: "new", - }}, - }, - { - Resource: ref("ConfigMap", "other"), - Action: semantic.DriftMissing, - Previous: snap(cm("other", "gone")), - }, - { - Resource: ref("ConfigMap", "extra"), - Action: semantic.DriftUnexpected, - Live: snap(cm("extra", "live")), + return mustPlanInput(t, semantic.Input{ + Header: humanHeader(), + HelmAction: semantic.HelmNone, + Drift: []semantic.DriftChange{ + { + Resource: ref("ConfigMap", "app"), + Action: semantic.DriftModified, + Previous: snap(cm("app", "old")), + Live: snap(cm("app", "new")), + Fields: []semantic.FieldChange{{ + Path: "/data/key", + Op: semantic.FieldReplace, + Before: "old", + After: "new", + }}, + }, + { + Resource: ref("ConfigMap", "other"), + Action: semantic.DriftMissing, + Previous: snap(cm("other", "gone")), + }, + { + Resource: ref("ConfigMap", "extra"), + Action: semantic.DriftUnexpected, + Live: snap(cm("extra", "live")), + }, }, }) - require.NoError(t, err) - return p } func TestWriteHuman_DriftSection(t *testing.T) { @@ -919,29 +913,37 @@ func TestWriteHuman_EmptyDriftHasNoSection(t *testing.T) { func TestWriteHuman_DriftSecretNormalizedPath(t *testing.T) { t.Parallel() - p, err := rebuildWithDrift(t, mustPlan(t, semantic.HelmNone, nil), []semantic.DriftChange{{ - Resource: ref("Secret", "db"), - Action: semantic.DriftModified, - Previous: snap(map[string]any{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]any{"name": "db", "namespace": "prod"}, - "stringData": map[string]any{"password": "old"}, - }), - Live: snap(map[string]any{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]any{"name": "db", "namespace": "prod"}, - "data": map[string]any{"password": "bmV3"}, - }), - Fields: []semantic.FieldChange{{ - Path: "/data/password", - Op: semantic.FieldReplace, - Before: "b2xk", - After: "bmV3", + p := mustPlanInput(t, semantic.Input{ + Header: semantic.Header{ + Project: "web", + Environment: "prod", + Release: "web", + Namespace: "prod", + }, + HelmAction: semantic.HelmNone, + Drift: []semantic.DriftChange{{ + Resource: ref("Secret", "db"), + Action: semantic.DriftModified, + Previous: snap(map[string]any{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]any{"name": "db", "namespace": "prod"}, + "stringData": map[string]any{"password": "old"}, + }), + Live: snap(map[string]any{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]any{"name": "db", "namespace": "prod"}, + "data": map[string]any{"password": "bmV3"}, + }), + Fields: []semantic.FieldChange{{ + Path: "/data/password", + Op: semantic.FieldReplace, + Before: "b2xk", + After: "bmV3", + }}, }}, - }}) - require.NoError(t, err) + }) tests := []struct { name string diff --git a/internal/plan/view/json_test.go b/internal/plan/view/json_test.go index fa7fc23..bb509c3 100644 --- a/internal/plan/view/json_test.go +++ b/internal/plan/view/json_test.go @@ -117,16 +117,17 @@ func TestWriteJSON_ChartCRDs(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - p := mustPlanWithHeader(t, tc.header, tc.action, nil, nil) - var err error - p, err = rebuildWithChartCRDs(t, p, []semantic.ChartCRD{{ - Source: ".deployah/crds/widgets.yaml", - Kind: "CustomResourceDefinition", - Name: "widgets.example.com", - Lifecycle: tc.lifecycle, - WillProcess: tc.willProcess, - }}) - require.NoError(t, err) + p := mustPlanInput(t, semantic.Input{ + Header: tc.header, + HelmAction: tc.action, + ChartCRDs: []semantic.ChartCRD{{ + Source: ".deployah/crds/widgets.yaml", + Kind: "CustomResourceDefinition", + Name: "widgets.example.com", + Lifecycle: tc.lifecycle, + WillProcess: tc.willProcess, + }}, + }) var buf bytes.Buffer require.NoError(t, view.WriteJSON(&buf, p, view.Options{})) var doc map[string]any @@ -141,7 +142,6 @@ func TestWriteJSON_ChartCRDs(t *testing.T) { assert.Equal(t, tc.lifecycle.String(), entry["lifecycle"]) assert.Equal(t, tc.willProcess, entry["willProcess"]) assert.Equal(t, float64(0), entry["index"]) - assert.NotContains(t, buf.String(), "Helm install will process") validatePlanSchema(t, buf.Bytes()) }) } @@ -450,42 +450,49 @@ func TestWriteJSON_DriftSecretRedaction(t *testing.T) { func secretDriftPlan(t *testing.T) semantic.Plan { t.Helper() - p, err := rebuildWithDrift(t, mustPlan(t, semantic.HelmNone, nil), []semantic.DriftChange{ - { - Resource: ref("Secret", "db"), - Action: semantic.DriftModified, - Previous: snap(map[string]any{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]any{"name": "db", "namespace": "prod"}, - "stringData": map[string]any{"password": "old"}, - }), - Live: snap(map[string]any{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]any{"name": "db", "namespace": "prod"}, - "data": map[string]any{"password": "bmV3"}, - }), - Fields: []semantic.FieldChange{{ - Path: "/data/password", - Op: semantic.FieldReplace, - Before: "b2xk", - After: "bmV3", - }}, + return mustPlanInput(t, semantic.Input{ + Header: semantic.Header{ + Project: "web", + Environment: "prod", + Release: "web", + Namespace: "prod", }, - { - Resource: ref("Secret", "leaked"), - Action: semantic.DriftUnexpected, - Live: snap(map[string]any{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": map[string]any{"name": "leaked", "namespace": "prod"}, - "data": map[string]any{"password": "c2VjcmV0"}, - }), + HelmAction: semantic.HelmNone, + Drift: []semantic.DriftChange{ + { + Resource: ref("Secret", "db"), + Action: semantic.DriftModified, + Previous: snap(map[string]any{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]any{"name": "db", "namespace": "prod"}, + "stringData": map[string]any{"password": "old"}, + }), + Live: snap(map[string]any{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]any{"name": "db", "namespace": "prod"}, + "data": map[string]any{"password": "bmV3"}, + }), + Fields: []semantic.FieldChange{{ + Path: "/data/password", + Op: semantic.FieldReplace, + Before: "b2xk", + After: "bmV3", + }}, + }, + { + Resource: ref("Secret", "leaked"), + Action: semantic.DriftUnexpected, + Live: snap(map[string]any{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]any{"name": "leaked", "namespace": "prod"}, + "data": map[string]any{"password": "c2VjcmV0"}, + }), + }, }, }) - require.NoError(t, err) - return p } func TestWriteRenderers_ManualSnapshotShapes(t *testing.T) { diff --git a/internal/plan/view/schema_test.go b/internal/plan/view/schema_test.go index 3870628..f72580b 100644 --- a/internal/plan/view/schema_test.go +++ b/internal/plan/view/schema_test.go @@ -232,31 +232,38 @@ func TestSchemaV1_ChartCRDs(t *testing.T) { for _, tc := range valid { t.Run(tc.name, func(t *testing.T) { t.Parallel() - p := mustPlanWithHeader(t, tc.header, tc.action, nil, nil) - var attachErr error - p, attachErr = rebuildWithChartCRDs(t, p, []semantic.ChartCRD{{ - Source: ".deployah/crds/widgets.yaml", - Kind: "CustomResourceDefinition", - Name: "widgets.example.com", - Lifecycle: tc.lifecycle, - WillProcess: tc.willProcess, - }}) - require.NoError(t, attachErr) + p := mustPlanInput(t, semantic.Input{ + Header: tc.header, + HelmAction: tc.action, + ChartCRDs: []semantic.ChartCRD{{ + Source: ".deployah/crds/widgets.yaml", + Kind: "CustomResourceDefinition", + Name: "widgets.example.com", + Lifecycle: tc.lifecycle, + WillProcess: tc.willProcess, + }}, + }) raw, err := json.Marshal(mustPlanDoc(t, p)) require.NoError(t, err) validatePlanSchema(t, raw) }) } - p := mustPlan(t, semantic.HelmUpgrade, nil) - var err error - p, err = rebuildWithChartCRDs(t, p, []semantic.ChartCRD{{ - Source: ".deployah/crds/widgets.yaml", - Kind: "CustomResourceDefinition", - Name: "widgets.example.com", - Lifecycle: semantic.ChartCRDUpgrade, - }}) - require.NoError(t, err) + p := mustPlanInput(t, semantic.Input{ + Header: semantic.Header{ + Project: "web", + Environment: "prod", + Release: "web", + Namespace: "prod", + }, + HelmAction: semantic.HelmUpgrade, + ChartCRDs: []semantic.ChartCRD{{ + Source: ".deployah/crds/widgets.yaml", + Kind: "CustomResourceDefinition", + Name: "widgets.example.com", + Lifecycle: semantic.ChartCRDUpgrade, + }}, + }) base := mustPlanDoc(t, p) tests := []struct { @@ -338,32 +345,39 @@ func TestSchemaV1_RejectsMalformedDrift(t *testing.T) { func sampleDriftPlan(t *testing.T) semantic.Plan { t.Helper() - p, err := rebuildWithDrift(t, mustPlan(t, semantic.HelmNone, nil), []semantic.DriftChange{ - { - Resource: ref("ConfigMap", "app"), - Action: semantic.DriftModified, - Previous: snap(cm("app", "old")), - Live: snap(cm("app", "new")), - Fields: []semantic.FieldChange{{ - Path: "/data/key", - Op: semantic.FieldReplace, - Before: "old", - After: "new", - }}, - }, - { - Resource: ref("ConfigMap", "other"), - Action: semantic.DriftMissing, - Previous: snap(cm("other", "gone")), + return mustPlanInput(t, semantic.Input{ + Header: semantic.Header{ + Project: "web", + Environment: "prod", + Release: "web", + Namespace: "prod", }, - { - Resource: ref("ConfigMap", "extra"), - Action: semantic.DriftUnexpected, - Live: snap(cm("extra", "live")), + HelmAction: semantic.HelmNone, + Drift: []semantic.DriftChange{ + { + Resource: ref("ConfigMap", "app"), + Action: semantic.DriftModified, + Previous: snap(cm("app", "old")), + Live: snap(cm("app", "new")), + Fields: []semantic.FieldChange{{ + Path: "/data/key", + Op: semantic.FieldReplace, + Before: "old", + After: "new", + }}, + }, + { + Resource: ref("ConfigMap", "other"), + Action: semantic.DriftMissing, + Previous: snap(cm("other", "gone")), + }, + { + Resource: ref("ConfigMap", "extra"), + Action: semantic.DriftUnexpected, + Live: snap(cm("extra", "live")), + }, }, }) - require.NoError(t, err) - return p } func firstChartCRD(t *testing.T, doc map[string]any) map[string]any { From 87a00d84738add105a42457024adbe3cb5326032 Mon Sep 17 00:00:00 2001 From: Mohammad Abdolirad Date: Wed, 7 Oct 2026 10:15:06 +0200 Subject: [PATCH 3/3] simplify semantic plan docs and normalize --- internal/plan/semantic/plan.go | 29 +++++++---------------- internal/plan/semantic_build.go | 26 +++----------------- internal/plan/view/human_semantic_test.go | 2 -- 3 files changed, 12 insertions(+), 45 deletions(-) diff --git a/internal/plan/semantic/plan.go b/internal/plan/semantic/plan.go index 9b71e10..2644a92 100644 --- a/internal/plan/semantic/plan.go +++ b/internal/plan/semantic/plan.go @@ -52,10 +52,7 @@ type Input struct { // given order. Changes, tasks, and drift are sorted. func New(in Input) (Plan, error) { changes := normalizeChanges(in.Changes) - tasks, normErr := normalizeTasks(in.Tasks) - if normErr != nil { - return Plan{}, normErr - } + tasks := normalizeTasks(in.Tasks) drift := copyDrift(in.Drift) crds := slices.Clone(in.ChartCRDs) if crds == nil { @@ -115,16 +112,12 @@ func normalizeChanges(in []ResourceChange) []ResourceChange { return out } -func normalizeTasks(in []TaskPlan) ([]TaskPlan, error) { +func normalizeTasks(in []TaskPlan) []TaskPlan { out := copyTasks(in) for i := range out { - normalized, err := normalizeTask(out[i]) - if err != nil { - return nil, fmt.Errorf("task %s: %w", out[i].Name, err) - } - out[i] = normalized + out[i] = normalizeTask(out[i]) } - return out, nil + return out } // deriveWillRun overwrites caller WillRun. Schedule and delete tasks @@ -211,7 +204,7 @@ func normalizeChange(c ResourceChange) ResourceChange { return c } -func normalizeTask(t TaskPlan) (TaskPlan, error) { +func normalizeTask(t TaskPlan) TaskPlan { if t.Definitions == nil { t.Definitions = []HookDefinition{} } @@ -219,23 +212,19 @@ func normalizeTask(t TaskPlan) (TaskPlan, error) { t.Resources = []ResourceRef{} } for i := range t.Definitions { - normalized, err := normalizeDefinition(t.Definitions[i]) - if err != nil { - return TaskPlan{}, fmt.Errorf("definition %s: %w", t.Definitions[i].Resource, err) - } - t.Definitions[i] = normalized + t.Definitions[i] = normalizeDefinition(t.Definitions[i]) } - return t, nil + return t } -func normalizeDefinition(d HookDefinition) (HookDefinition, error) { +func normalizeDefinition(d HookDefinition) HookDefinition { d.Before = copySnapshot(d.Before) d.After = copySnapshot(d.After) d.Fields = copyFields(d.Fields) if d.Fields == nil { d.Fields = []FieldChange{} } - return d, nil + return d } func validateTasks(tasks []TaskPlan, changes []ResourceChange) error { diff --git a/internal/plan/semantic_build.go b/internal/plan/semantic_build.go index e33542d..dc5b7b3 100644 --- a/internal/plan/semantic_build.go +++ b/internal/plan/semantic_build.go @@ -74,29 +74,9 @@ type SemanticBuildInput struct { SkipCRDs bool } -// BuildSemanticPlan renders the spec and returns Helm release intent, -// changes from Previous to Desired, and Drift from Previous to Live. -// -// Rendering calls [SemanticBuildClient.RenderManifestsWithPrep] as -// given. A Helm client may read release history and dry-run Desired -// on the client. mapper then only checks scope and whether a resource -// can be built. It does not read Live or write. -// -// Drift is always set. A fresh install leaves it empty, and live may -// be nil. An existing release requires live. Drift only gets and -// lists. A failed read fails the plan, except when the object is -// missing. Drift leaves the Helm action, changes, tasks, chart CRDs, -// and summary as they are. -// -// Previous is the stored release, empty on install. Desired is the -// render. Only Desired is [semantic.Create], only Previous is -// [semantic.Delete], and a difference is [semantic.Update]. Equal -// objects are left out. The target namespace in the render, or in -// Previous on upgrade, fails the plan. Other namespaces stay ordinary -// changes. -// -// Chart CRD files go to Helm. Their lifecycle comes from -// [SemanticBuildInput.CRDDocs], not from changes or drift. +// BuildSemanticPlan builds a read-only semantic deployment plan from +// the rendered release and, on upgrade, from live cluster state. An +// upgrade requires live. A fresh install may pass a nil reader. // // Call the returned cleanup once. It is never nil. On error the plan // is empty and the render result is nil. diff --git a/internal/plan/view/human_semantic_test.go b/internal/plan/view/human_semantic_test.go index 52e2df5..6948bae 100644 --- a/internal/plan/view/human_semantic_test.go +++ b/internal/plan/view/human_semantic_test.go @@ -90,7 +90,6 @@ func TestWriteHuman_SemanticScheduleCreate(t *testing.T) { assertCronJobOnlyUnderTasks(t, text, "web-prod-cleanup") assertSemanticCronJobBody(t, text) assert.NotContains(t, text, "helm.sh/hook:") - assert.NotContains(t, text, `create v1/Namespace`) } func TestWriteHuman_SemanticScheduleDelete(t *testing.T) { @@ -114,7 +113,6 @@ func TestWriteHuman_SemanticScheduleDelete(t *testing.T) { assert.Contains(t, text, `- delete batch/v1/CronJob "web-prod-cleanup"`) assertCronJobOnlyUnderTasks(t, text, "web-prod-cleanup") assertSemanticCronJobBody(t, text) - assert.NotContains(t, text, `create v1/Namespace`) } func assertCronJobOnlyUnderTasks(t *testing.T, text, name string) {