From ab8097799ecf2053b05256fded95b15403d05f3e Mon Sep 17 00:00:00 2001 From: caretak3r <50377477+caretak3r@users.noreply.github.com> Date: Wed, 18 Feb 2026 22:48:54 -0500 Subject: [PATCH] feat(spec): Task 11 - warning system for misconfigured annotations Add two new helper functions to pkg/action/sequencing.go: - warnIfPartialReadinessAnnotations: emits slog.Warn when a resource has only one of readiness-success/readiness-failure (both required) - warnIfIsolatedGroups: emits slog.Warn when multiple groups exist but some have no connections to other groups (possible misconfiguration) Both functions are called from deployResourceGroupBatches. The existing slog.Warn for non-existent group references (from ParseResourceGroups) was already present. --- pkg/action/sequencing.go | 56 +++++++++++++++++ pkg/action/sequencing_test.go | 113 ++++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+) diff --git a/pkg/action/sequencing.go b/pkg/action/sequencing.go index 3e092e5cf..29b5e37b4 100644 --- a/pkg/action/sequencing.go +++ b/pkg/action/sequencing.go @@ -164,6 +164,56 @@ func (s *sequencedDeployment) deployChartLevel(ctx context.Context, chrt *chartv return nil } +// warnIfPartialReadinessAnnotations emits a slog.Warn for each resource that has +// only one of the helm.sh/readiness-success / helm.sh/readiness-failure annotations. +// Both annotations must be present for custom readiness evaluation to work; a resource +// with only one will fall back to kstatus. +func warnIfPartialReadinessAnnotations(manifests []releaseutil.Manifest) { + for _, m := range manifests { + if m.Head == nil || m.Head.Metadata == nil { + continue + } + ann := m.Head.Metadata.Annotations + _, hasSuccess := ann[kube.AnnotationReadinessSuccess] + _, hasFailure := ann[kube.AnnotationReadinessFailure] + if hasSuccess != hasFailure { + slog.Warn("resource has only one readiness annotation; both helm.sh/readiness-success and helm.sh/readiness-failure must be present for custom readiness evaluation; falling back to kstatus", + "resource", m.Head.Metadata.Name, + ) + } + } +} + +// warnIfIsolatedGroups emits a slog.Warn for each resource-group that has no +// connections to other groups (no depends-on edges and not depended on by any +// other group). Isolated groups are valid but may indicate a misconfiguration +// when multiple groups are present. +func warnIfIsolatedGroups(result releaseutil.ResourceGroupResult) { + if len(result.Groups) <= 1 { + return // A single group is not isolated in a meaningful sense + } + for groupName := range result.Groups { + deps := result.GroupDeps[groupName] + isDependedOn := false + for _, otherDeps := range result.GroupDeps { + for _, d := range otherDeps { + if d == groupName { + isDependedOn = true + break + } + } + if isDependedOn { + break + } + } + if len(deps) == 0 && !isDependedOn { + slog.Warn("resource-group is isolated with no connections to other groups; if sequencing is intended, add helm.sh/depends-on/resource-groups annotation to related groups", + "group", groupName, + ) + } + } +} + // deployResourceGroupBatches deploys manifests for a single chart level using // resource-group annotation DAG ordering. Resources without group annotations // (or with invalid ones) are deployed last. @@ -172,6 +222,9 @@ func (s *sequencedDeployment) deployResourceGroupBatches(ctx context.Context, ma return nil } + // Emit warnings for misconfigured annotations before deploying. + warnIfPartialReadinessAnnotations(manifests) + result, warnings := releaseutil.ParseResourceGroups(manifests) for _, w := range warnings { slog.Warn("resource-group annotation warning", "warning", w) @@ -179,6 +232,9 @@ func (s *sequencedDeployment) deployResourceGroupBatches(ctx context.Context, ma // If there are sequenced groups, build their DAG and deploy in order if len(result.Groups) > 0 { + // Warn about groups that have no connections to other groups. + warnIfIsolatedGroups(result) + dag, err := releaseutil.BuildResourceGroupDAG(result) if err != nil { return fmt.Errorf("building resource-group DAG: %w", err) diff --git a/pkg/action/sequencing_test.go b/pkg/action/sequencing_test.go index 6ae335c11..a6841d1c0 100644 --- a/pkg/action/sequencing_test.go +++ b/pkg/action/sequencing_test.go @@ -17,7 +17,10 @@ limitations under the License. package action import ( + "bytes" "context" + "log/slog" + "strings" "testing" "time" @@ -218,3 +221,113 @@ func manifestNames(ms []releaseutil.Manifest) []string { } return names } + +// captureWarnings redirects the default slog logger to a buffer for the duration +// of the test and returns a function to retrieve the captured output. +func captureWarnings(t *testing.T) *bytes.Buffer { + t.Helper() + var buf bytes.Buffer + handler := slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn}) + oldLogger := slog.Default() + slog.SetDefault(slog.New(handler)) + t.Cleanup(func() { slog.SetDefault(oldLogger) }) + return &buf +} + +func TestWarnIfPartialReadinessAnnotations_OnlySuccess(t *testing.T) { + buf := captureWarnings(t) + manifests := []releaseutil.Manifest{ + makeTestManifest("cm", "chart/templates/cm.yaml", map[string]string{ + kube.AnnotationReadinessSuccess: "{.status.ready} == true", + }), + } + warnIfPartialReadinessAnnotations(manifests) + if !strings.Contains(buf.String(), "readiness") { + t.Errorf("expected warning about partial readiness annotation, got: %q", buf.String()) + } +} + +func TestWarnIfPartialReadinessAnnotations_OnlyFailure(t *testing.T) { + buf := captureWarnings(t) + manifests := []releaseutil.Manifest{ + makeTestManifest("cm", "chart/templates/cm.yaml", map[string]string{ + kube.AnnotationReadinessFailure: "{.status.failed} == true", + }), + } + warnIfPartialReadinessAnnotations(manifests) + if !strings.Contains(buf.String(), "readiness") { + t.Errorf("expected warning about partial readiness annotation, got: %q", buf.String()) + } +} + +func TestWarnIfPartialReadinessAnnotations_BothPresent(t *testing.T) { + buf := captureWarnings(t) + manifests := []releaseutil.Manifest{ + makeTestManifest("cm", "chart/templates/cm.yaml", map[string]string{ + kube.AnnotationReadinessSuccess: "{.status.ready} == true", + kube.AnnotationReadinessFailure: "{.status.failed} == true", + }), + } + warnIfPartialReadinessAnnotations(manifests) + if buf.Len() > 0 { + t.Errorf("expected no warning when both annotations present, got: %q", buf.String()) + } +} + +func TestWarnIfPartialReadinessAnnotations_NeitherPresent(t *testing.T) { + buf := captureWarnings(t) + manifests := []releaseutil.Manifest{ + makeTestManifest("cm", "chart/templates/cm.yaml", nil), + } + warnIfPartialReadinessAnnotations(manifests) + if buf.Len() > 0 { + t.Errorf("expected no warning when neither annotation present, got: %q", buf.String()) + } +} + +func TestWarnIfIsolatedGroups_TwoGroupsNoConnections(t *testing.T) { + buf := captureWarnings(t) + result := releaseutil.ResourceGroupResult{ + Groups: map[string][]releaseutil.Manifest{ + "groupA": {makeTestManifest("cmA", "chart/templates/cmA.yaml", nil)}, + "groupB": {makeTestManifest("cmB", "chart/templates/cmB.yaml", nil)}, + }, + GroupDeps: map[string][]string{}, + } + warnIfIsolatedGroups(result) + output := buf.String() + if !strings.Contains(output, "isolated") { + t.Errorf("expected warning about isolated groups, got: %q", output) + } +} + +func TestWarnIfIsolatedGroups_ConnectedGroups(t *testing.T) { + buf := captureWarnings(t) + result := releaseutil.ResourceGroupResult{ + Groups: map[string][]releaseutil.Manifest{ + "groupA": {makeTestManifest("cmA", "chart/templates/cmA.yaml", nil)}, + "groupB": {makeTestManifest("cmB", "chart/templates/cmB.yaml", nil)}, + }, + GroupDeps: map[string][]string{ + "groupB": {"groupA"}, // groupB depends on groupA + }, + } + warnIfIsolatedGroups(result) + if buf.Len() > 0 { + t.Errorf("expected no warning when groups are connected, got: %q", buf.String()) + } +} + +func TestWarnIfIsolatedGroups_SingleGroup(t *testing.T) { + buf := captureWarnings(t) + result := releaseutil.ResourceGroupResult{ + Groups: map[string][]releaseutil.Manifest{ + "groupA": {makeTestManifest("cmA", "chart/templates/cmA.yaml", nil)}, + }, + GroupDeps: map[string][]string{}, + } + warnIfIsolatedGroups(result) + if buf.Len() > 0 { + t.Errorf("expected no warning for single group, got: %q", buf.String()) + } +}