update how we merge the YAML, directly reading/writing files

Signed-off-by: Prune <prune@lecentre.net>

Author: prune998

internal/promotion/runner/builtin/yaml_merger.go

@@ -51,10 +51,8 @@ func (y *yamlMerger) Run(
 
 // convert validates yamlMerger configuration against a JSON schema and
 // converts it into a builtin.YAMLMergeConfig struct.
-func (y *yamlMerger) convert(
-	cfg promotion.Config,
-) (builtin.YAMLMergeConfig, error) {
-	return validateAndConvert[builtin.YAMLMergeConfig](y.schemaLoader, cfg, f.Name())
+func (y *yamlMerger) convert(cfg promotion.Config) (builtin.YAMLMergeConfig, error) {
+	return validateAndConvert[builtin.YAMLMergeConfig](y.schemaLoader, cfg, y.Name())
 }
 
 // validate validates yamlMerger configuration against a JSON schema.
@@ -71,63 +69,51 @@ func (y *yamlMerger) run(
 	result := promotion.StepResult{Status: kargoapi.PromotionStepStatusSucceeded}
 	failure := promotion.StepResult{Status: kargoapi.PromotionStepStatusErrored}
 
-	// Keep track of files actually merged.
-	mergedFiles := make([]string, 0, len(cfg.InFiles))
+	// sanity check
+	if len(cfg.InFiles) == 0 || cfg.OutFile == "" {
+		return failure, fmt.Errorf("inFiles and OutFile must not be empty")
+	}
 
-	// Secure join the paths to prevent path traversal attacks.
-	yamlData := []string{}
+	// Secure join the input paths to prevent path traversal attacks.
+	filePaths := []string{}
 	for _, path := range cfg.InFiles {
 		inFile, err := securejoin.SecureJoin(stepCtx.WorkDir, path)
 		if err != nil {
-			return promotion.StepResult{Status: kargoapi.PromotionStepStatusErrored},
-				fmt.Errorf("could not secure join input file %q: %w", path, err)
+			return failure, fmt.Errorf("could not secure join input file %q: %w", path, err)
 		}
 
-		inBytes, err := os.ReadFile(inFile)
+		// only add existing files
+		_, err = os.Stat(inFile)
 		if err != nil {
-			// we skip if file does not exist
-			if cfg.IgnoreMissingFiles && os.IsNotExist(err) {
+			if cfg.IgnoreMissingFiles {
 				continue
 			}
-			return failure, fmt.Errorf(
-				"error reading file %q: %w",
-				inFile,
-				err,
-			)
-		}
+			return failure, fmt.Errorf("input file not found:  %s", inFile)
 
-		// we skip if file is empty
-		if len(inBytes) == 0 {
-			continue
 		}
+		filePaths = append(filePaths, inFile)
 
-		mergedFiles = append(mergedFiles, path)
-		yamlData = append(yamlData, string(inBytes))
 	}
 
-	// Merge
-	outYAML, err := yaml.MergeYAMLFiles(yamlData)
-	if err != nil {
-		return promotion.StepResult{Status: kargoapi.PromotionStepStatusErrored},
-			fmt.Errorf("could not merge YAML files: %w", err)
-	}
-
-	// Write out a single YAML file.
+	// Secure join the output path to prevent path traversal attacks.
 	outFile, err := securejoin.SecureJoin(stepCtx.WorkDir, cfg.OutFile)
 	if err != nil {
-		return promotion.StepResult{Status: kargoapi.PromotionStepStatusErrored},
-			fmt.Errorf("could not secure join outFile %q: %w", cfg.OutFile, err)
+		return failure, fmt.Errorf("could not secure join outFile %q: %w", cfg.OutFile, err)
 	}
 
+	// ensure output path fully exist
 	if err = os.MkdirAll(filepath.Dir(outFile), 0o700); err != nil {
 		return failure, fmt.Errorf("error creating directory structure %s: %w", filepath.Dir(outFile), err)
 	}
-	if err = os.WriteFile(outFile, []byte(outYAML), 0o600); err != nil {
-		return failure, fmt.Errorf("error writing to file %s: %w", outFile, err)
+
+	// Merge files
+	err = yaml.MergeYAMLFiles(filePaths, outFile)
+	if err != nil {
+		return failure, fmt.Errorf("could not merge YAML files: %w", err)
 	}
 
 	// Add a commit message fragment to the step's output.
-	if commitMsg := y.generateCommitMessage(cfg.OutFile, mergedFiles); commitMsg != "" {
+	if commitMsg := y.generateCommitMessage(cfg.OutFile, filePaths); commitMsg != "" {
 		result.Output = map[string]any{
 			"commitMessage": commitMsg,
 		}

internal/promotion/runner/builtin/yaml_merger_test.go

@@ -100,7 +100,7 @@ func Test_YAMLMerger_run(t *testing.T) {
 			},
 			cfg: builtin.YAMLMergeConfig{
 				InFiles: []string{"base.yaml", "overrides.yaml"},
-				outFile: "modified.yaml",
+				OutFile: "modified.yaml",
 			},
 			files: map[string]string{
 				"base.yaml": `
@@ -129,17 +129,17 @@ app:
 			},
 			cfg: builtin.YAMLMergeConfig{
 				InFiles:            []string{"non-existent/values.yaml"},
-				outFile:            "modified.yaml",
+				OutFile:            "modified.yaml",
 				IgnoreMissingFiles: false,
 			},
 			assertions: func(t *testing.T, _ string, result promotion.StepResult, err error) {
 				assert.Error(t, err)
 				assert.Equal(t, promotion.StepResult{Status: kargoapi.PromotionStepStatusErrored}, result)
-				assert.Contains(t, err.Error(), "error reading file")
+				assert.Contains(t, err.Error(), "input file not found: ")
 			},
 		},
 		{
-			name: "failed to read InFiles file",
+			name: "failed when nonexistent input file is given",
 			stepCtx: &promotion.StepContext{
 				Project: "test-project",
 			},
@@ -149,13 +149,9 @@ app:
 				IgnoreMissingFiles: true,
 			},
 			assertions: func(t *testing.T, _ string, result promotion.StepResult, err error) {
-				assert.NoError(t, err)
-				assert.Equal(t, promotion.StepResult{
-					Status: kargoapi.PromotionStepStatusSucceeded,
-					Output: map[string]any{
-						"commitMessage": "Merged YAML files to modified.yaml\n",
-					},
-				}, result)
+				assert.Error(t, err)
+				assert.Equal(t, promotion.StepResult{Status: kargoapi.PromotionStepStatusErrored}, result)
+				assert.Contains(t, err.Error(), "could not merge YAML files:")
 			},
 		},
 		{
@@ -184,7 +180,7 @@ app:
 				assert.Equal(t, promotion.StepResult{
 					Status: kargoapi.PromotionStepStatusErrored,
 				}, result)
-				assert.Contains(t, err.Error(), "Error writing to file")
+				assert.Contains(t, err.Error(), "inFiles and OutFile must not be empty")
 			},
 		},
 	}

internal/yaml/yaml.go

@@ -4,7 +4,6 @@ import (
 	"bufio"
 	"bytes"
 	"fmt"
-	"io"
 	"os"
 	"strconv"
 	"strings"
@@ -150,30 +149,36 @@ func findScalarNode(node *yaml.Node, keyPath []string) (int, int, error) {
 	return 0, 0, fmt.Errorf("key path not found")
 }
 
-// MergeYAMLFiles merges a list of yaml strings.
-func MergeYAMLFiles(inputs []string) (string, error) {
-	if len(inputs) == 0 {
-		return "", fmt.Errorf("empty input list provided")
+// MergeYAMLFiles merges a list of YAML files.
+func MergeYAMLFiles(inputPaths []string, outputPath string) error {
+	if len(inputPaths) == 0 || outputPath == "" {
+		return fmt.Errorf("inFiles and OutFile must not be empty")
 	}
 
-	mergedNode, err := kyaml.Parse(inputs[0])
+	// read first YAML file
+	mergedNode, err := kyaml.ReadFile(inputPaths[0])
 	if err != nil {
-		return "", fmt.Errorf("error parsing first input: %w", err)
+		return fmt.Errorf("error parsing first input file: %w", err)
 	}
 
-	for i := 1; i < len(inputs); i++ {
-		patchNode, err := kyaml.Parse(inputs[i])
+	// read all other YAML file and apply the patch
+	for i := 1; i < len(inputPaths); i++ {
+		patchNode, err := kyaml.ReadFile(inputPaths[i])
 		if err != nil {
-			if err == io.EOF {
-				continue
-			}
-			return "", fmt.Errorf("error parsing input at index %d: %w", i, err)
+			return fmt.Errorf("error parsing input file %s: %w", inputPaths[i], err)
 		}
+
 		mergedNode, err = merge2.Merge(patchNode, mergedNode, kyaml.MergeOptions{ListIncreaseDirection: 1})
 		if err != nil {
-			return "", fmt.Errorf("error merging node at index %d: %w", i, err)
+			return fmt.Errorf("error merging node at index %d: %w", i, err)
 		}
 	}
 
-	return mergedNode.MustString(), nil
+	// write the resulting file
+	err = kyaml.WriteFile(mergedNode, outputPath)
+	if err != nil {
+		return fmt.Errorf("error writing the merged file to %s: %w", outputPath, err)
+	}
+
+	return nil
 }