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 <krish22092003@gmail.com>

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
+// renderResources renders the templates in a chart and returns the hooks, manifest buffer, and notes.
-//
-// 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.
 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
+	files, err := cfg.renderTemplates(ch, values, interactWithRemote, enableDNS)
-	var err2 error
+	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)
+		return e.Render(ch, values)
-	} else {
-		var e engine.Engine
-		e.EnableDNS = enableDNS
-		e.CustomTemplateFuncs = cfg.CustomTemplateFuncs
-
-		files, err2 = e.Render(ch, values)
 	}
 
-	if err2 != nil {
+	var e engine.Engine
-		return hs, b, "", err2
+	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,
+// extractNotes extracts NOTES.txt files from the rendered templates.
-	// pull it out of here into a separate file so that we can actually use the output of the rendered
+// NOTES.txt gets rendered like all the other files, but because it's not a hook nor a resource,
-	// text file. We have to spin through this map because the file contains path information, so we
+// we pull it out here into a separate string. We have to spin through this map because the file
-	// look for terminating NOTES.txt. We also remove it from the files so that we don't have to skip
+// contains path information, so we look for files ending with NOTES.txt. We also remove them from
-	// it in the sortHooks.
+// 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()
+	return notesBuffer.String(), files, nil
-
+}
-	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)
-		}
 
-		// Run the post renderer
+// applyPostRenderer applies the post-renderer to the rendered files.
-		postRendered, err := pr.Run(bytes.NewBufferString(merged))
+// We need to send files to the post-renderer before sorting and splitting hooks from manifests.
-		if err != nil {
+// The post-renderer interface expects a stream of manifests (similar to what tools like Kustomize
-			return hs, b, notes, fmt.Errorf("error while running post render on files: %w", err)
+// 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
+	// Run the post renderer
-		files, err = splitAndDeannotate(postRendered.String())
+	postRendered, err := pr.Run(bytes.NewBufferString(merged))
-		if err != nil {
+	if err != nil {
-			return hs, b, notes, fmt.Errorf("error while parsing post rendered output: %w", err)
+		return nil, fmt.Errorf("error while running post render on files: %w", err)
-		}
 	}
 
-	// Sort hooks, manifests, and partials. Only hooks and manifests are returned,
+	// Use the file list and contents received from the post renderer
-	// as partials are not used after renderer.Render. Empty manifests are also
+	files, err = splitAndDeannotate(postRendered.String())
-	// removed here.
-	hs, manifests, err := releaseutil.SortManifests(files, nil, releaseutil.InstallOrder)
 	if err != nil {
-		// By catching parse errors here, we can prevent bogus releases from going
+		return nil, fmt.Errorf("error while parsing post rendered output: %w", err)
-		// 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
 	}
 
-	// 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

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)
+}

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 <data> to <output-dir>/<name>. <appendData> 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.

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)
-}