From 0e2574897638e53ae9d576eb814b15ff6f30eb7a Mon Sep 17 00:00:00 2001 From: Krish Srivastava Date: Sat, 7 Feb 2026 11:12:08 +0530 Subject: [PATCH] refactor: decompose renderResources and eliminate duplicate file I/O code Resolves TODOs in pkg/action/action.go: - Refactored monolithic 159-line renderResources function into 5 focused functions - Consolidated duplicate file I/O code from install.go and cmd/template.go Changes: - Created pkg/action/filewriter.go with consolidated file writing helpers - Split renderResources into: renderTemplates, extractNotes, applyPostRenderer, writeRenderedOutput - Removed duplicate code from pkg/action/install.go and pkg/cmd/template.go - Maintained full backward compatibility All existing tests pass. No breaking changes to public API. Signed-off-by: Krish Srivastava --- pkg/action/action.go | 159 +++++++++++++++++++++------------------ pkg/action/filewriter.go | 74 ++++++++++++++++++ pkg/action/install.go | 47 ------------ pkg/cmd/template.go | 51 +------------ 4 files changed, 161 insertions(+), 170 deletions(-) create mode 100644 pkg/action/filewriter.go diff --git a/pkg/action/action.go b/pkg/action/action.go index c2a27940f..2805e593d 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -213,12 +213,7 @@ func splitAndDeannotate(postrendered string) (map[string]string, error) { return reconstructed, nil } -// renderResources renders the templates in a chart -// -// TODO: This function is badly in need of a refactor. -// TODO: As part of the refactor the duplicate code in cmd/helm/template.go should be removed -// -// This code has to do with writing files to disk. +// renderResources renders the templates in a chart and returns the hooks, manifest buffer, and notes. func (cfg *Configuration) renderResources(ch *chart.Chart, values common.Values, releaseName, outputDir string, subNotes, useReleaseName, includeCrds bool, pr postrenderer.PostRenderer, interactWithRemote, enableDNS, hideSecret bool) ([]*release.Hook, *bytes.Buffer, string, error) { var hs []*release.Hook b := bytes.NewBuffer(nil) @@ -234,43 +229,78 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values common.Values, } } - var files map[string]string - var err2 error + files, err := cfg.renderTemplates(ch, values, interactWithRemote, enableDNS) + if err != nil { + return hs, b, "", err + } + + notes, files, err := extractNotes(files, ch.Name(), subNotes) + if err != nil { + return hs, b, "", err + } + + if pr != nil { + files, err = cfg.applyPostRenderer(files, pr) + if err != nil { + return hs, b, notes, err + } + } + + hs, manifests, err := releaseutil.SortManifests(files, nil, releaseutil.InstallOrder) + if err != nil { + // By catching parse errors here, we can prevent bogus releases from going + // to Kubernetes. We return the files as a big blob of data to help the user + for name, content := range files { + if strings.TrimSpace(content) == "" { + continue + } + fmt.Fprintf(b, "---\n# Source: %s\n%s\n", name, content) + } + return hs, b, "", err + } + + err = cfg.writeRenderedOutput(b, manifests, ch.CRDObjects(), outputDir, releaseName, useReleaseName, includeCrds, hideSecret) + if err != nil { + return hs, b, "", err + } + + return hs, b, notes, nil +} +// renderTemplates renders the chart templates using the appropriate engine. +func (cfg *Configuration) renderTemplates(ch *chart.Chart, values common.Values, interactWithRemote, enableDNS bool) (map[string]string, error) { // A `helm template` should not talk to the remote cluster. However, commands with the flag // `--dry-run` with the value of `false`, `none`, or `server` should try to interact with the cluster. // It may break in interesting and exotic ways because other data (e.g. discovery) is mocked. if interactWithRemote && cfg.RESTClientGetter != nil { restConfig, err := cfg.RESTClientGetter.ToRESTConfig() if err != nil { - return hs, b, "", err + return nil, err } e := engine.New(restConfig) e.EnableDNS = enableDNS e.CustomTemplateFuncs = cfg.CustomTemplateFuncs - files, err2 = e.Render(ch, values) - } else { - var e engine.Engine - e.EnableDNS = enableDNS - e.CustomTemplateFuncs = cfg.CustomTemplateFuncs - - files, err2 = e.Render(ch, values) + return e.Render(ch, values) } - if err2 != nil { - return hs, b, "", err2 - } + var e engine.Engine + e.EnableDNS = enableDNS + e.CustomTemplateFuncs = cfg.CustomTemplateFuncs + + return e.Render(ch, values) +} - // NOTES.txt gets rendered like all the other files, but because it's not a hook nor a resource, - // pull it out of here into a separate file so that we can actually use the output of the rendered - // text file. We have to spin through this map because the file contains path information, so we - // look for terminating NOTES.txt. We also remove it from the files so that we don't have to skip - // it in the sortHooks. +// extractNotes extracts NOTES.txt files from the rendered templates. +// NOTES.txt gets rendered like all the other files, but because it's not a hook nor a resource, +// we pull it out here into a separate string. We have to spin through this map because the file +// contains path information, so we look for files ending with NOTES.txt. We also remove them from +// the files map so that we don't have to skip them in the sortHooks. +func extractNotes(files map[string]string, chartName string, subNotes bool) (string, map[string]string, error) { var notesBuffer bytes.Buffer for k, v := range files { if strings.HasSuffix(k, notesFileSuffix) { - if subNotes || (k == path.Join(ch.Name(), "templates", notesFileSuffix)) { + if subNotes || (k == path.Join(chartName, "templates", notesFileSuffix)) { // If buffer contains data, add newline before adding more if notesBuffer.Len() > 0 { notesBuffer.WriteString("\n") @@ -280,65 +310,48 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values common.Values, delete(files, k) } } - 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 - merged, err := annotateAndMerge(files) - if err != nil { - return hs, b, notes, fmt.Errorf("error merging manifests: %w", err) - } + return notesBuffer.String(), files, nil +} - // 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) - } +// applyPostRenderer applies the post-renderer to the rendered files. +// 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. +func (cfg *Configuration) applyPostRenderer(files map[string]string, pr postrenderer.PostRenderer) (map[string]string, error) { + // Merge files as stream of documents for sending to post renderer + merged, err := annotateAndMerge(files) + if err != nil { + return nil, fmt.Errorf("error merging manifests: %w", err) + } - // Use the file list and contents received from the post renderer - files, err = splitAndDeannotate(postRendered.String()) - if err != nil { - return hs, b, notes, fmt.Errorf("error while parsing post rendered output: %w", err) - } + // Run the post renderer + postRendered, err := pr.Run(bytes.NewBufferString(merged)) + if err != nil { + return nil, fmt.Errorf("error while running post render on 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. - hs, manifests, err := releaseutil.SortManifests(files, nil, releaseutil.InstallOrder) + // Use the file list and contents received from the post renderer + files, err = splitAndDeannotate(postRendered.String()) if err != nil { - // By catching parse errors here, we can prevent bogus releases from going - // to Kubernetes. - // - // We return the files as a big blob of data to help the user debug parser - // errors. - for name, content := range files { - if strings.TrimSpace(content) == "" { - continue - } - fmt.Fprintf(b, "---\n# Source: %s\n%s\n", name, content) - } - return hs, b, "", err + return nil, fmt.Errorf("error while parsing post rendered output: %w", err) } - // Aggregate all valid manifests into one big doc. + return files, nil +} + +// writeRenderedOutput writes the rendered manifests and CRDs to a buffer or to files. +func (cfg *Configuration) writeRenderedOutput(b *bytes.Buffer, manifests []releaseutil.Manifest, crds []chart.CRD, outputDir, releaseName string, useReleaseName, includeCrds, hideSecret bool) error { fileWritten := make(map[string]bool) if includeCrds { - for _, crd := range ch.CRDObjects() { + for _, crd := range crds { if outputDir == "" { fmt.Fprintf(b, "---\n# Source: %s\n%s\n", crd.Filename, string(crd.File.Data[:])) } else { - err = writeToFile(outputDir, crd.Filename, string(crd.File.Data[:]), fileWritten[crd.Filename]) + err := WriteManifestToFile(outputDir, crd.Filename, string(crd.File.Data[:]), fileWritten[crd.Filename]) if err != nil { - return hs, b, "", err + return err } fileWritten[crd.Filename] = true } @@ -361,15 +374,15 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values common.Values, // output dir is only used by `helm template`. In the next major // release, we should move this logic to template only as it is not // used by install or upgrade - err = writeToFile(newDir, m.Name, m.Content, fileWritten[m.Name]) + err := WriteManifestToFile(newDir, m.Name, m.Content, fileWritten[m.Name]) if err != nil { - return hs, b, "", err + return err } fileWritten[m.Name] = true } } - return hs, b, notes, nil + return nil } // RESTClientGetter gets the rest client diff --git a/pkg/action/filewriter.go b/pkg/action/filewriter.go new file mode 100644 index 000000000..57f0d3eed --- /dev/null +++ b/pkg/action/filewriter.go @@ -0,0 +1,74 @@ +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package action + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "strings" +) + +const defaultDirectoryPermission = 0755 + +// WriteManifestToFile writes the manifest data to a file in the output directory. +// If appendData is true, the data will be appended to an existing file; otherwise, a new file is created. +func WriteManifestToFile(outputDir string, name string, data string, appendData bool) error { + outfileName := strings.Join([]string{outputDir, name}, string(filepath.Separator)) + + err := ensureDirectoryForFile(outfileName) + if err != nil { + return err + } + + f, err := createOrOpenFile(outfileName, appendData) + if err != nil { + return err + } + + defer f.Close() + + _, err = fmt.Fprintf(f, "---\n# Source: %s\n%s\n", name, data) + + if err != nil { + return err + } + + fmt.Printf("wrote %s\n", outfileName) + return nil +} + +// createOrOpenFile creates a new file or opens an existing file for appending. +func createOrOpenFile(filename string, appendData bool) (*os.File, error) { + if appendData { + return os.OpenFile(filename, os.O_APPEND|os.O_WRONLY, 0600) + } + return os.Create(filename) +} + +// ensureDirectoryForFile checks if the directory exists to create file. Creates it if it doesn't exist. +func ensureDirectoryForFile(file string) error { + baseDir := filepath.Dir(file) + _, err := os.Stat(baseDir) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + return err + } + + return os.MkdirAll(baseDir, defaultDirectoryPermission) +} diff --git a/pkg/action/install.go b/pkg/action/install.go index 0fe1f1a6e..15c615641 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -22,7 +22,6 @@ import ( "errors" "fmt" "io" - "io/fs" "log/slog" "net/url" "os" @@ -68,8 +67,6 @@ import ( // since there can be filepath in front of it. const notesFileSuffix = "NOTES.txt" -const defaultDirectoryPermission = 0755 - // Install performs an installation operation. type Install struct { cfg *Configuration @@ -696,50 +693,6 @@ func (i *Install) replaceRelease(rel *release.Release) error { return i.recordRelease(last) } -// write the to /. controls if the file is created or content will be appended -func writeToFile(outputDir string, name string, data string, appendData bool) error { - outfileName := strings.Join([]string{outputDir, name}, string(filepath.Separator)) - - err := ensureDirectoryForFile(outfileName) - if err != nil { - return err - } - - f, err := createOrOpenFile(outfileName, appendData) - if err != nil { - return err - } - - defer f.Close() - - _, err = fmt.Fprintf(f, "---\n# Source: %s\n%s\n", name, data) - - if err != nil { - return err - } - - fmt.Printf("wrote %s\n", outfileName) - return nil -} - -func createOrOpenFile(filename string, appendData bool) (*os.File, error) { - if appendData { - return os.OpenFile(filename, os.O_APPEND|os.O_WRONLY, 0600) - } - return os.Create(filename) -} - -// check if the directory exists to create file. creates if doesn't exist -func ensureDirectoryForFile(file string) error { - baseDir := filepath.Dir(file) - _, err := os.Stat(baseDir) - if err != nil && !errors.Is(err, fs.ErrNotExist) { - return err - } - - return os.MkdirAll(baseDir, defaultDirectoryPermission) -} - // NameAndChart returns the name and chart that should be used. // // This will read the flags and handle name generation if necessary. diff --git a/pkg/cmd/template.go b/pkg/cmd/template.go index 14f85042b..112ba3b59 100644 --- a/pkg/cmd/template.go +++ b/pkg/cmd/template.go @@ -18,10 +18,8 @@ package cmd import ( "bytes" - "errors" "fmt" "io" - "io/fs" "os" "path/filepath" "regexp" @@ -137,7 +135,7 @@ func newTemplateCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { fileWritten[m.Path] = true } - err = writeToFile(newDir, m.Path, m.Manifest, fileWritten[m.Path]) + err = action.WriteManifestToFile(newDir, m.Path, m.Manifest, fileWritten[m.Path]) if err != nil { return err } @@ -228,50 +226,3 @@ func newTemplateCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { func isTestHook(h *release.Hook) bool { return slices.Contains(h.Events, release.HookTest) } - -// The following functions (writeToFile, createOrOpenFile, and ensureDirectoryForFile) -// are copied from the actions package. This is part of a change to correct a -// bug introduced by #8156. As part of the todo to refactor renderResources -// this duplicate code should be removed. It is added here so that the API -// surface area is as minimally impacted as possible in fixing the issue. -func writeToFile(outputDir string, name string, data string, appendData bool) error { - outfileName := strings.Join([]string{outputDir, name}, string(filepath.Separator)) - - err := ensureDirectoryForFile(outfileName) - if err != nil { - return err - } - - f, err := createOrOpenFile(outfileName, appendData) - if err != nil { - return err - } - - defer f.Close() - - _, err = fmt.Fprintf(f, "---\n# Source: %s\n%s\n", name, data) - - if err != nil { - return err - } - - fmt.Printf("wrote %s\n", outfileName) - return nil -} - -func createOrOpenFile(filename string, appendData bool) (*os.File, error) { - if appendData { - return os.OpenFile(filename, os.O_APPEND|os.O_WRONLY, 0600) - } - return os.Create(filename) -} - -func ensureDirectoryForFile(file string) error { - baseDir := filepath.Dir(file) - _, err := os.Stat(baseDir) - if err != nil && !errors.Is(err, fs.ErrNotExist) { - return err - } - - return os.MkdirAll(baseDir, 0755) -}