Skip to content

Commit a82562f

Browse files
ostermanclaude
andcommitted
fix(kubernetes): honor validate:false, surface real git errors, close schema gap
A field-test pass over the Kustomize GitOps delivery feature found four real bugs, all fixed here: - validate: false on a Kubernetes component was silently dropped during stack processing and never reached any command: internal/exec's Kubernetes comp-assembly copied provider/paths/manifests/render but never validate. Threaded it through the full 3-layer merge (global -> base component -> instance) via the same key-presence-safe mergeComponentAnySection pattern paths/manifests already use (not provider's zero-value pattern, which would be unsafe for a bool), plus the matching --affected diffing and base-component cache entries. - Git target errors (atmos kubernetes deploy/apply --target <git>) were always opaque ("git clone (exit 128)", no cause) because the git provisioner target never captured subprocess stderr, unlike the atmos git command family. Moved the capture-and-hint machinery from cmd/git into exported pkg/git symbols (CaptureStderr, WrapOperationError) so both share one implementation, and wired it into the provisioner's clone/commit/push calls. - components.kubernetes.<name>.validate: false failed schema validation ("additionalProperties 'validate' not allowed') because the repo has three hand-maintained copies of the stack-manifest JSON schema and the original fix only patched one; atmos describe stacks/validate stacks enforce a different copy. Patched the copy that's actually enforced and added a regression test. - A successful git-target delivery printed nothing at all, unlike cluster apply and validate. Added a confirmation message. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent a051e6f commit a82562f

22 files changed

Lines changed: 483 additions & 79 deletions

cmd/git/executor.go

Lines changed: 15 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,14 @@
11
package git
22

33
import (
4-
"bytes"
54
"context"
65
"errors"
76
"fmt"
8-
"io"
97
"os"
108
"path/filepath"
11-
"strings"
129

1310
errUtils "github.com/cloudposse/atmos/errors"
1411
atmosgit "github.com/cloudposse/atmos/pkg/git"
15-
iolib "github.com/cloudposse/atmos/pkg/io"
1612
"github.com/cloudposse/atmos/pkg/perf"
1713
"github.com/cloudposse/atmos/pkg/ui"
1814
"github.com/cloudposse/atmos/pkg/ui/spinner"
@@ -38,10 +34,6 @@ type Executor struct {
3834
provider atmosgit.Provider
3935
}
4036

41-
type stderrSwapper interface {
42-
SwapStderr(io.Writer) func()
43-
}
44-
4537
// newExecutor builds an Executor using the named provider from the registry.
4638
// Pass an empty string to use the default "cli" provider.
4739
func newExecutor(providerName string) (*Executor, error) {
@@ -65,12 +57,12 @@ func (e *Executor) Init(ctx context.Context, opts *atmosgit.InitOptions, label s
6557
reconcile := initWillReconcile(opts)
6658
progressMsg := initProgressMessage(label, opts, reconcile)
6759
completedMsg := initCompletedMessage(label, opts, reconcile)
68-
stderr, err := e.captureStderr(func() error {
60+
stderr, err := atmosgit.CaptureStderr(e.provider, func() error {
6961
return spinner.ExecWithSpinner(progressMsg, completedMsg, func() error {
7062
return e.provider.Init(ctx, opts)
7163
})
7264
})
73-
return wrapGitOperationError(
65+
return atmosgit.WrapOperationError(
7466
fmt.Sprintf("initialize Git repository %q", label),
7567
opts.Workdir,
7668
stderr,
@@ -133,7 +125,7 @@ func (e *Executor) Clone(ctx context.Context, opts *atmosgit.CloneOptions, label
133125

134126
progressMsg := fmt.Sprintf("Cloning %s", label)
135127
completedMsg := fmt.Sprintf("Cloned %s into %s.", label, opts.Workdir)
136-
stderr, err := e.captureStderr(func() error {
128+
stderr, err := atmosgit.CaptureStderr(e.provider, func() error {
137129
return spinner.ExecWithSpinner(progressMsg, completedMsg, func() error {
138130
return e.provider.Clone(ctx, opts)
139131
})
@@ -144,7 +136,7 @@ func (e *Executor) Clone(ctx context.Context, opts *atmosgit.CloneOptions, label
144136
func (e *Executor) CloneWithoutSpinner(ctx context.Context, opts *atmosgit.CloneOptions, label string) error {
145137
defer perf.Track(nil, "git.Executor.CloneWithoutSpinner")()
146138

147-
stderr, err := e.captureStderr(func() error {
139+
stderr, err := atmosgit.CaptureStderr(e.provider, func() error {
148140
return e.provider.Clone(ctx, opts)
149141
})
150142
if err != nil {
@@ -155,22 +147,8 @@ func (e *Executor) CloneWithoutSpinner(ctx context.Context, opts *atmosgit.Clone
155147
return nil
156148
}
157149

158-
func (e *Executor) captureStderr(operation func() error) (string, error) {
159-
swapper, ok := e.provider.(stderrSwapper)
160-
if !ok {
161-
return "", operation()
162-
}
163-
164-
var stderr bytes.Buffer
165-
restore := swapper.SwapStderr(iolib.MaskWriter(&stderr))
166-
defer restore()
167-
168-
err := operation()
169-
return strings.TrimSpace(stderr.String()), err
170-
}
171-
172150
func wrapCloneError(label, workdir, stderr string, err error) error {
173-
return wrapGitOperationError(
151+
return atmosgit.WrapOperationError(
174152
fmt.Sprintf("clone Git repository %q", label),
175153
workdir,
176154
stderr,
@@ -179,34 +157,11 @@ func wrapCloneError(label, workdir, stderr string, err error) error {
179157
)
180158
}
181159

182-
func wrapGitOperationError(action, workdir, stderr string, err error, hint string) error {
183-
if err == nil {
184-
return nil
185-
}
186-
187-
explanation := fmt.Sprintf("Failed to %s.", action)
188-
if workdir != "" {
189-
explanation = fmt.Sprintf("Failed to %s in %q.", action, workdir)
190-
}
191-
explanation += "\n\nUnderlying error:\n\n```text\n" + err.Error() + "\n```"
192-
if stderr != "" {
193-
explanation += "\n\nGit output:\n\n```text\n" + stderr + "\n```"
194-
}
195-
196-
builder := errUtils.Build(err).
197-
WithExplanation(explanation).
198-
WithExitCode(2)
199-
if hint != "" {
200-
builder = builder.WithHint(hint)
201-
}
202-
return builder.Err()
203-
}
204-
205160
// Pull delegates to the provider.
206161
func (e *Executor) Pull(ctx context.Context, opts *atmosgit.PullOptions) error {
207162
defer perf.Track(nil, "git.Executor.Pull")()
208163

209-
stderr, err := e.captureStderr(func() error {
164+
stderr, err := atmosgit.CaptureStderr(e.provider, func() error {
210165
return e.provider.Pull(ctx, opts)
211166
})
212167
if errors.Is(err, errUtils.ErrGitNoTrackingBranch) {
@@ -218,7 +173,7 @@ func (e *Executor) Pull(ctx context.Context, opts *atmosgit.PullOptions) error {
218173
Err()
219174
}
220175
if err != nil {
221-
return wrapGitOperationError(
176+
return atmosgit.WrapOperationError(
222177
"pull Git repository",
223178
opts.Workdir,
224179
stderr,
@@ -236,13 +191,13 @@ func (e *Executor) Status(ctx context.Context, opts *atmosgit.StatusOptions) (*a
236191
defer perf.Track(nil, "git.Executor.Status")()
237192

238193
var result *atmosgit.StatusResult
239-
stderr, err := e.captureStderr(func() error {
194+
stderr, err := atmosgit.CaptureStderr(e.provider, func() error {
240195
var opErr error
241196
result, opErr = e.provider.Status(ctx, opts)
242197
return opErr
243198
})
244199
if err != nil {
245-
return nil, wrapGitOperationError("read Git status", opts.Workdir, stderr, err, "")
200+
return nil, atmosgit.WrapOperationError("read Git status", opts.Workdir, stderr, err, "")
246201
}
247202
return result, nil
248203
}
@@ -252,13 +207,13 @@ func (e *Executor) Diff(ctx context.Context, opts *atmosgit.DiffOptions) (*atmos
252207
defer perf.Track(nil, "git.Executor.Diff")()
253208

254209
var result *atmosgit.DiffResult
255-
stderr, err := e.captureStderr(func() error {
210+
stderr, err := atmosgit.CaptureStderr(e.provider, func() error {
256211
var opErr error
257212
result, opErr = e.provider.Diff(ctx, opts)
258213
return opErr
259214
})
260215
if err != nil {
261-
return nil, wrapGitOperationError("show Git diff", opts.Workdir, stderr, err, "")
216+
return nil, atmosgit.WrapOperationError("show Git diff", opts.Workdir, stderr, err, "")
262217
}
263218
return result, nil
264219
}
@@ -268,13 +223,13 @@ func (e *Executor) Commit(ctx context.Context, opts *atmosgit.CommitOptions) (*a
268223
defer perf.Track(nil, "git.Executor.Commit")()
269224

270225
var result *atmosgit.CommitResult
271-
stderr, err := e.captureStderr(func() error {
226+
stderr, err := atmosgit.CaptureStderr(e.provider, func() error {
272227
var opErr error
273228
result, opErr = e.provider.Commit(ctx, opts)
274229
return opErr
275230
})
276231
if err != nil {
277-
return nil, wrapGitOperationError("commit Git changes", opts.Workdir, stderr, err, "")
232+
return nil, atmosgit.WrapOperationError("commit Git changes", opts.Workdir, stderr, err, "")
278233
}
279234
return result, nil
280235
}
@@ -283,11 +238,11 @@ func (e *Executor) Commit(ctx context.Context, opts *atmosgit.CommitOptions) (*a
283238
func (e *Executor) Push(ctx context.Context, opts *atmosgit.PushOptions) error {
284239
defer perf.Track(nil, "git.Executor.Push")()
285240

286-
stderr, err := e.captureStderr(func() error {
241+
stderr, err := atmosgit.CaptureStderr(e.provider, func() error {
287242
return e.provider.Push(ctx, opts)
288243
})
289244
if err != nil {
290-
return wrapGitOperationError(
245+
return atmosgit.WrapOperationError(
291246
"push Git repository",
292247
opts.Workdir,
293248
stderr,

internal/exec/describe_affected_components.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,7 @@ func addKubernetesSectionAffected(
590590
{sectionNamePaths, affectedReasonStackPaths},
591591
{sectionNameManifests, affectedReasonStackManifests},
592592
{sectionNameRender, affectedReasonStackRender},
593+
{cfg.ValidateSectionName, fmt.Sprintf("stack.%s", cfg.ValidateSectionName)},
593594
}...)
594595
sections = appendSectionChecks(sections, resolveComponentSectionChecks(atmosConfig)...)
595596

internal/exec/stack_processor_cache.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,11 @@ func deepCopyBaseComponentConfigMaps(dst, src *schema.BaseComponentConfig) error
139139
return err
140140
}
141141
}
142+
if src.BaseComponentValidate != nil {
143+
if dst.BaseComponentValidate, err = deepCopyComponentAnySection(src.BaseComponentValidate); err != nil {
144+
return err
145+
}
146+
}
142147
if src.BaseComponentPlugins != nil {
143148
if dst.BaseComponentPlugins, err = deepCopyComponentAnySection(src.BaseComponentPlugins); err != nil {
144149
return err

internal/exec/stack_processor_merge.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,17 @@ func mergeComponentConfigurations(atmosConfig *schema.AtmosConfiguration, opts *
381381
return nil, err
382382
}
383383

384+
finalComponentValidate, err := mergeComponentAnySection(
385+
mergeConfig,
386+
cfg.ValidateSectionName,
387+
opts.GlobalKubernetesValidate,
388+
result.BaseComponentValidate,
389+
result.ComponentValidate,
390+
)
391+
if err != nil {
392+
return nil, err
393+
}
394+
384395
var finalComponentRender map[string]any
385396
if opts.ComponentType == cfg.KubernetesComponentType {
386397
finalComponentRender, err = m.Merge(
@@ -608,6 +619,9 @@ func mergeComponentConfigurations(atmosConfig *schema.AtmosConfiguration, opts *
608619
if len(finalComponentRender) > 0 {
609620
comp[cfg.RenderSectionName] = finalComponentRender
610621
}
622+
if finalComponentValidate != nil {
623+
comp[cfg.ValidateSectionName] = finalComponentValidate
624+
}
611625
comp[cfg.GenerateSectionName] = finalComponentGenerate
612626
}
613627

internal/exec/stack_processor_merge_test.go

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -735,6 +735,48 @@ func TestMergeComponentConfigurations_Kubernetes(t *testing.T) {
735735
assert.Equal(t, "global-wd", provision["workdir"])
736736
assert.Equal(t, "5m", provision["timeout"])
737737
})
738+
739+
t.Run("validate-component-instance-false-overrides-base-true", func(t *testing.T) {
740+
opts := ComponentProcessorOptions{
741+
ComponentType: cfg.KubernetesComponentType,
742+
Component: "api",
743+
AtmosConfig: atmosCfg,
744+
}
745+
res := minimalComponentResult()
746+
res.BaseComponentValidate = true
747+
res.ComponentValidate = false
748+
comp, err := mergeComponentConfigurations(atmosCfg, &opts, res)
749+
require.NoError(t, err)
750+
assert.Equal(t, false, comp[cfg.ValidateSectionName],
751+
"an explicit component-instance validate:false must override a base-component validate:true")
752+
})
753+
754+
t.Run("validate-base-true-flows-through-when-component-unset", func(t *testing.T) {
755+
opts := ComponentProcessorOptions{
756+
ComponentType: cfg.KubernetesComponentType,
757+
Component: "api",
758+
AtmosConfig: atmosCfg,
759+
}
760+
res := minimalComponentResult()
761+
res.BaseComponentValidate = true
762+
comp, err := mergeComponentConfigurations(atmosCfg, &opts, res)
763+
require.NoError(t, err)
764+
assert.Equal(t, true, comp[cfg.ValidateSectionName],
765+
"base-component validate:true must flow through when the component instance sets nothing")
766+
})
767+
768+
t.Run("validate-unset-everywhere-is-absent-from-comp", func(t *testing.T) {
769+
opts := ComponentProcessorOptions{
770+
ComponentType: cfg.KubernetesComponentType,
771+
Component: "api",
772+
AtmosConfig: atmosCfg,
773+
}
774+
res := minimalComponentResult()
775+
comp, err := mergeComponentConfigurations(atmosCfg, &opts, res)
776+
require.NoError(t, err)
777+
_, ok := comp[cfg.ValidateSectionName]
778+
assert.False(t, ok, "validate must be absent (not defaulted to any value) when unset at every layer")
779+
})
738780
}
739781

740782
// TestMergeComponentConfigurations_Retry covers the per-component retry merge added by

internal/exec/stack_processor_process_stacks.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,7 @@ func ProcessStackConfig(
166166
var kubernetesPaths any
167167
var kubernetesManifests any
168168
kubernetesRender := map[string]any{}
169+
var kubernetesValidate any
169170

170171
helmVars := map[string]any{}
171172
helmSettings := map[string]any{}
@@ -812,6 +813,10 @@ func ProcessStackConfig(
812813
}
813814
}
814815

816+
if i, ok := globalKubernetesSection[cfg.ValidateSectionName]; ok {
817+
kubernetesValidate = i
818+
}
819+
815820
// Helm section.
816821
if i, ok := globalHelmSection[cfg.CommandSectionName]; ok {
817822
helmCommand, ok = i.(string)
@@ -1146,6 +1151,7 @@ func ProcessStackConfig(
11461151
GlobalKubernetesPaths: kubernetesPaths,
11471152
GlobalKubernetesManifests: kubernetesManifests,
11481153
GlobalKubernetesRender: kubernetesRender,
1154+
GlobalKubernetesValidate: kubernetesValidate,
11491155
AtmosConfig: atmosConfig,
11501156
}, nil
11511157
}

internal/exec/stack_processor_process_stacks_helpers.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ type ComponentProcessorOptions struct {
5353
GlobalKubernetesPaths any
5454
GlobalKubernetesManifests any
5555
GlobalKubernetesRender map[string]any
56+
GlobalKubernetesValidate any
5657

5758
// Atmos configuration.
5859
AtmosConfig *schema.AtmosConfiguration
@@ -70,6 +71,7 @@ type ComponentProcessorResult struct {
7071
ComponentProvider string
7172
ComponentPaths any
7273
ComponentManifests any
74+
ComponentValidate any
7375
// ComponentPlugins holds the Helm CLI plugins list (helm/helmfile components).
7476
ComponentPlugins any
7577
ComponentRender map[string]any
@@ -94,6 +96,7 @@ type ComponentProcessorResult struct {
9496
BaseComponentProvider string
9597
BaseComponentPaths any
9698
BaseComponentManifests any
99+
BaseComponentValidate any
97100
// BaseComponentPlugins holds the inherited Helm CLI plugins list from base components.
98101
BaseComponentPlugins any
99102
BaseComponentRender map[string]any

internal/exec/stack_processor_process_stacks_helpers_extraction.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,10 @@ func extractComponentSections(opts *ComponentProcessorOptions, result *Component
290290
result.ComponentManifests = i
291291
}
292292

293+
if i, ok := opts.ComponentMap[cfg.ValidateSectionName]; ok {
294+
result.ComponentValidate = i
295+
}
296+
293297
if i, ok := opts.ComponentMap[cfg.RenderSectionName]; ok {
294298
componentRender, ok := i.(map[string]any)
295299
if !ok {

internal/exec/stack_processor_process_stacks_helpers_inheritance.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,7 @@ func applyBaseComponentConfig(opts *ComponentProcessorOptions, result *Component
235235
result.BaseComponentProvider = baseComponentConfig.BaseComponentProvider
236236
result.BaseComponentPaths = baseComponentConfig.BaseComponentPaths
237237
result.BaseComponentManifests = baseComponentConfig.BaseComponentManifests
238+
result.BaseComponentValidate = baseComponentConfig.BaseComponentValidate
238239
result.BaseComponentRender = baseComponentConfig.BaseComponentRender
239240
result.BaseComponentHelm = baseComponentConfig.BaseComponentHelm
240241
// BaseComponentRetry flows from the inheritance chain through to merge — see

internal/exec/stack_processor_utils.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2319,6 +2319,7 @@ func processBaseComponentConfigInternal(
23192319
var baseComponentProvider string
23202320
var baseComponentPaths any
23212321
var baseComponentManifests any
2322+
var baseComponentValidate any
23222323
var baseComponentPlugins any
23232324
var baseComponentRender map[string]any
23242325
var baseComponentHelm map[string]any
@@ -2547,6 +2548,10 @@ func processBaseComponentConfigInternal(
25472548
baseComponentManifests = baseComponentManifestsSection
25482549
}
25492550

2551+
if baseComponentValidateSection, baseComponentValidateSectionExist := baseComponentMap[cfg.ValidateSectionName]; baseComponentValidateSectionExist {
2552+
baseComponentValidate = baseComponentValidateSection
2553+
}
2554+
25502555
if baseComponentPluginsSection, baseComponentPluginsSectionExist := baseComponentMap[cfg.PluginsSectionName]; baseComponentPluginsSectionExist {
25512556
baseComponentPlugins = baseComponentPluginsSection
25522557
}
@@ -2784,6 +2789,13 @@ func processBaseComponentConfigInternal(
27842789
}
27852790
baseComponentConfig.BaseComponentManifests = mergedAny
27862791

2792+
// Base component `validate`
2793+
mergedAny, err = mergeComponentAnySection(levelMergeConfig, cfg.ValidateSectionName, baseComponentConfig.BaseComponentValidate, baseComponentValidate)
2794+
if err != nil {
2795+
return err
2796+
}
2797+
baseComponentConfig.BaseComponentValidate = mergedAny
2798+
27872799
// Base component `plugins` (Helm CLI plugins list).
27882800
mergedAny, err = mergeComponentAnySection(levelMergeConfig, cfg.PluginsSectionName, baseComponentConfig.BaseComponentPlugins, baseComponentPlugins)
27892801
if err != nil {

0 commit comments

Comments
 (0)