From e6362d74c815ad6d1c8ccc1c617aa4246a4b5b05 Mon Sep 17 00:00:00 2001 From: Carlos Lima Date: Thu, 29 May 2025 04:39:57 +0800 Subject: [PATCH 1/8] Allow post-renderer to process hooks This annotates and merges all manifests before sending to the postrender, reversing the process and recovering the filenames afterwards. closes #7891 Signed-off-by: Carlos Lima --- go.mod | 2 +- pkg/action/action.go | 105 ++++++- pkg/action/action_test.go | 578 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 677 insertions(+), 8 deletions(-) diff --git a/go.mod b/go.mod index 106c499b2..1d98d916e 100644 --- a/go.mod +++ b/go.mod @@ -45,6 +45,7 @@ require ( k8s.io/kubectl v0.33.2 oras.land/oras-go/v2 v2.6.0 sigs.k8s.io/controller-runtime v0.21.0 + sigs.k8s.io/kustomize/kyaml v0.19.0 sigs.k8s.io/yaml v1.5.0 ) @@ -175,7 +176,6 @@ require ( k8s.io/utils v0.0.0-20250321185631-1f6e0b77f77e // indirect sigs.k8s.io/json v0.0.0-20241014173422-cfa47c3a1cc8 // indirect sigs.k8s.io/kustomize/api v0.19.0 // indirect - sigs.k8s.io/kustomize/kyaml v0.19.0 // indirect sigs.k8s.io/randfill v1.0.0 // indirect sigs.k8s.io/structured-merge-diff/v4 v4.6.0 // indirect ) diff --git a/pkg/action/action.go b/pkg/action/action.go index 40194dfd7..9c99a6cfa 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -34,6 +34,8 @@ import ( "k8s.io/client-go/discovery" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" + "sigs.k8s.io/kustomize/kyaml/kio" + kyaml "sigs.k8s.io/kustomize/kyaml/yaml" chart "helm.sh/helm/v4/pkg/chart/v2" chartutil "helm.sh/helm/v4/pkg/chart/v2/util" @@ -91,6 +93,76 @@ type Configuration struct { mutex sync.Mutex } +const ( + // FilenameAnnotation is the annotation key used to store the original filename + // information in manifest annotations for post-rendering reconstruction. + FilenameAnnotation = "helm-postrender-filename" +) + +// AnnotateAndMerge combines multiple YAML files into a single stream of documents, +// adding filename annotations to each document for later reconstruction. +func AnnotateAndMerge(files map[string]string) (string, error) { + var combinedManifests []*kyaml.RNode + for fname, content := range files { + // Skip partials and empty files. + if strings.HasPrefix(path.Base(fname), "_") || strings.TrimSpace(content) == "" { + continue + } + + manifests, err := kio.ParseAll(content) + if err != nil { + return "", fmt.Errorf("parsing %s: %w", fname, err) + } + for _, manifest := range manifests { + if err := manifest.PipeE(kyaml.SetAnnotation(FilenameAnnotation, fname)); err != nil { + return "", fmt.Errorf("annotating %s: %w", fname, err) + } + combinedManifests = append(combinedManifests, manifest) + } + } + + merged, err := kio.StringAll(combinedManifests) + if err != nil { + return "", fmt.Errorf("writing merged docs: %w", err) + } + return merged, nil +} + +// SplitAndDeannotate reconstructs individual files from a merged YAML stream, +// removing filename annotations and grouping documents by their original filenames. +func SplitAndDeannotate(postrendered string) (map[string]string, error) { + manifests, err := kio.ParseAll(postrendered) + if err != nil { + return nil, fmt.Errorf("re-parsing merged buffer: %w", err) + } + + manifestsByFilename := make(map[string][]*kyaml.RNode) + for i, manifest := range manifests { + meta, err := manifest.GetMeta() + if err != nil { + return nil, fmt.Errorf("getting metadata: %w", err) + } + fname := meta.Annotations[FilenameAnnotation] + if fname == "" { + fname = fmt.Sprintf("generated-by-postrender-%d.yaml", i) + } + if err := manifest.PipeE(kyaml.ClearAnnotation(FilenameAnnotation)); err != nil { + return nil, fmt.Errorf("clearing filename annotation: %w", err) + } + manifestsByFilename[fname] = append(manifestsByFilename[fname], manifest) + } + + reconstructed := make(map[string]string, len(manifestsByFilename)) + for fname, docs := range manifestsByFilename { + fileContents, err := kio.StringAll(docs) + if err != nil { + return nil, fmt.Errorf("re-writing %s: %w", fname, err) + } + reconstructed[fname] = fileContents + } + return reconstructed, nil +} + // renderResources renders the templates in a chart // // TODO: This function is badly in need of a refactor. @@ -160,6 +232,32 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Valu } notes := notesBuffer.String() + if pr != nil { + // We need to send files to the post-renderer before sorting and splitting + // hooks from manifests. The post-renderer interface expects a stream of + // manifests (similar to what tools like Kustomize and kubectl expect), whereas + // the sorter uses filenames. + // Here, we merge the documents into a stream, post-render them, and then split + // them back into a map of filename -> content. + + // Merge files as stream of documents for sending to post renderer + var merged string + if merged, err = AnnotateAndMerge(files); err != nil { + return hs, b, notes, fmt.Errorf("error merging manifests: %w", err) + } + + // Run the post renderer + postRendered, err := pr.Run(bytes.NewBufferString(merged)) + if err != nil { + return hs, b, notes, fmt.Errorf("error while running post render on files: %w", err) + } + + // Use the file list and contents received from the post renderer + if files, err = SplitAndDeannotate(postRendered.String()); err != nil { + return hs, b, notes, fmt.Errorf("error while parsing post rendered files: %w", err) + } + } + // Sort hooks, manifests, and partials. Only hooks and manifests are returned, // as partials are not used after renderer.Render. Empty manifests are also // removed here. @@ -220,13 +318,6 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Valu } } - if pr != nil { - b, err = pr.Run(b) - if err != nil { - return hs, b, notes, fmt.Errorf("error while running post render on files: %w", err) - } - } - return hs, b, notes, nil } diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index 9436abef5..aaa5b4c16 100644 --- a/pkg/action/action_test.go +++ b/pkg/action/action_test.go @@ -16,13 +16,17 @@ limitations under the License. package action import ( + "bytes" + "errors" "flag" "fmt" "io" "log/slog" + "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" fakeclientset "k8s.io/client-go/kubernetes/fake" "helm.sh/helm/v4/internal/logging" @@ -368,3 +372,577 @@ func TestGetVersionSet(t *testing.T) { t.Error("Non-existent version is reported found.") } } + +// Mock PostRenderer for testing +type mockPostRenderer struct { + shouldError bool + transform func(string) string +} + +func (m *mockPostRenderer) Run(renderedManifests *bytes.Buffer) (*bytes.Buffer, error) { + if m.shouldError { + return nil, errors.New("mock post-renderer error") + } + + content := renderedManifests.String() + if m.transform != nil { + content = m.transform(content) + } + + return bytes.NewBufferString(content), nil +} + +func TestAnnotateAndMerge(t *testing.T) { + tests := []struct { + name string + files map[string]string + expectedError string + expected string + }{ + { + name: "no files", + files: map[string]string{}, + expected: "", + }, + { + name: "single file with single manifest", + files: map[string]string{ + "templates/configmap.yaml": `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm +data: + key: value`, + }, + expected: `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm + annotations: + helm-postrender-filename: 'templates/configmap.yaml' +data: + key: value +`, + }, + { + name: "multiple files with multiple manifests", + files: map[string]string{ + "templates/configmap.yaml": `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm +data: + key: value`, + "templates/secret.yaml": `apiVersion: v1 +kind: Secret +metadata: + name: test-secret +data: + password: dGVzdA==`, + }, + expected: `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm + annotations: + helm-postrender-filename: 'templates/configmap.yaml' +data: + key: value +--- +apiVersion: v1 +kind: Secret +metadata: + name: test-secret + annotations: + helm-postrender-filename: 'templates/secret.yaml' +data: + password: dGVzdA== +`, + }, + { + name: "file with multiple manifests", + files: map[string]string{ + "templates/multi.yaml": `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm1 +data: + key: value1 +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm2 +data: + key: value2`, + }, + expected: `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm1 + annotations: + helm-postrender-filename: 'templates/multi.yaml' +data: + key: value1 +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm2 + annotations: + helm-postrender-filename: 'templates/multi.yaml' +data: + key: value2 +`, + }, + { + name: "partials and empty files are removed", + files: map[string]string{ + "templates/cm.yaml": `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm1 +`, + "templates/_partial.tpl": ` +{{-define name}} + {{- "abracadabra"}} +{{- end -}}`, + "templates/empty.yaml": ``, + }, + expected: `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm1 + annotations: + helm-postrender-filename: 'templates/cm.yaml' +`, + }, + { + name: "empty file", + files: map[string]string{ + "templates/empty.yaml": "", + }, + expected: ``, + }, + { + name: "invalid yaml", + files: map[string]string{ + "templates/invalid.yaml": `invalid: yaml: content: + - malformed`, + }, + expectedError: "parsing templates/invalid.yaml", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + merged, err := AnnotateAndMerge(tt.files) + + if tt.expectedError != "" { + assert.Error(t, err) + assert.Contains(t, err.Error(), tt.expectedError) + } else { + assert.NoError(t, err) + assert.NotNil(t, merged) + assert.Equal(t, tt.expected, merged) + } + }) + } +} + +func TestSplitAndDeannotate(t *testing.T) { + tests := []struct { + name string + input string + expectedFiles map[string]string + expectedError string + }{ + { + name: "single annotated manifest", + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm + annotations: + helm-postrender-filename: templates/configmap.yaml +data: + key: value`, + expectedFiles: map[string]string{ + "templates/configmap.yaml": `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm +data: + key: value +`, + }, + }, + { + name: "multiple manifests with different filenames", + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm + annotations: + helm-postrender-filename: templates/configmap.yaml +data: + key: value +--- +apiVersion: v1 +kind: Secret +metadata: + name: test-secret + annotations: + helm-postrender-filename: templates/secret.yaml +data: + password: dGVzdA==`, + expectedFiles: map[string]string{ + "templates/configmap.yaml": `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm +data: + key: value +`, + "templates/secret.yaml": `apiVersion: v1 +kind: Secret +metadata: + name: test-secret +data: + password: dGVzdA== +`, + }, + }, + { + name: "multiple manifests with same filename", + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm1 + annotations: + helm-postrender-filename: templates/multi.yaml +data: + key: value1 +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm2 + annotations: + helm-postrender-filename: templates/multi.yaml +data: + key: value2`, + expectedFiles: map[string]string{ + "templates/multi.yaml": `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm1 +data: + key: value1 +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm2 +data: + key: value2 +`, + }, + }, + { + name: "manifest with other annotations", + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm + annotations: + helm-postrender-filename: templates/configmap.yaml + other-annotation: should-remain +data: + key: value`, + expectedFiles: map[string]string{ + "templates/configmap.yaml": `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm + annotations: + other-annotation: should-remain +data: + key: value +`, + }, + }, + { + name: "invalid yaml input", + input: "invalid: yaml: content:", + expectedError: "re-parsing merged buffer", + }, + { + name: "manifest without filename annotation", + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm +data: + key: value`, + expectedFiles: map[string]string{ + "generated-by-postrender-0.yaml": `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm +data: + key: value +`, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + files, err := SplitAndDeannotate(tt.input) + + if tt.expectedError != "" { + assert.Error(t, err) + assert.Contains(t, err.Error(), tt.expectedError) + } else { + assert.NoError(t, err) + assert.Equal(t, len(tt.expectedFiles), len(files)) + + for expectedFile, expectedContent := range tt.expectedFiles { + actualContent, exists := files[expectedFile] + assert.True(t, exists, "Expected file %s not found", expectedFile) + assert.Equal(t, expectedContent, actualContent) + } + } + }) + } +} + +func TestAnnotateAndMerge_SplitAndDeannotate_Roundtrip(t *testing.T) { + // Test that merge/split operations are symmetric + originalFiles := map[string]string{ + "templates/configmap.yaml": `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm +data: + key: value`, + "templates/secret.yaml": `apiVersion: v1 +kind: Secret +metadata: + name: test-secret +data: + password: dGVzdA==`, + "templates/multi.yaml": `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm1 +data: + key: value1 +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-cm2 +data: + key: value2`, + } + + // Merge and annotate + merged, err := AnnotateAndMerge(originalFiles) + require.NoError(t, err) + + // Split and deannotate + reconstructed, err := SplitAndDeannotate(merged) + require.NoError(t, err) + + // Compare the results + assert.Equal(t, len(originalFiles), len(reconstructed)) + for filename, originalContent := range originalFiles { + reconstructedContent, exists := reconstructed[filename] + assert.True(t, exists, "File %s should exist in reconstructed files", filename) + + // Normalize whitespace for comparison since YAML processing might affect formatting + normalizeContent := func(content string) string { + return strings.TrimSpace(strings.ReplaceAll(content, "\r\n", "\n")) + } + + assert.Equal(t, normalizeContent(originalContent), normalizeContent(reconstructedContent)) + } +} + +func TestRenderResources_PostRenderer_Success(t *testing.T) { + cfg := actionConfigFixture(t) + + // Create a simple mock post-renderer + mockPR := &mockPostRenderer{ + transform: func(content string) string { + content = strings.ReplaceAll(content, "hello", "yellow") + content = strings.ReplaceAll(content, "goodbye", "foodpie") + return strings.ReplaceAll(content, "test-cm", "test-cm-postrendered") + }, + } + + ch := buildChart(withSampleTemplates()) + values := map[string]interface{}{} + + hooks, buf, notes, err := cfg.renderResources( + ch, values, "test-release", "", false, false, false, + mockPR, false, false, false, + ) + + assert.NoError(t, err) + assert.NotNil(t, hooks) + assert.NotNil(t, buf) + assert.Equal(t, "", notes) + expectedBuf := `--- +# Source: yellow/templates/foodpie +foodpie: world +--- +# Source: yellow/templates/with-partials +yellow: Earth +--- +# Source: yellow/templates/yellow +yellow: world +` + expectedHook := `kind: ConfigMap +metadata: + name: test-cm-postrendered + annotations: + "helm.sh/hook": post-install,pre-delete,post-upgrade +data: + name: value` + + assert.Equal(t, expectedBuf, buf.String()) + assert.Len(t, hooks, 1) + assert.Equal(t, expectedHook, hooks[0].Manifest) +} + +func TestRenderResources_PostRenderer_Error(t *testing.T) { + cfg := actionConfigFixture(t) + + // Create a post-renderer that returns an error + mockPR := &mockPostRenderer{ + shouldError: true, + } + + ch := buildChart(withSampleTemplates()) + values := map[string]interface{}{} + + _, _, _, err := cfg.renderResources( + ch, values, "test-release", "", false, false, false, + mockPR, false, false, false, + ) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "error while running post render on files") +} + +func TestRenderResources_PostRenderer_MergeError(t *testing.T) { + cfg := actionConfigFixture(t) + + // Create a mock post-renderer + mockPR := &mockPostRenderer{} + + // Create a chart with invalid YAML that would cause AnnotateAndMerge to fail + ch := &chart.Chart{ + Metadata: &chart.Metadata{ + APIVersion: "v1", + Name: "test-chart", + Version: "0.1.0", + }, + Templates: []*chart.File{ + {Name: "templates/invalid", Data: []byte("invalid: yaml: content:")}, + }, + } + values := map[string]interface{}{} + + _, _, _, err := cfg.renderResources( + ch, values, "test-release", "", false, false, false, + mockPR, false, false, false, + ) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "error merging manifests") +} + +func TestRenderResources_PostRenderer_SplitError(t *testing.T) { + cfg := actionConfigFixture(t) + + // Create a post-renderer that returns invalid YAML + mockPR := &mockPostRenderer{ + transform: func(_ string) string { + return "invalid: yaml: content:" + }, + } + + ch := buildChart(withSampleTemplates()) + values := map[string]interface{}{} + + _, _, _, err := cfg.renderResources( + ch, values, "test-release", "", false, false, false, + mockPR, false, false, false, + ) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "error while parsing post rendered files") +} + +func TestRenderResources_PostRenderer_Integration(t *testing.T) { + cfg := actionConfigFixture(t) + + mockPR := &mockPostRenderer{ + transform: func(content string) string { + return strings.ReplaceAll(content, "metadata:", "color: blue\nmetadata:") + }, + } + + ch := buildChart(withSampleTemplates()) + values := map[string]interface{}{} + + hooks, buf, notes, err := cfg.renderResources( + ch, values, "test-release", "", false, false, false, + mockPR, false, false, false, + ) + + assert.NoError(t, err) + assert.NotNil(t, hooks) + assert.NotNil(t, buf) + assert.Equal(t, "", notes) // Notes should be empty for this test + + // Verify that the post-renderer modifications are present in the output + output := buf.String() + expected := `--- +# Source: hello/templates/goodbye +goodbye: world +color: blue +--- +# Source: hello/templates/hello +hello: world +color: blue +--- +# Source: hello/templates/with-partials +hello: Earth +color: blue +` + assert.Contains(t, output, "color: blue") + assert.Equal(t, 3, strings.Count(output, "color: blue")) + assert.Equal(t, expected, output) +} + +func TestRenderResources_NoPostRenderer(t *testing.T) { + cfg := actionConfigFixture(t) + + ch := buildChart(withSampleTemplates()) + values := map[string]interface{}{} + + hooks, buf, notes, err := cfg.renderResources( + ch, values, "test-release", "", false, false, false, + nil, false, false, false, + ) + + assert.NoError(t, err) + assert.NotNil(t, hooks) + assert.NotNil(t, buf) + assert.Equal(t, "", notes) +} From 1d993f9e2d2b1297607d1e60cd6961c7898e1614 Mon Sep 17 00:00:00 2001 From: Carlos Lima Date: Sun, 15 Jun 2025 19:00:47 +0800 Subject: [PATCH 2/8] review: make filenameAnnotation private Signed-off-by: Carlos Lima --- pkg/action/action.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/action/action.go b/pkg/action/action.go index 9c99a6cfa..95a38905d 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -94,9 +94,9 @@ type Configuration struct { } const ( - // FilenameAnnotation is the annotation key used to store the original filename + // filenameAnnotation is the annotation key used to store the original filename // information in manifest annotations for post-rendering reconstruction. - FilenameAnnotation = "helm-postrender-filename" + filenameAnnotation = "helm-postrender-filename" ) // AnnotateAndMerge combines multiple YAML files into a single stream of documents, @@ -114,7 +114,7 @@ func AnnotateAndMerge(files map[string]string) (string, error) { return "", fmt.Errorf("parsing %s: %w", fname, err) } for _, manifest := range manifests { - if err := manifest.PipeE(kyaml.SetAnnotation(FilenameAnnotation, fname)); err != nil { + if err := manifest.PipeE(kyaml.SetAnnotation(filenameAnnotation, fname)); err != nil { return "", fmt.Errorf("annotating %s: %w", fname, err) } combinedManifests = append(combinedManifests, manifest) @@ -142,11 +142,11 @@ func SplitAndDeannotate(postrendered string) (map[string]string, error) { if err != nil { return nil, fmt.Errorf("getting metadata: %w", err) } - fname := meta.Annotations[FilenameAnnotation] + fname := meta.Annotations[filenameAnnotation] if fname == "" { fname = fmt.Sprintf("generated-by-postrender-%d.yaml", i) } - if err := manifest.PipeE(kyaml.ClearAnnotation(FilenameAnnotation)); err != nil { + if err := manifest.PipeE(kyaml.ClearAnnotation(filenameAnnotation)); err != nil { return nil, fmt.Errorf("clearing filename annotation: %w", err) } manifestsByFilename[fname] = append(manifestsByFilename[fname], manifest) From 855b5a44b75896c200526af0970e0f6d8bff803d Mon Sep 17 00:00:00 2001 From: Carlos Lima Date: Sun, 15 Jun 2025 19:02:21 +0800 Subject: [PATCH 3/8] review: make annotateAndMerge private Signed-off-by: Carlos Lima --- pkg/action/action.go | 6 +++--- pkg/action/action_test.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/action/action.go b/pkg/action/action.go index 95a38905d..4c02031fa 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -99,9 +99,9 @@ const ( filenameAnnotation = "helm-postrender-filename" ) -// AnnotateAndMerge combines multiple YAML files into a single stream of documents, +// annotateAndMerge combines multiple YAML files into a single stream of documents, // adding filename annotations to each document for later reconstruction. -func AnnotateAndMerge(files map[string]string) (string, error) { +func annotateAndMerge(files map[string]string) (string, error) { var combinedManifests []*kyaml.RNode for fname, content := range files { // Skip partials and empty files. @@ -242,7 +242,7 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Valu // Merge files as stream of documents for sending to post renderer var merged string - if merged, err = AnnotateAndMerge(files); err != nil { + if merged, err = annotateAndMerge(files); err != nil { return hs, b, notes, fmt.Errorf("error merging manifests: %w", err) } diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index aaa5b4c16..c55b726ea 100644 --- a/pkg/action/action_test.go +++ b/pkg/action/action_test.go @@ -536,7 +536,7 @@ metadata: for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - merged, err := AnnotateAndMerge(tt.files) + merged, err := annotateAndMerge(tt.files) if tt.expectedError != "" { assert.Error(t, err) @@ -749,7 +749,7 @@ data: } // Merge and annotate - merged, err := AnnotateAndMerge(originalFiles) + merged, err := annotateAndMerge(originalFiles) require.NoError(t, err) // Split and deannotate From b26b473bf6c4755e1b1d6ba9c63a3f2c73dfaa74 Mon Sep 17 00:00:00 2001 From: Carlos Lima Date: Sun, 15 Jun 2025 19:03:51 +0800 Subject: [PATCH 4/8] review: make splitAndDeannotate private Signed-off-by: Carlos Lima --- pkg/action/action.go | 6 +++--- pkg/action/action_test.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/action/action.go b/pkg/action/action.go index 4c02031fa..643dddc45 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -128,9 +128,9 @@ func annotateAndMerge(files map[string]string) (string, error) { return merged, nil } -// SplitAndDeannotate reconstructs individual files from a merged YAML stream, +// splitAndDeannotate reconstructs individual files from a merged YAML stream, // removing filename annotations and grouping documents by their original filenames. -func SplitAndDeannotate(postrendered string) (map[string]string, error) { +func splitAndDeannotate(postrendered string) (map[string]string, error) { manifests, err := kio.ParseAll(postrendered) if err != nil { return nil, fmt.Errorf("re-parsing merged buffer: %w", err) @@ -253,7 +253,7 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Valu } // Use the file list and contents received from the post renderer - if files, err = SplitAndDeannotate(postRendered.String()); err != nil { + if files, err = splitAndDeannotate(postRendered.String()); err != nil { return hs, b, notes, fmt.Errorf("error while parsing post rendered files: %w", err) } } diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index c55b726ea..401451a42 100644 --- a/pkg/action/action_test.go +++ b/pkg/action/action_test.go @@ -699,7 +699,7 @@ data: for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - files, err := SplitAndDeannotate(tt.input) + files, err := splitAndDeannotate(tt.input) if tt.expectedError != "" { assert.Error(t, err) @@ -753,7 +753,7 @@ data: require.NoError(t, err) // Split and deannotate - reconstructed, err := SplitAndDeannotate(merged) + reconstructed, err := splitAndDeannotate(merged) require.NoError(t, err) // Compare the results From 859721bd770a5a0eba196813b643d95dc0c181cc Mon Sep 17 00:00:00 2001 From: Carlos Lima Date: Sun, 15 Jun 2025 19:12:24 +0800 Subject: [PATCH 5/8] review: rewrite error messages from the end-user perspective Signed-off-by: Carlos Lima --- pkg/action/action.go | 4 ++-- pkg/action/action_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/action/action.go b/pkg/action/action.go index 643dddc45..4c028df5f 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -133,7 +133,7 @@ func annotateAndMerge(files map[string]string) (string, error) { func splitAndDeannotate(postrendered string) (map[string]string, error) { manifests, err := kio.ParseAll(postrendered) if err != nil { - return nil, fmt.Errorf("re-parsing merged buffer: %w", err) + return nil, fmt.Errorf("error parsing YAML: %w", err) } manifestsByFilename := make(map[string][]*kyaml.RNode) @@ -254,7 +254,7 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Valu // Use the file list and contents received from the post renderer if files, err = splitAndDeannotate(postRendered.String()); err != nil { - return hs, b, notes, fmt.Errorf("error while parsing post rendered files: %w", err) + return hs, b, notes, fmt.Errorf("error while parsing post rendered output: %w", err) } } diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index 401451a42..892c4c226 100644 --- a/pkg/action/action_test.go +++ b/pkg/action/action_test.go @@ -675,7 +675,7 @@ data: { name: "invalid yaml input", input: "invalid: yaml: content:", - expectedError: "re-parsing merged buffer", + expectedError: "error parsing YAML: MalformedYAMLError", }, { name: "manifest without filename annotation", @@ -885,7 +885,7 @@ func TestRenderResources_PostRenderer_SplitError(t *testing.T) { ) assert.Error(t, err) - assert.Contains(t, err.Error(), "error while parsing post rendered files") + assert.Contains(t, err.Error(), "error while parsing post rendered output: error parsing YAML: MalformedYAMLError:") } func TestRenderResources_PostRenderer_Integration(t *testing.T) { From a1416cf2255a01bd25e6efaa515ac24bb95efd86 Mon Sep 17 00:00:00 2001 From: Carlos Lima Date: Sun, 15 Jun 2025 19:18:50 +0800 Subject: [PATCH 6/8] review: style changes Signed-off-by: Carlos Lima --- pkg/action/action.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/action/action.go b/pkg/action/action.go index 4c028df5f..d67564688 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -241,8 +241,8 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Valu // them back into a map of filename -> content. // Merge files as stream of documents for sending to post renderer - var merged string - if merged, err = annotateAndMerge(files); err != nil { + merged, err := annotateAndMerge(files) + if err != nil { return hs, b, notes, fmt.Errorf("error merging manifests: %w", err) } @@ -253,7 +253,8 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Valu } // Use the file list and contents received from the post renderer - if files, err = splitAndDeannotate(postRendered.String()); err != nil { + files, err = splitAndDeannotate(postRendered.String()) + if err != nil { return hs, b, notes, fmt.Errorf("error while parsing post rendered output: %w", err) } } From c01e76b5c384886c8daaebf9436a16dbab37c77c Mon Sep 17 00:00:00 2001 From: Carlos Lima Date: Sun, 15 Jun 2025 19:20:51 +0800 Subject: [PATCH 7/8] review: change annotation name to postrenderer.helm.sh/postrender-filename Signed-off-by: Carlos Lima --- pkg/action/action.go | 2 +- pkg/action/action_test.go | 24 ++++++++++++------------ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/pkg/action/action.go b/pkg/action/action.go index d67564688..b6ac047b7 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -96,7 +96,7 @@ type Configuration struct { const ( // filenameAnnotation is the annotation key used to store the original filename // information in manifest annotations for post-rendering reconstruction. - filenameAnnotation = "helm-postrender-filename" + filenameAnnotation = "postrenderer.helm.sh/postrender-filename" ) // annotateAndMerge combines multiple YAML files into a single stream of documents, diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index 892c4c226..43cf94622 100644 --- a/pkg/action/action_test.go +++ b/pkg/action/action_test.go @@ -419,7 +419,7 @@ kind: ConfigMap metadata: name: test-cm annotations: - helm-postrender-filename: 'templates/configmap.yaml' + postrenderer.helm.sh/postrender-filename: 'templates/configmap.yaml' data: key: value `, @@ -445,7 +445,7 @@ kind: ConfigMap metadata: name: test-cm annotations: - helm-postrender-filename: 'templates/configmap.yaml' + postrenderer.helm.sh/postrender-filename: 'templates/configmap.yaml' data: key: value --- @@ -454,7 +454,7 @@ kind: Secret metadata: name: test-secret annotations: - helm-postrender-filename: 'templates/secret.yaml' + postrenderer.helm.sh/postrender-filename: 'templates/secret.yaml' data: password: dGVzdA== `, @@ -481,7 +481,7 @@ kind: ConfigMap metadata: name: test-cm1 annotations: - helm-postrender-filename: 'templates/multi.yaml' + postrenderer.helm.sh/postrender-filename: 'templates/multi.yaml' data: key: value1 --- @@ -490,7 +490,7 @@ kind: ConfigMap metadata: name: test-cm2 annotations: - helm-postrender-filename: 'templates/multi.yaml' + postrenderer.helm.sh/postrender-filename: 'templates/multi.yaml' data: key: value2 `, @@ -514,7 +514,7 @@ kind: ConfigMap metadata: name: test-cm1 annotations: - helm-postrender-filename: 'templates/cm.yaml' + postrenderer.helm.sh/postrender-filename: 'templates/cm.yaml' `, }, { @@ -564,7 +564,7 @@ kind: ConfigMap metadata: name: test-cm annotations: - helm-postrender-filename: templates/configmap.yaml + postrenderer.helm.sh/postrender-filename: templates/configmap.yaml data: key: value`, expectedFiles: map[string]string{ @@ -584,7 +584,7 @@ kind: ConfigMap metadata: name: test-cm annotations: - helm-postrender-filename: templates/configmap.yaml + postrenderer.helm.sh/postrender-filename: templates/configmap.yaml data: key: value --- @@ -593,7 +593,7 @@ kind: Secret metadata: name: test-secret annotations: - helm-postrender-filename: templates/secret.yaml + postrenderer.helm.sh/postrender-filename: templates/secret.yaml data: password: dGVzdA==`, expectedFiles: map[string]string{ @@ -620,7 +620,7 @@ kind: ConfigMap metadata: name: test-cm1 annotations: - helm-postrender-filename: templates/multi.yaml + postrenderer.helm.sh/postrender-filename: templates/multi.yaml data: key: value1 --- @@ -629,7 +629,7 @@ kind: ConfigMap metadata: name: test-cm2 annotations: - helm-postrender-filename: templates/multi.yaml + postrenderer.helm.sh/postrender-filename: templates/multi.yaml data: key: value2`, expectedFiles: map[string]string{ @@ -656,7 +656,7 @@ kind: ConfigMap metadata: name: test-cm annotations: - helm-postrender-filename: templates/configmap.yaml + postrenderer.helm.sh/postrender-filename: templates/configmap.yaml other-annotation: should-remain data: key: value`, From 6991a0a531adb0a514eb7fadf5b8f2a2f6c17007 Mon Sep 17 00:00:00 2001 From: Carlos Lima Date: Wed, 18 Jun 2025 01:43:30 +0800 Subject: [PATCH 8/8] Make annotateAndMerge deterministic Signed-off-by: Carlos Lima --- pkg/action/action.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/action/action.go b/pkg/action/action.go index b6ac047b7..69bcf4da2 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -22,9 +22,11 @@ import ( "fmt" "io" "log/slog" + "maps" "os" "path" "path/filepath" + "slices" "strings" "sync" "text/template" @@ -103,7 +105,12 @@ const ( // adding filename annotations to each document for later reconstruction. func annotateAndMerge(files map[string]string) (string, error) { var combinedManifests []*kyaml.RNode - for fname, content := range files { + + // Get sorted filenames to ensure result is deterministic + fnames := slices.Sorted(maps.Keys(files)) + + for _, fname := range fnames { + content := files[fname] // Skip partials and empty files. if strings.HasPrefix(path.Base(fname), "_") || strings.TrimSpace(content) == "" { continue