diff --git a/docs/docs/35-references/10-promotion-steps.md b/docs/docs/35-references/10-promotion-steps.md index 2f8a342c5..38fdc2bd3 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 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/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) + } + }) + } +} 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 ..... 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 ..... +// 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 ..... 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 ..... +// 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) }) }