Skip to content

Commit 2f2a2ba

Browse files
committed
fix: harden aggregate Helm result reporting
1 parent 90b8448 commit 2f2a2ba

5 files changed

Lines changed: 172 additions & 8 deletions

File tree

pkg/ci/plugins/helm/aggregate.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -277,13 +277,9 @@ func writeHelmAggregateDetails(builder *strings.Builder, command string, compone
277277
detail.WriteString(helmMarkdownCell(component.Result.Stack + "/" + component.Result.Component + ": " + component.Status))
278278
detail.WriteString("</summary>\n\n")
279279
if component.Status == "failed" {
280-
detail.WriteString("```text\n")
281-
detail.WriteString(plugin.TruncateDetail(component.Result.Error))
282-
detail.WriteString("\n```\n")
280+
detail.WriteString(helmMarkdownCodeBlock("text", plugin.TruncateDetail(component.Result.Error)))
283281
} else {
284-
detail.WriteString("```diff\n")
285-
detail.WriteString(plugin.TruncateDetail(component.Summary.Diff))
286-
detail.WriteString("\n```\n")
282+
detail.WriteString(helmMarkdownCodeBlock("diff", plugin.TruncateDetail(component.Summary.Diff)))
287283
}
288284
detail.WriteString("\n</details>\n\n")
289285
if builder.Len()+detail.Len() > helmAggregateMarkdownMaxBytes {
@@ -294,6 +290,14 @@ func writeHelmAggregateDetails(builder *strings.Builder, command string, compone
294290
}
295291
}
296292

293+
func helmMarkdownCodeBlock(language, value string) string {
294+
fence := "```"
295+
for strings.Contains(value, fence) {
296+
fence += "`"
297+
}
298+
return fence + language + "\n" + value + "\n" + fence + "\n"
299+
}
300+
297301
func formatHelmAggregateDuration(milliseconds int64) string {
298302
if milliseconds <= 0 {
299303
return "-"

pkg/ci/plugins/helm/aggregate_test.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,29 @@ func TestOnAfterAggregateRendersApplySummary(t *testing.T) {
7272
assert.Contains(t, writer.summary, "| dev | api | succeeded | api | api | apps | kubernetes | - |")
7373
}
7474

75+
func TestOnAfterAggregateUsesSafeDetailFences(t *testing.T) {
76+
writer := &fakeWriter{}
77+
err := (&Plugin{}).onAfterAggregate(&plugin.HookContext{
78+
Provider: fakeProvider{writer: writer},
79+
Aggregate: schema.HelmCIResultSet{
80+
Command: "plan",
81+
Results: []schema.HelmCIResult{
82+
{
83+
Stack: "dev", Component: "api", Processed: true,
84+
Summary: map[string]any{"diff": "+ change\n```\nnot summary Markdown"},
85+
},
86+
{
87+
Stack: "dev", Component: "worker", Status: "failed",
88+
Error: "render failed\n```\nnot summary Markdown",
89+
},
90+
},
91+
},
92+
})
93+
require.NoError(t, err)
94+
assert.Contains(t, writer.summary, "````diff\n+ change\n```\nnot summary Markdown\n````")
95+
assert.Contains(t, writer.summary, "````text\nrender failed\n```\nnot summary Markdown\n````")
96+
}
97+
7598
func TestOnAfterAggregateSkipsInvalidOrDisabledAndReturnsWriterError(t *testing.T) {
7699
pluginUnderTest := &Plugin{}
77100
require.NoError(t, pluginUnderTest.onAfterAggregate(&plugin.HookContext{Provider: fakeProvider{}, Aggregate: "invalid"}))

pkg/component/graph.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,12 @@ type GraphExecutionOptions struct {
3939
Selection *GraphSelection
4040
}
4141

42+
// GraphNodeSkipObserver is implemented by providers that need to record graph
43+
// nodes skipped after execution stops before reaching them.
44+
type GraphNodeSkipObserver interface {
45+
OnGraphNodeSkipped(node *dependency.Node)
46+
}
47+
4248
// ExecuteGraph runs selected components in dependency order and stops before
4349
// starting another component when the caller context is canceled.
4450
func ExecuteGraph(ctx context.Context, opts *GraphExecutionOptions) error {
@@ -63,24 +69,37 @@ func ExecuteGraph(ctx context.Context, opts *GraphExecutionOptions) error {
6369
for i := range order {
6470
select {
6571
case <-ctx.Done():
72+
notifyGraphNodeSkips(opts.Provider, order[i:])
6673
return fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrGraphExecutionCanceled, ctx.Err())
6774
default:
6875
}
6976

7077
if err := executeGraphNode(ctx, opts, &order[i]); err != nil {
78+
notifyGraphNodeSkips(opts.Provider, order[i+1:])
7179
if ctxErr := ctx.Err(); ctxErr != nil {
7280
return fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrGraphExecutionCanceled, errors.Join(ctxErr, err))
7381
}
7482
return err
7583
}
7684
if ctxErr := ctx.Err(); ctxErr != nil {
85+
notifyGraphNodeSkips(opts.Provider, order[i+1:])
7786
return fmt.Errorf(errUtils.ErrWrapFormat, errUtils.ErrGraphExecutionCanceled, ctxErr)
7887
}
7988
}
8089

8190
return nil
8291
}
8392

93+
func notifyGraphNodeSkips(provider ComponentProvider, nodes dependency.ExecutionOrder) {
94+
observer, ok := provider.(GraphNodeSkipObserver)
95+
if !ok {
96+
return
97+
}
98+
for i := range nodes {
99+
observer.OnGraphNodeSkipped(&nodes[i])
100+
}
101+
}
102+
84103
// prepareExecutionOrder validates options, builds and filters the graph, and returns
85104
// the topologically sorted execution order. An empty order indicates no matching components.
86105
func prepareExecutionOrder(opts *GraphExecutionOptions) (dependency.ExecutionOrder, error) {

pkg/component/helm/aggregate_ci.go

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77

88
errUtils "github.com/cloudposse/atmos/errors"
99
"github.com/cloudposse/atmos/pkg/component"
10+
"github.com/cloudposse/atmos/pkg/dependency"
1011
"github.com/cloudposse/atmos/pkg/hooks"
1112
log "github.com/cloudposse/atmos/pkg/logger"
1213
"github.com/cloudposse/atmos/pkg/schema"
@@ -32,7 +33,7 @@ func newHelmBulkCICollector(command string) *helmBulkCICollector {
3233
// bulkCollectingProvider wraps the native Helm provider and records failures
3334
// that happen before an operation produces its structured summary.
3435
type bulkCollectingProvider struct {
35-
*ComponentProvider
36+
component.ComponentProvider
3637
collector *helmBulkCICollector
3738
}
3839

@@ -43,6 +44,13 @@ func (p *bulkCollectingProvider) Execute(ctx *component.ExecutionContext) error
4344
return err
4445
}
4546

47+
func (p *bulkCollectingProvider) OnGraphNodeSkipped(node *dependency.Node) {
48+
if p == nil || p.collector == nil || node == nil {
49+
return
50+
}
51+
p.collector.markSkipped(node.Stack, node.Component)
52+
}
53+
4654
func helmBulkCollector(ctx *component.ExecutionContext) *helmBulkCICollector {
4755
if ctx == nil {
4856
return nil
@@ -82,6 +90,17 @@ func (c *helmBulkCICollector) finish(ctx *component.ExecutionContext, startedAt,
8290
applyHelmResultError(result, execErr)
8391
}
8492

93+
func (c *helmBulkCICollector) markSkipped(stack, componentName string) {
94+
if c == nil {
95+
return
96+
}
97+
c.mu.Lock()
98+
defer c.mu.Unlock()
99+
100+
result := c.ensure(stack, componentName)
101+
result.Status = "skipped"
102+
}
103+
85104
func (c *helmBulkCICollector) ensure(stack, componentName string) *schema.HelmCIResult {
86105
nodeID := component.GraphNodeID(componentName, stack)
87106
if result, ok := c.results[nodeID]; ok {
@@ -137,11 +156,28 @@ func cloneHelmSummary(summary map[string]any) map[string]any {
137156
}
138157
cloned := make(map[string]any, len(summary))
139158
for key, value := range summary {
140-
cloned[key] = value
159+
cloned[key] = cloneHelmSummaryValue(value)
141160
}
142161
return cloned
143162
}
144163

164+
func cloneHelmSummaryValue(value any) any {
165+
switch typed := value.(type) {
166+
case map[string]any:
167+
return cloneHelmSummary(typed)
168+
case []any:
169+
cloned := make([]any, len(typed))
170+
for i := range typed {
171+
cloned[i] = cloneHelmSummaryValue(typed[i])
172+
}
173+
return cloned
174+
case []string:
175+
return append([]string(nil), typed...)
176+
default:
177+
return value
178+
}
179+
}
180+
145181
func supportsHelmAggregateCI(command string) bool {
146182
switch command {
147183
case "plan", "diff", "apply", "deploy":

pkg/component/helm/aggregate_ci_test.go

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111

1212
"github.com/cloudposse/atmos/pkg/auth"
1313
"github.com/cloudposse/atmos/pkg/component"
14+
cfg "github.com/cloudposse/atmos/pkg/config"
1415
"github.com/cloudposse/atmos/pkg/hooks"
1516
"github.com/cloudposse/atmos/pkg/schema"
1617
)
@@ -39,6 +40,87 @@ func TestHelmBulkCICollectorRecordsAndSortsResults(t *testing.T) {
3940
assert.Equal(t, "+ change", collector.resultSet().Results[1].Summary["diff"])
4041
}
4142

43+
func TestHelmBulkCICollectorDeepCopiesNestedSummary(t *testing.T) {
44+
collector := newHelmBulkCICollector("apply")
45+
wait := map[string]any{"strategy": "watcher"}
46+
kinds := []string{"Deployment", "Service"}
47+
summary := map[string]any{
48+
"release": map[string]any{"wait": wait},
49+
"object_kinds": kinds,
50+
}
51+
collector.setSummary(&schema.ConfigAndStacksInfo{Stack: "dev", ComponentFromArg: "api"}, summary, nil)
52+
53+
wait["strategy"] = "legacy"
54+
kinds[0] = "Job"
55+
result := collector.resultSet().Results[0]
56+
release := result.Summary["release"].(map[string]any)
57+
assert.Equal(t, "watcher", release["wait"].(map[string]any)["strategy"])
58+
assert.Equal(t, []string{"Deployment", "Service"}, result.Summary["object_kinds"])
59+
60+
release["wait"].(map[string]any)["strategy"] = "mutated"
61+
result.Summary["object_kinds"].([]string)[1] = "ConfigMap"
62+
retained := collector.resultSet().Results[0].Summary
63+
assert.Equal(t, "watcher", retained["release"].(map[string]any)["wait"].(map[string]any)["strategy"])
64+
assert.Equal(t, []string{"Deployment", "Service"}, retained["object_kinds"])
65+
}
66+
67+
type helmBulkGraphTestProvider struct {
68+
failComponent string
69+
}
70+
71+
func (p *helmBulkGraphTestProvider) GetType() string { return cfg.HelmComponentType }
72+
func (p *helmBulkGraphTestProvider) GetGroup() string { return "test" }
73+
func (p *helmBulkGraphTestProvider) GetBasePath(*schema.AtmosConfiguration) string { return "" }
74+
func (p *helmBulkGraphTestProvider) ListComponents(context.Context, string, map[string]any) ([]string, error) {
75+
return nil, nil
76+
}
77+
func (p *helmBulkGraphTestProvider) ValidateComponent(map[string]any) error { return nil }
78+
func (p *helmBulkGraphTestProvider) Execute(ctx *component.ExecutionContext) error {
79+
if ctx.Component == p.failComponent {
80+
return errors.New("operation failed")
81+
}
82+
return nil
83+
}
84+
func (p *helmBulkGraphTestProvider) GenerateArtifacts(*component.ExecutionContext) error { return nil }
85+
func (p *helmBulkGraphTestProvider) GetAvailableCommands() []string { return nil }
86+
87+
func TestHelmBulkCICollectorRecordsDependencyBlockedComponents(t *testing.T) {
88+
collector := newHelmBulkCICollector("apply")
89+
provider := &bulkCollectingProvider{
90+
ComponentProvider: &helmBulkGraphTestProvider{failComponent: "base"},
91+
collector: collector,
92+
}
93+
stacks := map[string]any{
94+
"dev": map[string]any{
95+
cfg.ComponentsSectionName: map[string]any{
96+
cfg.HelmComponentType: map[string]any{
97+
"base": map[string]any{},
98+
"api": map[string]any{
99+
cfg.SettingsSectionName: map[string]any{"depends_on": []any{"base"}},
100+
},
101+
},
102+
},
103+
},
104+
}
105+
106+
err := component.ExecuteGraph(context.Background(), &component.GraphExecutionOptions{
107+
Provider: provider,
108+
Info: &schema.ConfigAndStacksInfo{},
109+
Stacks: stacks,
110+
ComponentType: cfg.HelmComponentType,
111+
SubCommand: "apply",
112+
})
113+
require.Error(t, err)
114+
115+
results := collector.resultSet().Results
116+
require.Len(t, results, 2)
117+
assert.Equal(t, "api", results[0].Component)
118+
assert.Equal(t, "skipped", results[0].Status)
119+
assert.False(t, results[0].Processed)
120+
assert.Equal(t, "base", results[1].Component)
121+
assert.Equal(t, "failed", results[1].Status)
122+
}
123+
42124
func TestHelmBulkCICollectorUsesProcessedComponentIdentity(t *testing.T) {
43125
collector := newHelmBulkCICollector("plan")
44126
info := schema.ConfigAndStacksInfo{Stack: "dev", ComponentFromArg: "apps/app"}

0 commit comments

Comments
 (0)