From f6c01135c8ab1aeea6e27552d8fc77b84bedf2dc Mon Sep 17 00:00:00 2001
From: Kent Rancourt <kent.rancourt@gmail.com>
Date: Wed, 18 Dec 2024 14:51:37 -0500
Subject: [PATCH 1/3] make yaml updates array-based instead of map based

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
---
 internal/directives/helm_chart_updater.go     | 15 +++---
 .../directives/helm_chart_updater_test.go     | 54 +++++++++++++++----
 internal/directives/helm_image_updater.go     | 17 +++---
 .../directives/helm_image_updater_test.go     | 45 +++++++++++-----
 internal/directives/yaml_updater.go           | 29 +++++-----
 internal/directives/yaml_updater_test.go      | 23 ++++----
 internal/yaml/yaml.go                         | 47 ++++++++--------
 internal/yaml/yaml_test.go                    | 11 ++--
 8 files changed, 150 insertions(+), 91 deletions(-)

diff --git a/internal/directives/helm_chart_updater.go b/internal/directives/helm_chart_updater.go
index 4b3f7f3fd..c47a22618 100644
--- a/internal/directives/helm_chart_updater.go
+++ b/internal/directives/helm_chart_updater.go
@@ -138,9 +138,9 @@ func (h *helmChartUpdater) processChartUpdates(
 	stepCtx *PromotionStepContext,
 	cfg HelmUpdateChartConfig,
 	chartDependencies []chartDependency,
-) (map[string]string, error) {
-	changes := make(map[string]string)
-	for _, update := range cfg.Charts {
+) ([]intyaml.Update, error) {
+	updates := make([]intyaml.Update, len(cfg.Charts))
+	for i, update := range cfg.Charts {
 		version := update.Version
 		if update.Version == "" {
 			// TODO(krancour): Remove this for v1.3.0
@@ -169,9 +169,12 @@ func (h *helmChartUpdater) processChartUpdates(
 		}
 
 		var updateUsed bool
-		for i, dep := range chartDependencies {
+		for j, dep := range chartDependencies {
 			if dep.Repository == update.Repository && dep.Name == update.Name {
-				changes[fmt.Sprintf("dependencies.%d.version", i)] = version
+				updates[i] = intyaml.Update{
+					Key:   fmt.Sprintf("dependencies.%d.version", j),
+					Value: version,
+				}
 				updateUsed = true
 				break
 			}
@@ -183,7 +186,7 @@ func (h *helmChartUpdater) processChartUpdates(
 			)
 		}
 	}
-	return changes, nil
+	return updates, nil
 }
 
 func (h *helmChartUpdater) updateDependencies(
diff --git a/internal/directives/helm_chart_updater_test.go b/internal/directives/helm_chart_updater_test.go
index 7b03f8fed..d2b29d3a5 100644
--- a/internal/directives/helm_chart_updater_test.go
+++ b/internal/directives/helm_chart_updater_test.go
@@ -26,6 +26,7 @@ import (
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
 	"github.com/akuity/kargo/internal/credentials"
 	"github.com/akuity/kargo/internal/helm"
+	intyaml "github.com/akuity/kargo/internal/yaml"
 )
 
 func Test_helmChartUpdater_validate(t *testing.T) {
@@ -310,7 +311,7 @@ func Test_helmChartUpdater_processChartUpdates(t *testing.T) {
 		context           *PromotionStepContext
 		cfg               HelmUpdateChartConfig
 		chartDependencies []chartDependency
-		assertions        func(*testing.T, map[string]string, error)
+		assertions        func(*testing.T, []intyaml.Update, error)
 	}{
 		{
 			name: "finds chart update",
@@ -358,9 +359,13 @@ func Test_helmChartUpdater_processChartUpdates(t *testing.T) {
 			chartDependencies: []chartDependency{
 				{Repository: "https://charts.example.com", Name: "test-chart"},
 			},
-			assertions: func(t *testing.T, changes map[string]string, err error) {
+			assertions: func(t *testing.T, updates []intyaml.Update, err error) {
 				assert.NoError(t, err)
-				assert.Equal(t, map[string]string{"dependencies.0.version": "1.0.0"}, changes)
+				assert.Equal(
+					t,
+					[]intyaml.Update{{Key: "dependencies.0.version", Value: "1.0.0"}},
+					updates,
+				)
 			},
 		},
 		{
@@ -378,7 +383,7 @@ func Test_helmChartUpdater_processChartUpdates(t *testing.T) {
 			chartDependencies: []chartDependency{
 				{Repository: "https://charts.example.com", Name: "non-existent-chart"},
 			},
-			assertions: func(t *testing.T, _ map[string]string, err error) {
+			assertions: func(t *testing.T, _ []intyaml.Update, err error) {
 				assert.ErrorContains(t, err, "not found in referenced Freight")
 			},
 		},
@@ -430,7 +435,7 @@ func Test_helmChartUpdater_processChartUpdates(t *testing.T) {
 				{Repository: "https://charts.example.com", Name: "chart1"},
 				{Repository: "https://charts.example.com", Name: "chart2"},
 			},
-			assertions: func(t *testing.T, _ map[string]string, err error) {
+			assertions: func(t *testing.T, _ []intyaml.Update, err error) {
 				assert.ErrorContains(t, err, "not found in referenced Freight")
 			},
 		},
@@ -484,9 +489,13 @@ func Test_helmChartUpdater_processChartUpdates(t *testing.T) {
 			chartDependencies: []chartDependency{
 				{Repository: "https://charts.example.com", Name: "origin-chart"},
 			},
-			assertions: func(t *testing.T, changes map[string]string, err error) {
+			assertions: func(t *testing.T, updates []intyaml.Update, err error) {
 				assert.NoError(t, err)
-				assert.Equal(t, map[string]string{"dependencies.0.version": "2.0.0"}, changes)
+				assert.Equal(
+					t,
+					[]intyaml.Update{{Key: "dependencies.0.version", Value: "2.0.0"}},
+					updates,
+				)
 			},
 		},
 		{
@@ -506,9 +515,32 @@ func Test_helmChartUpdater_processChartUpdates(t *testing.T) {
 			chartDependencies: []chartDependency{
 				{Repository: "https://charts.example.com", Name: "origin-chart"},
 			},
-			assertions: func(t *testing.T, changes map[string]string, err error) {
+			assertions: func(t *testing.T, updates []intyaml.Update, err error) {
 				assert.NoError(t, err)
-				assert.Equal(t, map[string]string{"dependencies.0.version": "fake-version"}, changes)
+				assert.Equal(
+					t,
+					[]intyaml.Update{{Key: "dependencies.0.version", Value: "fake-version"}},
+					updates,
+				)
+			},
+		},
+		{
+			name: "update specified for non-existent chart dependency",
+			context: &PromotionStepContext{
+				Project: "test-project",
+			},
+			cfg: HelmUpdateChartConfig{
+				Charts: []Chart{
+					{
+						Repository: "https://charts.example.com",
+						Name:       "origin-chart",
+						Version:    "fake-version",
+					},
+				},
+			},
+			chartDependencies: []chartDependency{},
+			assertions: func(t *testing.T, _ []intyaml.Update, err error) {
+				assert.ErrorContains(t, err, "no dependency in Chart.yaml matched update")
 			},
 		},
 	}
@@ -523,8 +555,8 @@ func Test_helmChartUpdater_processChartUpdates(t *testing.T) {
 			stepCtx := tt.context
 			stepCtx.KargoClient = fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.objects...).Build()
 
-			changes, err := runner.processChartUpdates(context.Background(), stepCtx, tt.cfg, tt.chartDependencies)
-			tt.assertions(t, changes, err)
+			updates, err := runner.processChartUpdates(context.Background(), stepCtx, tt.cfg, tt.chartDependencies)
+			tt.assertions(t, updates, err)
 		})
 	}
 }
diff --git a/internal/directives/helm_image_updater.go b/internal/directives/helm_image_updater.go
index 4d2f1ce09..f2d5cd099 100644
--- a/internal/directives/helm_image_updater.go
+++ b/internal/directives/helm_image_updater.go
@@ -10,7 +10,7 @@ import (
 
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
 	"github.com/akuity/kargo/internal/controller/freight"
-	libYAML "github.com/akuity/kargo/internal/yaml"
+	intyaml "github.com/akuity/kargo/internal/yaml"
 )
 
 func init() {
@@ -99,11 +99,11 @@ func (h *helmImageUpdater) generateImageUpdates(
 	ctx context.Context,
 	stepCtx *PromotionStepContext,
 	cfg HelmUpdateImageConfig,
-) (map[string]string, []string, error) {
-	updates := make(map[string]string, len(cfg.Images))
+) ([]intyaml.Update, []string, error) {
+	updates := make([]intyaml.Update, len(cfg.Images))
 	fullImageRefs := make([]string, 0, len(cfg.Images))
 
-	for _, image := range cfg.Images {
+	for i, image := range cfg.Images {
 		desiredOrigin := h.getDesiredOrigin(image.FromOrigin)
 
 		targetImage, err := freight.FindImage(
@@ -124,7 +124,10 @@ func (h *helmImageUpdater) generateImageUpdates(
 			return nil, nil, err
 		}
 
-		updates[image.Key] = value
+		updates[i] = intyaml.Update{
+			Key:   image.Key,
+			Value: value,
+		}
 		fullImageRefs = append(fullImageRefs, imageRef)
 	}
 	return updates, fullImageRefs, nil
@@ -157,12 +160,12 @@ func (h *helmImageUpdater) getImageValues(image *kargoapi.Image, valueType strin
 	}
 }
 
-func (h *helmImageUpdater) updateValuesFile(workDir string, path string, changes map[string]string) error {
+func (h *helmImageUpdater) updateValuesFile(workDir string, path string, updates []intyaml.Update) error {
 	absValuesFile, err := securejoin.SecureJoin(workDir, path)
 	if err != nil {
 		return fmt.Errorf("error joining path %q: %w", path, err)
 	}
-	if err := libYAML.SetStringsInFile(absValuesFile, changes); err != nil {
+	if err := intyaml.SetStringsInFile(absValuesFile, updates); err != nil {
 		return fmt.Errorf("error updating image references in values file %q: %w", path, err)
 	}
 	return nil
diff --git a/internal/directives/helm_image_updater_test.go b/internal/directives/helm_image_updater_test.go
index e269555f0..706e096be 100644
--- a/internal/directives/helm_image_updater_test.go
+++ b/internal/directives/helm_image_updater_test.go
@@ -16,6 +16,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
 
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
+	intyaml "github.com/akuity/kargo/internal/yaml"
 )
 
 func Test_helmImageUpdater_validate(t *testing.T) {
@@ -334,7 +335,7 @@ func Test_helmImageUpdater_generateImageUpdates(t *testing.T) {
 		objects    []client.Object
 		stepCtx    *PromotionStepContext
 		cfg        HelmUpdateImageConfig
-		assertions func(*testing.T, map[string]string, []string, error)
+		assertions func(*testing.T, []intyaml.Update, []string, error)
 	}{
 		{
 			name: "finds image update",
@@ -378,9 +379,13 @@ func Test_helmImageUpdater_generateImageUpdates(t *testing.T) {
 					{Key: "image.tag", Image: "docker.io/library/nginx", Value: Tag},
 				},
 			},
-			assertions: func(t *testing.T, changes map[string]string, summary []string, err error) {
+			assertions: func(t *testing.T, updates []intyaml.Update, summary []string, err error) {
 				assert.NoError(t, err)
-				assert.Equal(t, map[string]string{"image.tag": "1.19.0"}, changes)
+				assert.Equal(
+					t,
+					[]intyaml.Update{{Key: "image.tag", Value: "1.19.0"}},
+					updates,
+				)
 				assert.Equal(t, []string{"docker.io/library/nginx:1.19.0"}, summary)
 			},
 		},
@@ -396,7 +401,7 @@ func Test_helmImageUpdater_generateImageUpdates(t *testing.T) {
 					{Key: "image.tag", Image: "docker.io/library/non-existent", Value: Tag},
 				},
 			},
-			assertions: func(t *testing.T, _ map[string]string, _ []string, err error) {
+			assertions: func(t *testing.T, _ []intyaml.Update, _ []string, err error) {
 				assert.ErrorContains(t, err, "not found in referenced Freight")
 			},
 		},
@@ -443,7 +448,7 @@ func Test_helmImageUpdater_generateImageUpdates(t *testing.T) {
 					{Key: "image2.tag", Image: "docker.io/library/non-existent", Value: Tag},
 				},
 			},
-			assertions: func(t *testing.T, _ map[string]string, _ []string, err error) {
+			assertions: func(t *testing.T, _ []intyaml.Update, _ []string, err error) {
 				assert.ErrorContains(t, err, "not found in referenced Freight")
 			},
 		},
@@ -494,9 +499,13 @@ func Test_helmImageUpdater_generateImageUpdates(t *testing.T) {
 					},
 				},
 			},
-			assertions: func(t *testing.T, changes map[string]string, summary []string, err error) {
+			assertions: func(t *testing.T, updates []intyaml.Update, summary []string, err error) {
 				assert.NoError(t, err)
-				assert.Equal(t, map[string]string{"image.tag": "2.0.0"}, changes)
+				assert.Equal(
+					t,
+					[]intyaml.Update{{Key: "image.tag", Value: "2.0.0"}},
+					updates,
+				)
 				assert.Equal(t, []string{"docker.io/library/origin-image:2.0.0"}, summary)
 			},
 		},
@@ -512,8 +521,8 @@ func Test_helmImageUpdater_generateImageUpdates(t *testing.T) {
 			stepCtx := tt.stepCtx
 			stepCtx.KargoClient = fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.objects...).Build()
 
-			changes, summary, err := runner.generateImageUpdates(context.Background(), stepCtx, tt.cfg)
-			tt.assertions(t, changes, summary, err)
+			updates, summary, err := runner.generateImageUpdates(context.Background(), stepCtx, tt.cfg)
+			tt.assertions(t, updates, summary, err)
 		})
 	}
 }
@@ -640,13 +649,13 @@ func Test_helmImageUpdater_updateValuesFile(t *testing.T) {
 	tests := []struct {
 		name          string
 		valuesContent string
-		changes       map[string]string
+		updates       []intyaml.Update
 		assertions    func(*testing.T, string, error)
 	}{
 		{
 			name:          "successful update",
 			valuesContent: "key: value\n",
-			changes:       map[string]string{"key": "newvalue"},
+			updates:       []intyaml.Update{{Key: "key", Value: "newvalue"}},
 			assertions: func(t *testing.T, valuesFilePath string, err error) {
 				require.NoError(t, err)
 
@@ -659,7 +668,7 @@ func Test_helmImageUpdater_updateValuesFile(t *testing.T) {
 		{
 			name:          "file does not exist",
 			valuesContent: "",
-			changes:       map[string]string{"key": "value"},
+			updates:       []intyaml.Update{{Key: "key", Value: "value"}},
 			assertions: func(t *testing.T, valuesFilePath string, err error) {
 				require.ErrorContains(t, err, "no such file or directory")
 				require.NoFileExists(t, valuesFilePath)
@@ -668,7 +677,7 @@ func Test_helmImageUpdater_updateValuesFile(t *testing.T) {
 		{
 			name:          "empty changes",
 			valuesContent: "key: value\n",
-			changes:       map[string]string{},
+			updates:       []intyaml.Update{},
 			assertions: func(t *testing.T, valuesFilePath string, err error) {
 				require.NoError(t, err)
 				require.FileExists(t, valuesFilePath)
@@ -677,6 +686,14 @@ func Test_helmImageUpdater_updateValuesFile(t *testing.T) {
 				assert.Equal(t, "key: value\n", string(content))
 			},
 		},
+		{
+			name:          "update specified for non-existent key",
+			valuesContent: "key: value\n",
+			updates:       []intyaml.Update{{Key: "non-existent-key", Value: "newvalue"}},
+			assertions: func(t *testing.T, _ string, err error) {
+				require.ErrorContains(t, err, "key path not found")
+			},
+		},
 	}
 
 	runner := &helmImageUpdater{}
@@ -691,7 +708,7 @@ func Test_helmImageUpdater_updateValuesFile(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			err := runner.updateValuesFile(workDir, path.Base(valuesFile), tt.changes)
+			err := runner.updateValuesFile(workDir, path.Base(valuesFile), tt.updates)
 			tt.assertions(t, valuesFile, err)
 		})
 	}
diff --git a/internal/directives/yaml_updater.go b/internal/directives/yaml_updater.go
index 64bd86e92..6a9c96c14 100644
--- a/internal/directives/yaml_updater.go
+++ b/internal/directives/yaml_updater.go
@@ -3,14 +3,13 @@ package directives
 import (
 	"context"
 	"fmt"
-	"slices"
 	"strings"
 
 	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/xeipuuv/gojsonschema"
 
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
-	libYAML "github.com/akuity/kargo/internal/yaml"
+	intyaml "github.com/akuity/kargo/internal/yaml"
 )
 
 func init() {
@@ -66,9 +65,12 @@ func (y *yamlUpdater) runPromotionStep(
 	stepCtx *PromotionStepContext,
 	cfg YAMLUpdateConfig,
 ) (PromotionStepResult, error) {
-	updates := make(map[string]string, len(cfg.Updates))
-	for _, image := range cfg.Updates {
-		updates[image.Key] = image.Value
+	updates := make([]intyaml.Update, len(cfg.Updates))
+	for i, update := range cfg.Updates {
+		updates[i] = intyaml.Update{
+			Key:   update.Key,
+			Value: update.Value,
+		}
 	}
 
 	result := PromotionStepResult{Status: kargoapi.PromotionPhaseSucceeded}
@@ -78,7 +80,7 @@ func (y *yamlUpdater) runPromotionStep(
 				fmt.Errorf("values file update failed: %w", err)
 		}
 
-		if commitMsg := y.generateCommitMessage(cfg.Path, updates); commitMsg != "" {
+		if commitMsg := y.generateCommitMessage(cfg.Path, cfg.Updates); commitMsg != "" {
 			result.Output = map[string]any{
 				"commitMessage": commitMsg,
 			}
@@ -87,31 +89,26 @@ func (y *yamlUpdater) runPromotionStep(
 	return result, nil
 }
 
-func (y *yamlUpdater) updateFile(workDir string, path string, changes map[string]string) error {
+func (y *yamlUpdater) updateFile(workDir string, path string, updates []intyaml.Update) error {
 	absValuesFile, err := securejoin.SecureJoin(workDir, path)
 	if err != nil {
 		return fmt.Errorf("error joining path %q: %w", path, err)
 	}
-	if err := libYAML.SetStringsInFile(absValuesFile, changes); err != nil {
+	if err := intyaml.SetStringsInFile(absValuesFile, updates); err != nil {
 		return fmt.Errorf("error updating image references in values file %q: %w", path, err)
 	}
 	return nil
 }
 
-func (y *yamlUpdater) generateCommitMessage(path string, updates map[string]string) string {
+func (y *yamlUpdater) generateCommitMessage(path string, updates []YAMLUpdate) string {
 	if len(updates) == 0 {
 		return ""
 	}
 
 	var commitMsg strings.Builder
 	_, _ = commitMsg.WriteString(fmt.Sprintf("Updated %s\n", path))
-	keys := make([]string, 0, len(updates))
-	for key := range updates {
-		keys = append(keys, key)
-	}
-	slices.Sort(keys)
-	for _, key := range keys {
-		_, _ = commitMsg.WriteString(fmt.Sprintf("\n- %s: %q", key, updates[key]))
+	for _, update := range updates {
+		_, _ = commitMsg.WriteString(fmt.Sprintf("\n- %s: %q", update.Key, update.Value))
 	}
 
 	return commitMsg.String()
diff --git a/internal/directives/yaml_updater_test.go b/internal/directives/yaml_updater_test.go
index f7fef0a64..52c45f43d 100644
--- a/internal/directives/yaml_updater_test.go
+++ b/internal/directives/yaml_updater_test.go
@@ -10,6 +10,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
+	"github.com/akuity/kargo/internal/yaml"
 )
 
 func Test_yamlUpdater_validate(t *testing.T) {
@@ -191,13 +192,13 @@ func Test_yamlUpdater_updateValuesFile(t *testing.T) {
 	tests := []struct {
 		name          string
 		valuesContent string
-		changes       map[string]string
+		updates       []yaml.Update
 		assertions    func(*testing.T, string, error)
 	}{
 		{
 			name:          "successful update",
 			valuesContent: "key: value\n",
-			changes:       map[string]string{"key": "newvalue"},
+			updates:       []yaml.Update{{Key: "key", Value: "newvalue"}},
 			assertions: func(t *testing.T, valuesFilePath string, err error) {
 				require.NoError(t, err)
 
@@ -210,7 +211,7 @@ func Test_yamlUpdater_updateValuesFile(t *testing.T) {
 		{
 			name:          "file does not exist",
 			valuesContent: "",
-			changes:       map[string]string{"key": "value"},
+			updates:       []yaml.Update{{Key: "key", Value: "value"}},
 			assertions: func(t *testing.T, valuesFilePath string, err error) {
 				require.ErrorContains(t, err, "no such file or directory")
 				require.NoFileExists(t, valuesFilePath)
@@ -219,7 +220,7 @@ func Test_yamlUpdater_updateValuesFile(t *testing.T) {
 		{
 			name:          "empty changes",
 			valuesContent: "key: value\n",
-			changes:       map[string]string{},
+			updates:       []yaml.Update{},
 			assertions: func(t *testing.T, valuesFilePath string, err error) {
 				require.NoError(t, err)
 				require.FileExists(t, valuesFilePath)
@@ -242,7 +243,7 @@ func Test_yamlUpdater_updateValuesFile(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			err := runner.updateFile(workDir, path.Base(valuesFile), tt.changes)
+			err := runner.updateFile(workDir, path.Base(valuesFile), tt.updates)
 			tt.assertions(t, valuesFile, err)
 		})
 	}
@@ -252,7 +253,7 @@ func Test_yamlUpdater_generateCommitMessage(t *testing.T) {
 	tests := []struct {
 		name       string
 		path       string
-		changes    map[string]string
+		updates    []YAMLUpdate
 		assertions func(*testing.T, string)
 	}{
 		{
@@ -265,7 +266,7 @@ func Test_yamlUpdater_generateCommitMessage(t *testing.T) {
 		{
 			name:    "single change",
 			path:    "values.yaml",
-			changes: map[string]string{"image": "repo/image:tag1"},
+			updates: []YAMLUpdate{{Key: "image", Value: "repo/image:tag1"}},
 			assertions: func(t *testing.T, result string) {
 				assert.Equal(t, `Updated values.yaml
 
@@ -275,9 +276,9 @@ func Test_yamlUpdater_generateCommitMessage(t *testing.T) {
 		{
 			name: "multiple changes",
 			path: "chart/values.yaml",
-			changes: map[string]string{
-				"image1": "repo1/image1:tag1",
-				"image2": "repo2/image2:tag2",
+			updates: []YAMLUpdate{
+				{Key: "image1", Value: "repo1/image1:tag1"},
+				{Key: "image2", Value: "repo2/image2:tag2"},
 			},
 			assertions: func(t *testing.T, result string) {
 				assert.Equal(t, `Updated chart/values.yaml
@@ -292,7 +293,7 @@ func Test_yamlUpdater_generateCommitMessage(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result := runner.generateCommitMessage(tt.path, tt.changes)
+			result := runner.generateCommitMessage(tt.path, tt.updates)
 			tt.assertions(t, result)
 		})
 	}
diff --git a/internal/yaml/yaml.go b/internal/yaml/yaml.go
index ad3384b93..488cf4849 100644
--- a/internal/yaml/yaml.go
+++ b/internal/yaml/yaml.go
@@ -11,14 +11,21 @@ import (
 	yaml "sigs.k8s.io/yaml/goyaml.v3"
 )
 
+// Update represents a discrete update to be made to a YAML document.
+type Update struct {
+	// Key is the dot-separated path to the field to update.
+	Key string
+	// Value is the new value to set for the field.
+	Value string
+}
+
 // SetStringsInFile overwrites the specified file with the changes specified by
-// the changes map applied. The changes map maps keys to new values. Keys are of
-// the form <key 0>.<key 1>...<key n>. Integers may be used as keys in cases
-// where a specific node needs to be selected from a sequence. An error is
-// returned for any attempted update to a key that does not exist or does not
-// address a scalar node. Importantly, all comments and style choices in the
-// input bytes are preserved in the output.
-func SetStringsInFile(file string, changes map[string]string) error {
+// the the list of Updates. Keys are of the form <key 0>.<key 1>...<key n>.
+// Integers may be used as keys in cases where a specific node needs to be
+// selected from a sequence. An error is returned for any attempted update to a
+// key that does not exist or does not address a scalar node. Importantly, all
+// comments and style choices in the input bytes are preserved in the output.
+func SetStringsInFile(file string, updates []Update) error {
 	inBytes, err := os.ReadFile(file)
 	if err != nil {
 		return fmt.Errorf(
@@ -27,7 +34,7 @@ func SetStringsInFile(file string, changes map[string]string) error {
 			err,
 		)
 	}
-	outBytes, err := SetStringsInBytes(inBytes, changes)
+	outBytes, err := SetStringsInBytes(inBytes, updates)
 	if err != nil {
 		return fmt.Errorf("error mutating bytes: %w", err)
 	}
@@ -47,16 +54,12 @@ func SetStringsInFile(file string, changes map[string]string) error {
 }
 
 // SetStringsInBytes returns a copy of the provided bytes with the changes
-// specified by the changes map applied. The changes map maps keys to new
-// values. Keys are of the form <key 0>.<key 1>...<key n>. Integers may be used
-// as keys in cases where a specific node needs to be selected from a sequence.
-// An error is returned for any attempted update to a key that does not exist or
-// does not address a scalar node. Importantly, all comments and style choices
-// in the input bytes are preserved in the output.
-func SetStringsInBytes(
-	inBytes []byte,
-	changes map[string]string,
-) ([]byte, error) {
+// specified by Updates applied. Keys are of the form <key 0>.<key 1>...<key n>.
+// Integers may be used as keys in cases where a specific node needs to be
+// selected from a sequence. An error is returned for any attempted update to a
+// key that does not exist or does not address a scalar node. Importantly, all
+// comments and style choices in the input bytes are preserved in the output.
+func SetStringsInBytes(inBytes []byte, updates []Update) ([]byte, error) {
 	doc := &yaml.Node{}
 	if err := yaml.Unmarshal(inBytes, doc); err != nil {
 		return nil, fmt.Errorf("error unmarshaling input: %w", err)
@@ -67,15 +70,15 @@ func SetStringsInBytes(
 		value string
 	}
 	changesByLine := map[int]change{}
-	for k, v := range changes {
-		keyPath := strings.Split(k, ".")
+	for _, update := range updates {
+		keyPath := strings.Split(update.Key, ".")
 		line, col, err := findScalarNode(doc, keyPath)
 		if err != nil {
-			return nil, fmt.Errorf("error finding key %s: %w", k, err)
+			return nil, fmt.Errorf("error finding key %s: %w", update.Key, err)
 		}
 		changesByLine[line] = change{
 			col:   col,
-			value: v,
+			value: update.Value,
 		}
 	}
 
diff --git a/internal/yaml/yaml_test.go b/internal/yaml/yaml_test.go
index febb11223..1783f5fdd 100644
--- a/internal/yaml/yaml_test.go
+++ b/internal/yaml/yaml_test.go
@@ -12,7 +12,7 @@ func TestSetStringsInBytes(t *testing.T) {
 	testCases := []struct {
 		name       string
 		inBytes    []byte
-		changes    map[string]string
+		updates    []Update
 		assertions func(*testing.T, []byte, error)
 	}{
 		{
@@ -35,8 +35,11 @@ characters:
 - name: Anakin
   affiliation: Light side
 `),
-			changes: map[string]string{
-				"characters.0.affiliation": "Dark side",
+			updates: []Update{
+				{
+					Key:   "characters.0.affiliation",
+					Value: "Dark side",
+				},
 			},
 			assertions: func(t *testing.T, bytes []byte, err error) {
 				require.NoError(t, err)
@@ -54,7 +57,7 @@ characters:
 	}
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			b, err := SetStringsInBytes(testCase.inBytes, testCase.changes)
+			b, err := SetStringsInBytes(testCase.inBytes, testCase.updates)
 			testCase.assertions(t, b, err)
 		})
 	}

From ad8f7ccf67e0a87a90874457c52fbbd382835bb8 Mon Sep 17 00:00:00 2001
From: Kent Rancourt <kent.rancourt@gmail.com>
Date: Wed, 18 Dec 2024 15:14:33 -0500
Subject: [PATCH 2/3] make syntax supported by yaml pkg consistent with
 tidwall/sjson

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
---
 internal/yaml/decode.go      |   8 +-
 internal/yaml/update.go      |  92 ++++++++++++--
 internal/yaml/update_test.go | 226 +++++++++++++++++++++++++++++++++--
 3 files changed, 299 insertions(+), 27 deletions(-)

diff --git a/internal/yaml/decode.go b/internal/yaml/decode.go
index a2ba3ea1a..daa2d4f16 100644
--- a/internal/yaml/decode.go
+++ b/internal/yaml/decode.go
@@ -2,13 +2,10 @@ package yaml
 
 import (
 	"fmt"
-	"strings"
 
 	yaml "sigs.k8s.io/yaml/goyaml.v3"
 )
 
-const pathSeparator = "."
-
 // FieldNotFoundErr is an error type that is returned when a field is not found
 // in the YAML document.
 type FieldNotFoundErr struct {
@@ -24,7 +21,10 @@ func (e FieldNotFoundErr) Error() string {
 // and decodes it into the provided value. The path is specified using a
 // dot-separated string, similar to the UpdateField function.
 func DecodeField(node *yaml.Node, path string, out any) error {
-	parts := strings.Split(path, pathSeparator)
+	parts, err := splitKey(path)
+	if err != nil {
+		return err
+	}
 	targetNode, err := findNode(node, parts)
 	if err != nil {
 		return err
diff --git a/internal/yaml/update.go b/internal/yaml/update.go
index 6ce34f64a..55150ec99 100644
--- a/internal/yaml/update.go
+++ b/internal/yaml/update.go
@@ -2,6 +2,7 @@ package yaml
 
 import (
 	"fmt"
+	"strconv"
 	"strings"
 
 	yaml "sigs.k8s.io/yaml/goyaml.v3"
@@ -15,7 +16,10 @@ import (
 // the yaml package. This includes basic types (string, int, bool, etc.), maps,
 // slices, and structs.
 func UpdateField(node *yaml.Node, key string, value any) error {
-	parts := strings.Split(key, pathSeparator)
+	parts, err := splitKey(key)
+	if err != nil {
+		return err
+	}
 	return updateNodeRecursively(node, parts, value)
 }
 
@@ -23,10 +27,6 @@ func UpdateField(node *yaml.Node, key string, value any) error {
 // the specified field. It recursively descends into mapping nodes and sequence
 // nodes to find the target field. If the field does not exist, it is created.
 func updateNodeRecursively(node *yaml.Node, parts []string, newValue any) error {
-	if len(parts) == 0 || (len(parts) == 1 && parts[0] == "") {
-		return fmt.Errorf("empty field path")
-	}
-
 	currentPart := parts[0]
 	remainingParts := parts[1:]
 
@@ -100,12 +100,7 @@ func updateSequenceNode(node *yaml.Node, currentPart string, remainingParts []st
 	if err != nil {
 		return err
 	}
-
-	if index < 0 {
-		return fmt.Errorf("invalid negative index: %d", index)
-	}
-
-	if index >= len(node.Content) {
+	if index == -1 || index >= len(node.Content) {
 		// Add new node at the end of the sequence
 		var newNode *yaml.Node
 		if len(remainingParts) > 0 {
@@ -164,12 +159,83 @@ func preserveComments(oldNode, newNode *yaml.Node) {
 	}
 }
 
-// parseIndex extracts and returns the numeric index from a string in the
-// format "[n]".
+// splitKey splits a key string into parts separated by dots. It observes the
+// same basic syntax rules as tidwall/sjson. Dots are separators unless escaped.
+// Colons, unless escaped, are hints that a numeric-looking key part should be
+// treated as a key in an object, rather than an index in a sequence.
+func splitKey(key string) ([]string, error) {
+	key = strings.TrimSpace(key)
+	if key == "" {
+		return nil, fmt.Errorf("empty key")
+	}
+	parts := make([]string, 0, strings.Count(key, ".")+1)
+	currentPart := strings.Builder{}
+	escaped := false
+	for i := 0; i < len(key); i++ {
+		char := key[i]
+		if !escaped {
+			switch char {
+			case '\\':
+				escaped = true // Enter escape mode.
+			case '.':
+				// We've reached the end of the current part.
+				if currentPart.Len() == 0 {
+					return nil, fmt.Errorf("empty key part in key %q", key)
+				}
+				parts = append(parts, currentPart.String())
+				currentPart.Reset()
+			case ':':
+				if currentPart.Len() > 0 {
+					// An unescaped colon is only valid as the first character of a key
+					// part.
+					return nil, fmt.Errorf("unexpected colon in key %q", key)
+				}
+				// We don't actually need to KEEP the colon, since the code that uses
+				// the key parts returned from this function requires no hint that a
+				// numeric-looking key part should be treated as a key in an object,
+				// rather than an index in a sequence.
+			default:
+				// Any other character is added to the current part as is.
+				if err := currentPart.WriteByte(char); err != nil {
+					return nil, err
+				}
+			}
+			continue
+		}
+		// If we get to here, we're currently in escape mode.
+		switch char {
+		case '.', ':':
+			if err := currentPart.WriteByte(char); err != nil {
+				return nil, err
+			}
+			escaped = false // Exit escape mode.
+		default:
+			return nil, fmt.Errorf("invalid escape sequence in key %q", key)
+		}
+	}
+	// Don't forget about whatever is left over in currentPart.
+	if currentPart.Len() == 0 {
+		return nil, fmt.Errorf("empty key part in key %q", key)
+	}
+	return append(parts, currentPart.String()), nil
+}
+
+// parseIndex attempts to parse the provided string as an int. If it is unable
+// to do so, it falls back on parsing according to legacy syntax rules, i.e.
+// extracting n from from a string in the  format "[n]".
 func parseIndex(s string) (int, error) {
+	if index, err := strconv.Atoi(s); err == nil {
+		if index < -1 {
+			return 0, fmt.Errorf("invalid negative index: %d", index)
+		}
+		return index, nil
+	}
 	var index int
 	if _, err := fmt.Sscanf(s, "[%d]", &index); err != nil {
 		return 0, fmt.Errorf("invalid index format: %s", s)
 	}
+	if index < -1 {
+		return 0, fmt.Errorf("invalid negative index: %d", index)
+	}
 	return index, nil
 }
diff --git a/internal/yaml/update_test.go b/internal/yaml/update_test.go
index 80b871f7c..8e7cbe851 100644
--- a/internal/yaml/update_test.go
+++ b/internal/yaml/update_test.go
@@ -39,6 +39,34 @@ func TestUpdateField(t *testing.T) {
 				assert.Equal(t, "new value", node.Content[0].Content[1].Content[1].Value)
 				assert.Equal(t, `root:
     nested: new value
+`, result)
+			},
+		},
+		{
+			name: "update nested field with a number for a key",
+			yaml: `root:
+    0: old value`,
+			path:  "root.0",
+			value: "new value",
+			assertions: func(t *testing.T, node *yaml.Node, result string, err error) {
+				require.NoError(t, err)
+				assert.Equal(t, "new value", node.Content[0].Content[1].Content[1].Value)
+				assert.Equal(t, `root:
+    0: new value
+`, result)
+			},
+		},
+		{
+			name: "update nested field with a number for a key (with colon)",
+			yaml: `root:
+    0: old value`,
+			path:  "root.:0",
+			value: "new value",
+			assertions: func(t *testing.T, node *yaml.Node, result string, err error) {
+				require.NoError(t, err)
+				assert.Equal(t, "new value", node.Content[0].Content[1].Content[1].Value)
+				assert.Equal(t, `root:
+    0: new value
 `, result)
 			},
 		},
@@ -57,7 +85,7 @@ func TestUpdateField(t *testing.T) {
 			},
 		},
 		{
-			name: "add new scalar to end of sequence",
+			name: "add new scalar to end of sequence (legacy syntax)",
 			yaml: `
 root:
   array:
@@ -76,7 +104,26 @@ root:
 			},
 		},
 		{
-			name: "add new mapping to end of sequence",
+			name: "add new scalar to end of sequence",
+			yaml: `
+root:
+  array:
+    - item1
+    - item2`,
+			path:  "root.array.-1",
+			value: "item3",
+			assertions: func(t *testing.T, _ *yaml.Node, result string, err error) {
+				require.NoError(t, err)
+				assert.Equal(t, `root:
+    array:
+        - item1
+        - item2
+        - item3
+`, result)
+			},
+		},
+		{
+			name: "add new mapping to end of sequence (legacy syntax)",
 			yaml: `
 root:
   array:
@@ -95,7 +142,26 @@ root:
 			},
 		},
 		{
-			name: "add new item beyond current length",
+			name: "add new mapping to end of sequence",
+			yaml: `
+root:
+  array:
+    - item1
+    - item2`,
+			path:  "root.array.-1.key",
+			value: "value",
+			assertions: func(t *testing.T, _ *yaml.Node, result string, err error) {
+				require.NoError(t, err)
+				assert.Equal(t, `root:
+    array:
+        - item1
+        - item2
+        - key: value
+`, result)
+			},
+		},
+		{
+			name: "add new item beyond current length (legacy syntax)",
 			yaml: `
 root:
   array:
@@ -112,7 +178,24 @@ root:
 			},
 		},
 		{
-			name: "update existing item in sequence",
+			name: "add new item beyond current length",
+			yaml: `
+root:
+  array:
+    - item1`,
+			path:  "root.array.-1",
+			value: "item2",
+			assertions: func(t *testing.T, _ *yaml.Node, result string, err error) {
+				require.NoError(t, err)
+				assert.Equal(t, `root:
+    array:
+        - item1
+        - item2
+`, result)
+			},
+		},
+		{
+			name: "update existing item in sequence (legacy syntax)",
 			yaml: `
 root:
   array:
@@ -132,7 +215,27 @@ root:
 			},
 		},
 		{
-			name: "add new sequence to end of sequence",
+			name: "update existing item in sequence",
+			yaml: `
+root:
+  array:
+    - item1
+    - item2
+    - item3`,
+			path:  "root.array.1",
+			value: "updated_item",
+			assertions: func(t *testing.T, _ *yaml.Node, result string, err error) {
+				require.NoError(t, err)
+				assert.Equal(t, `root:
+    array:
+        - item1
+        - updated_item
+        - item3
+`, result)
+			},
+		},
+		{
+			name: "add new sequence to end of sequence (legacy syntax)",
 			yaml: `
 root:
   array:
@@ -146,6 +249,24 @@ root:
         - item1
         - - sub1
           - sub2
+`, result)
+			},
+		},
+		{
+			name: "add new sequence to end of sequence",
+			yaml: `
+root:
+  array:
+    - item1`,
+			path:  "root.array.-1",
+			value: []string{"sub1", "sub2"},
+			assertions: func(t *testing.T, _ *yaml.Node, result string, err error) {
+				require.NoError(t, err)
+				assert.Equal(t, `root:
+    array:
+        - item1
+        - - sub1
+          - sub2
 `, result)
 			},
 		},
@@ -175,7 +296,7 @@ root:
 			path:  "",
 			value: "new value",
 			assertions: func(t *testing.T, _ *yaml.Node, _ string, err error) {
-				assert.EqualError(t, err, "empty field path")
+				assert.EqualError(t, err, "empty key")
 			},
 		},
 		{
@@ -186,8 +307,7 @@ root:
 			path:  "root.nested.", // Note the trailing dot, which will result in an empty part
 			value: "new value",
 			assertions: func(t *testing.T, _ *yaml.Node, result string, err error) {
-				require.Error(t, err)
-				assert.Equal(t, "empty field path", err.Error())
+				assert.ErrorContains(t, err, "empty key part in key")
 				assert.Empty(t, result)
 			},
 		},
@@ -201,6 +321,22 @@ root:
 				assert.Empty(t, result)
 			},
 		},
+		{
+			name: "error on negative array index (legacy syntax)",
+			yaml: `
+root:
+  array:
+    - item1
+    - item2
+    - item3`,
+			path:  "root.array.[-2]",
+			value: "new value",
+			assertions: func(t *testing.T, _ *yaml.Node, result string, err error) {
+				require.Error(t, err)
+				assert.Equal(t, "invalid negative index: -2", err.Error())
+				assert.Empty(t, result)
+			},
+		},
 		{
 			name: "error on negative array index",
 			yaml: `
@@ -209,11 +345,11 @@ root:
     - item1
     - item2
     - item3`,
-			path:  "root.array.[-1]",
+			path:  "root.array.-2",
 			value: "new value",
 			assertions: func(t *testing.T, _ *yaml.Node, result string, err error) {
 				require.Error(t, err)
-				assert.Equal(t, "invalid negative index: -1", err.Error())
+				assert.Equal(t, "invalid negative index: -2", err.Error())
 				assert.Empty(t, result)
 			},
 		},
@@ -391,3 +527,73 @@ root:
 		})
 	}
 }
+
+func TestSplitKey(t *testing.T) {
+	testCases := []struct {
+		name        string
+		key         string
+		expected    []string
+		errContains string
+	}{
+		{
+			name:        "empty key",
+			errContains: "empty key",
+		},
+		{
+			name:        "starts with dot",
+			key:         ".foo",
+			errContains: "empty key part in key",
+		},
+		{
+			name:        "ends with dot",
+			key:         "foo.",
+			errContains: "empty key part in key",
+		},
+		{
+			name:        "double dots",
+			key:         "foo..bar",
+			errContains: "empty key part in key",
+		},
+		{
+			name:        "invalid escape sequence",
+			key:         `foo\nbar`,
+			errContains: "invalid escape sequence",
+		},
+		{
+			name:        "invalid use of colon",
+			key:         `foo:bar`,
+			errContains: "unexpected colon in key",
+		},
+		{
+			name:     "basic split",
+			key:      "foo.bar.bat.baz",
+			expected: []string{"foo", "bar", "bat", "baz"},
+		},
+		{
+			name:     "split key with escaped dots",
+			key:      `foo\.bar.bat\.baz`,
+			expected: []string{"foo.bar", "bat.baz"},
+		},
+		{
+			name:     "split key with escaped colon",
+			key:      `foo.bar\:bat.baz`,
+			expected: []string{"foo", "bar:bat", "baz"},
+		},
+		{
+			name:     "split key with unescaped colon",
+			key:      `foo.bar.:2.baz`,
+			expected: []string{"foo", "bar", "2", "baz"},
+		},
+	}
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			parts, err := splitKey(testCase.key)
+			if testCase.errContains != "" {
+				require.ErrorContains(t, err, testCase.errContains)
+			} else {
+				require.NoError(t, err)
+				assert.Equal(t, testCase.expected, parts)
+			}
+		})
+	}
+}

From bed52c4ffbd96d51d7ff585f7440969e6822bd89 Mon Sep 17 00:00:00 2001
From: Kent Rancourt <kent.rancourt@gmail.com>
Date: Wed, 18 Dec 2024 21:57:22 -0500
Subject: [PATCH 3/3] corresponding doc updates

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
---
 docs/docs/35-references/10-promotion-steps.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/docs/35-references/10-promotion-steps.md b/docs/docs/35-references/10-promotion-steps.md
index df9a15b94..593eb47ca 100644
--- a/docs/docs/35-references/10-promotion-steps.md
+++ b/docs/docs/35-references/10-promotion-steps.md
@@ -551,7 +551,7 @@ followed by a [`helm-template`](#helm-template) step.
 |------|------|----------|-------------|
 | `path` | `string` | Y | Path to a YAML file. This path is relative to the temporary workspace that Kargo provisions for use by the promotion process. |
 | `updates` | `[]object` | Y | The details of changes to be applied to the file. At least one must be specified. |
-| `updates[].key` | `string` | Y | The key to update within the file. For nested values, use a YAML dot notation path. |
+| `updates[].key` | `string` | Y | The key to update within the file. For nested values, use dots to delimit key parts. e.g. `image.tag`. The syntax is identical to that supported by the `json-update` step and is documented in more detail [here](https://github.com/tidwall/sjson?tab=readme-ov-file#path-syntax). |
 | `updates[].value` | `string` | Y | The new value for the key. Typically specified using an expression. |
 
 #### `yaml-update` Example