From 8932d1739341cb97b9a0d2486f0492472201170a Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 22 Jun 2023 18:36:55 +0200 Subject: [PATCH 01/11] Cleanup: remove unnecessary if statements The assert package is already very good at displaying errors, including printing a diff of expected and actual value, so there's no point in printing the same information again ourselves. --- pkg/utils/formatting_test.go | 8 ++------ pkg/utils/rebase_todo_test.go | 12 +++--------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/pkg/utils/formatting_test.go b/pkg/utils/formatting_test.go index 27c1d172c..2de282311 100644 --- a/pkg/utils/formatting_test.go +++ b/pkg/utils/formatting_test.go @@ -79,9 +79,7 @@ func TestGetPadWidths(t *testing.T) { for _, test := range tests { output := getPadWidths(test.input) - if !assert.EqualValues(t, output, test.expected) { - t.Errorf("getPadWidths(%v) = %v, want %v", test.input, output, test.expected) - } + assert.EqualValues(t, output, test.expected) } } @@ -219,8 +217,6 @@ func TestRenderDisplayStrings(t *testing.T) { for _, test := range tests { output := RenderDisplayStrings(test.input, test.columnAlignments) - if !assert.EqualValues(t, output, test.expected) { - t.Errorf("RenderDisplayStrings(%v) = %v, want %v", test.input, output, test.expected) - } + assert.EqualValues(t, output, test.expected) } } diff --git a/pkg/utils/rebase_todo_test.go b/pkg/utils/rebase_todo_test.go index 4f554e926..a9bcb574b 100644 --- a/pkg/utils/rebase_todo_test.go +++ b/pkg/utils/rebase_todo_test.go @@ -321,18 +321,12 @@ func TestRebaseCommands_moveFixupCommitDown(t *testing.T) { actualTodos, actualErr := moveFixupCommitDown(scenario.todos, scenario.originalSha, scenario.fixupSha) if scenario.expectedErr == nil { - if !assert.NoError(t, actualErr) { - t.Errorf("Expected no error, got: %v", actualErr) - } + assert.NoError(t, actualErr) } else { - if !assert.EqualError(t, actualErr, scenario.expectedErr.Error()) { - t.Errorf("Expected err: %v, got: %v", scenario.expectedErr, actualErr) - } + assert.EqualError(t, actualErr, scenario.expectedErr.Error()) } - if !assert.EqualValues(t, actualTodos, scenario.expectedTodos) { - t.Errorf("Expected todos: %v, got: %v", scenario.expectedTodos, actualTodos) - } + assert.EqualValues(t, actualTodos, scenario.expectedTodos) }) } } From bf685cf8326dc6896cb3573fb18e12a6dadb79d1 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 22 Jun 2023 18:32:49 +0200 Subject: [PATCH 02/11] Cleanup: improve test setup and check for the right error string Use the assert package to check expectations; also, check for the exact error message instead of just whether any error occurred. --- pkg/utils/yaml_utils/yaml_utils_test.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/pkg/utils/yaml_utils/yaml_utils_test.go b/pkg/utils/yaml_utils/yaml_utils_test.go index 4bdfeb432..518907cb1 100644 --- a/pkg/utils/yaml_utils/yaml_utils_test.go +++ b/pkg/utils/yaml_utils/yaml_utils_test.go @@ -1,6 +1,10 @@ package yaml_utils -import "testing" +import ( + "testing" + + "github.com/stretchr/testify/assert" +) func TestUpdateYaml(t *testing.T) { tests := []struct { @@ -49,16 +53,14 @@ func TestUpdateYaml(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - out, err := UpdateYaml([]byte(test.in), test.path, test.value) - if test.expectedErr != "" { - if err == nil { - t.Errorf("expected error %q but got none", test.expectedErr) - } - } else if err != nil { - t.Errorf("unexpected error: %v", err) - } else if string(out) != test.expectedOut { - t.Errorf("expected %q but got %q", test.expectedOut, string(out)) + out, actualErr := UpdateYaml([]byte(test.in), test.path, test.value) + if test.expectedErr == "" { + assert.NoError(t, actualErr) + } else { + assert.EqualError(t, actualErr, test.expectedErr) } + + assert.Equal(t, test.expectedOut, string(out)) }) } } From a14794bf5ce856ce97691b5533d403a86033849c Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 8 Jun 2023 08:38:46 +0200 Subject: [PATCH 03/11] Rename UpdateYaml to UpdateYamlValue We are going to add other ways to update yaml documents in the future. --- pkg/utils/yaml_utils/yaml_utils.go | 2 +- pkg/utils/yaml_utils/yaml_utils_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/utils/yaml_utils/yaml_utils.go b/pkg/utils/yaml_utils/yaml_utils.go index 9ed7ae875..2a490f903 100644 --- a/pkg/utils/yaml_utils/yaml_utils.go +++ b/pkg/utils/yaml_utils/yaml_utils.go @@ -7,7 +7,7 @@ import ( ) // takes a yaml document in bytes, a path to a key, and a value to set. The value must be a scalar. -func UpdateYaml(yamlBytes []byte, path []string, value string) ([]byte, error) { +func UpdateYamlValue(yamlBytes []byte, path []string, value string) ([]byte, error) { // Parse the YAML file. var node yaml.Node err := yaml.Unmarshal(yamlBytes, &node) diff --git a/pkg/utils/yaml_utils/yaml_utils_test.go b/pkg/utils/yaml_utils/yaml_utils_test.go index 518907cb1..ff39cbe3f 100644 --- a/pkg/utils/yaml_utils/yaml_utils_test.go +++ b/pkg/utils/yaml_utils/yaml_utils_test.go @@ -6,7 +6,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestUpdateYaml(t *testing.T) { +func TestUpdateYamlValue(t *testing.T) { tests := []struct { name string in string @@ -53,7 +53,7 @@ func TestUpdateYaml(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - out, actualErr := UpdateYaml([]byte(test.in), test.path, test.value) + out, actualErr := UpdateYamlValue([]byte(test.in), test.path, test.value) if test.expectedErr == "" { assert.NoError(t, actualErr) } else { From 90084d115e701e08cbd6a5c88063ce46912428b5 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 8 Jun 2023 08:36:13 +0200 Subject: [PATCH 04/11] Support updating values in empty documents --- pkg/utils/yaml_utils/yaml_utils.go | 7 +++++++ pkg/utils/yaml_utils/yaml_utils_test.go | 8 ++++++++ 2 files changed, 15 insertions(+) diff --git a/pkg/utils/yaml_utils/yaml_utils.go b/pkg/utils/yaml_utils/yaml_utils.go index 2a490f903..cff63871e 100644 --- a/pkg/utils/yaml_utils/yaml_utils.go +++ b/pkg/utils/yaml_utils/yaml_utils.go @@ -15,6 +15,13 @@ func UpdateYamlValue(yamlBytes []byte, path []string, value string) ([]byte, err return nil, fmt.Errorf("failed to parse YAML: %w", err) } + // Empty document: need to create the top-level map ourselves + if len(node.Content) == 0 { + node.Content = append(node.Content, &yaml.Node{ + Kind: yaml.MappingNode, + }) + } + body := node.Content[0] updateYamlNode(body, path, value) diff --git a/pkg/utils/yaml_utils/yaml_utils_test.go b/pkg/utils/yaml_utils/yaml_utils_test.go index ff39cbe3f..fa861b1b8 100644 --- a/pkg/utils/yaml_utils/yaml_utils_test.go +++ b/pkg/utils/yaml_utils/yaml_utils_test.go @@ -31,6 +31,14 @@ func TestUpdateYamlValue(t *testing.T) { expectedOut: "foo: bar\nfoo2: baz\n", expectedErr: "", }, + { + name: "add new key and value when document was empty", + in: "", + path: []string{"foo"}, + value: "bar", + expectedOut: "foo: bar\n", + expectedErr: "", + }, { name: "preserve inline comment", in: "foo: bar # my comment\n", From 221433522d15ed930f54470aeb53099759b9477e Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 8 Jun 2023 08:45:07 +0200 Subject: [PATCH 05/11] Return an error if document is not a dictionary --- pkg/utils/yaml_utils/yaml_utils.go | 5 +++++ pkg/utils/yaml_utils/yaml_utils_test.go | 10 ++++++++++ 2 files changed, 15 insertions(+) diff --git a/pkg/utils/yaml_utils/yaml_utils.go b/pkg/utils/yaml_utils/yaml_utils.go index cff63871e..d93c7f57b 100644 --- a/pkg/utils/yaml_utils/yaml_utils.go +++ b/pkg/utils/yaml_utils/yaml_utils.go @@ -1,6 +1,7 @@ package yaml_utils import ( + "errors" "fmt" "gopkg.in/yaml.v3" @@ -24,6 +25,10 @@ func UpdateYamlValue(yamlBytes []byte, path []string, value string) ([]byte, err body := node.Content[0] + if body.Kind != yaml.MappingNode { + return yamlBytes, errors.New("yaml document is not a dictionary") + } + updateYamlNode(body, path, value) // Convert the updated YAML node back to YAML bytes. diff --git a/pkg/utils/yaml_utils/yaml_utils_test.go b/pkg/utils/yaml_utils/yaml_utils_test.go index fa861b1b8..2e1b2739c 100644 --- a/pkg/utils/yaml_utils/yaml_utils_test.go +++ b/pkg/utils/yaml_utils/yaml_utils_test.go @@ -56,6 +56,16 @@ func TestUpdateYamlValue(t *testing.T) { expectedOut: "foo:\n bar: qux\n", expectedErr: "", }, + + // Error cases + { + name: "existing document is not a dictionary", + in: "42\n", + path: []string{"foo"}, + value: "bar", + expectedOut: "42\n", + expectedErr: "yaml document is not a dictionary", + }, } for _, test := range tests { From 7fb86d6e9c6e0ec9b8468bd251845817c2aeed7b Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 8 Jun 2023 09:00:31 +0200 Subject: [PATCH 06/11] Return an error if node to be updated is not a scalar --- pkg/utils/yaml_utils/yaml_utils.go | 15 ++++++++++----- pkg/utils/yaml_utils/yaml_utils_test.go | 8 ++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/pkg/utils/yaml_utils/yaml_utils.go b/pkg/utils/yaml_utils/yaml_utils.go index d93c7f57b..5957d8d7c 100644 --- a/pkg/utils/yaml_utils/yaml_utils.go +++ b/pkg/utils/yaml_utils/yaml_utils.go @@ -29,7 +29,9 @@ func UpdateYamlValue(yamlBytes []byte, path []string, value string) ([]byte, err return yamlBytes, errors.New("yaml document is not a dictionary") } - updateYamlNode(body, path, value) + if err := updateYamlNode(body, path, value); err != nil { + return yamlBytes, err + } // Convert the updated YAML node back to YAML bytes. updatedYAMLBytes, err := yaml.Marshal(body) @@ -41,17 +43,19 @@ func UpdateYamlValue(yamlBytes []byte, path []string, value string) ([]byte, err } // Recursive function to update the YAML node. -func updateYamlNode(node *yaml.Node, path []string, value string) { +func updateYamlNode(node *yaml.Node, path []string, value string) error { if len(path) == 0 { + if node.Kind != yaml.ScalarNode { + return errors.New("yaml node is not a scalar") + } node.Value = value - return + return nil } key := path[0] for i := 0; i < len(node.Content)-1; i += 2 { if node.Content[i].Value == key { - updateYamlNode(node.Content[i+1], path[1:], value) - return + return updateYamlNode(node.Content[i+1], path[1:], value) } } @@ -63,4 +67,5 @@ func updateYamlNode(node *yaml.Node, path []string, value string) { Kind: yaml.ScalarNode, Value: value, }) + return nil } diff --git a/pkg/utils/yaml_utils/yaml_utils_test.go b/pkg/utils/yaml_utils/yaml_utils_test.go index 2e1b2739c..4e7aaded3 100644 --- a/pkg/utils/yaml_utils/yaml_utils_test.go +++ b/pkg/utils/yaml_utils/yaml_utils_test.go @@ -66,6 +66,14 @@ func TestUpdateYamlValue(t *testing.T) { expectedOut: "42\n", expectedErr: "yaml document is not a dictionary", }, + { + name: "trying to update a note that is not a scalar", + in: "foo: [1, 2, 3]\n", + path: []string{"foo"}, + value: "bar", + expectedOut: "foo: [1, 2, 3]\n", + expectedErr: "yaml node is not a scalar", + }, } for _, test := range tests { From 6acabba417cb4c5a9fb931f8bbeee6b7f553ea16 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 8 Jun 2023 09:09:59 +0200 Subject: [PATCH 07/11] Return an error if some node in the path is not a dictionary --- pkg/utils/yaml_utils/yaml_utils.go | 4 ++++ pkg/utils/yaml_utils/yaml_utils_test.go | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/pkg/utils/yaml_utils/yaml_utils.go b/pkg/utils/yaml_utils/yaml_utils.go index 5957d8d7c..9e2af864f 100644 --- a/pkg/utils/yaml_utils/yaml_utils.go +++ b/pkg/utils/yaml_utils/yaml_utils.go @@ -52,6 +52,10 @@ func updateYamlNode(node *yaml.Node, path []string, value string) error { return nil } + if node.Kind != yaml.MappingNode { + return errors.New("yaml node in path is not a dictionary") + } + key := path[0] for i := 0; i < len(node.Content)-1; i += 2 { if node.Content[i].Value == key { diff --git a/pkg/utils/yaml_utils/yaml_utils_test.go b/pkg/utils/yaml_utils/yaml_utils_test.go index 4e7aaded3..5808cfd38 100644 --- a/pkg/utils/yaml_utils/yaml_utils_test.go +++ b/pkg/utils/yaml_utils/yaml_utils_test.go @@ -74,6 +74,14 @@ func TestUpdateYamlValue(t *testing.T) { expectedOut: "foo: [1, 2, 3]\n", expectedErr: "yaml node is not a scalar", }, + { + name: "not all path elements are dictionaries", + in: "foo:\n bar: [1, 2, 3]\n", + path: []string{"foo", "bar", "baz"}, + value: "qux", + expectedOut: "foo:\n bar: [1, 2, 3]\n", + expectedErr: "yaml node in path is not a dictionary", + }, } for _, test := range tests { From 4461dc68b749b31690501d5d9880a785450e3291 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 8 Jun 2023 09:22:42 +0200 Subject: [PATCH 08/11] Create missing path elements This fixes a serious error: trying to change a value on gui.someOption would add a someOption key at root if gui doesn't exist. --- pkg/utils/yaml_utils/yaml_utils.go | 24 +++++++++++++++++++----- pkg/utils/yaml_utils/yaml_utils_test.go | 8 ++++++++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/pkg/utils/yaml_utils/yaml_utils.go b/pkg/utils/yaml_utils/yaml_utils.go index 9e2af864f..81bc11fca 100644 --- a/pkg/utils/yaml_utils/yaml_utils.go +++ b/pkg/utils/yaml_utils/yaml_utils.go @@ -64,12 +64,26 @@ func updateYamlNode(node *yaml.Node, path []string, value string) error { } // if the key doesn't exist, we'll add it + + // at end of path: add the new key, done + if len(path) == 1 { + node.Content = append(node.Content, &yaml.Node{ + Kind: yaml.ScalarNode, + Value: key, + }, &yaml.Node{ + Kind: yaml.ScalarNode, + Value: value, + }) + return nil + } + + // otherwise, create the missing intermediate node and continue + newNode := &yaml.Node{ + Kind: yaml.MappingNode, + } node.Content = append(node.Content, &yaml.Node{ Kind: yaml.ScalarNode, Value: key, - }, &yaml.Node{ - Kind: yaml.ScalarNode, - Value: value, - }) - return nil + }, newNode) + return updateYamlNode(newNode, path[1:], value) } diff --git a/pkg/utils/yaml_utils/yaml_utils_test.go b/pkg/utils/yaml_utils/yaml_utils_test.go index 5808cfd38..6a544cee5 100644 --- a/pkg/utils/yaml_utils/yaml_utils_test.go +++ b/pkg/utils/yaml_utils/yaml_utils_test.go @@ -56,6 +56,14 @@ func TestUpdateYamlValue(t *testing.T) { expectedOut: "foo:\n bar: qux\n", expectedErr: "", }, + { + name: "nested where parents doesn't exist yet", + in: "", + path: []string{"foo", "bar", "baz"}, + value: "qux", + expectedOut: "foo:\n bar:\n baz: qux\n", + expectedErr: "", + }, // Error cases { From 9cbd7fe69edfd8854735da1cd443420f80173cff Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 8 Jun 2023 09:55:23 +0200 Subject: [PATCH 09/11] Extract a lookupKey function that will be useful in the next commit --- pkg/utils/yaml_utils/yaml_utils.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/pkg/utils/yaml_utils/yaml_utils.go b/pkg/utils/yaml_utils/yaml_utils.go index 81bc11fca..f38a47264 100644 --- a/pkg/utils/yaml_utils/yaml_utils.go +++ b/pkg/utils/yaml_utils/yaml_utils.go @@ -57,10 +57,8 @@ func updateYamlNode(node *yaml.Node, path []string, value string) error { } key := path[0] - for i := 0; i < len(node.Content)-1; i += 2 { - if node.Content[i].Value == key { - return updateYamlNode(node.Content[i+1], path[1:], value) - } + if _, valueNode := lookupKey(node, key); valueNode != nil { + return updateYamlNode(valueNode, path[1:], value) } // if the key doesn't exist, we'll add it @@ -87,3 +85,13 @@ func updateYamlNode(node *yaml.Node, path []string, value string) error { }, newNode) return updateYamlNode(newNode, path[1:], value) } + +func lookupKey(node *yaml.Node, key string) (*yaml.Node, *yaml.Node) { + for i := 0; i < len(node.Content)-1; i += 2 { + if node.Content[i].Value == key { + return node.Content[i], node.Content[i+1] + } + } + + return nil, nil +} From 85f293af1affd8ef879216a4e6bb1be3eaa8c040 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Thu, 8 Jun 2023 12:30:36 +0200 Subject: [PATCH 10/11] Add new function RenameYamlKey --- pkg/utils/yaml_utils/yaml_utils.go | 55 ++++++++++++++++++ pkg/utils/yaml_utils/yaml_utils_test.go | 77 +++++++++++++++++++++++++ 2 files changed, 132 insertions(+) diff --git a/pkg/utils/yaml_utils/yaml_utils.go b/pkg/utils/yaml_utils/yaml_utils.go index f38a47264..4363dedf3 100644 --- a/pkg/utils/yaml_utils/yaml_utils.go +++ b/pkg/utils/yaml_utils/yaml_utils.go @@ -95,3 +95,58 @@ func lookupKey(node *yaml.Node, key string) (*yaml.Node, *yaml.Node) { return nil, nil } + +// takes a yaml document in bytes, a path to a key, and a new name for the key. +// Will rename the key to the new name if it exists, and do nothing otherwise. +func RenameYamlKey(yamlBytes []byte, path []string, newKey string) ([]byte, error) { + // Parse the YAML file. + var node yaml.Node + err := yaml.Unmarshal(yamlBytes, &node) + if err != nil { + return nil, fmt.Errorf("failed to parse YAML: %w", err) + } + + // Empty document: nothing to do. + if len(node.Content) == 0 { + return yamlBytes, nil + } + + body := node.Content[0] + + if err := renameYamlKey(body, path, newKey); err != nil { + return yamlBytes, err + } + + // Convert the updated YAML node back to YAML bytes. + updatedYAMLBytes, err := yaml.Marshal(body) + if err != nil { + return nil, fmt.Errorf("failed to convert YAML node to bytes: %w", err) + } + + return updatedYAMLBytes, nil +} + +// Recursive function to rename the YAML key. +func renameYamlKey(node *yaml.Node, path []string, newKey string) error { + if node.Kind != yaml.MappingNode { + return errors.New("yaml node in path is not a dictionary") + } + + keyNode, valueNode := lookupKey(node, path[0]) + if keyNode == nil { + return nil + } + + // end of path reached: rename key + if len(path) == 1 { + // Check that new key doesn't exist yet + if newKeyNode, _ := lookupKey(node, newKey); newKeyNode != nil { + return fmt.Errorf("new key `%s' already exists", newKey) + } + + keyNode.Value = newKey + return nil + } + + return renameYamlKey(valueNode, path[1:], newKey) +} diff --git a/pkg/utils/yaml_utils/yaml_utils_test.go b/pkg/utils/yaml_utils/yaml_utils_test.go index 6a544cee5..2e29fce75 100644 --- a/pkg/utils/yaml_utils/yaml_utils_test.go +++ b/pkg/utils/yaml_utils/yaml_utils_test.go @@ -106,3 +106,80 @@ func TestUpdateYamlValue(t *testing.T) { }) } } + +func TestRenameYamlKey(t *testing.T) { + tests := []struct { + name string + in string + path []string + newKey string + expectedOut string + expectedErr string + }{ + { + name: "rename key", + in: "foo: 5\n", + path: []string{"foo"}, + newKey: "bar", + expectedOut: "bar: 5\n", + expectedErr: "", + }, + { + name: "rename key, nested", + in: "foo:\n bar: 5\n", + path: []string{"foo", "bar"}, + newKey: "baz", + // indentation is not preserved. See https://github.com/go-yaml/yaml/issues/899 + expectedOut: "foo:\n baz: 5\n", + expectedErr: "", + }, + { + name: "rename non-scalar key", + in: "foo:\n bar: 5\n", + path: []string{"foo"}, + newKey: "qux", + // indentation is not preserved. See https://github.com/go-yaml/yaml/issues/899 + expectedOut: "qux:\n bar: 5\n", + expectedErr: "", + }, + + // Error cases + { + name: "existing document is not a dictionary", + in: "42\n", + path: []string{"foo"}, + newKey: "bar", + expectedOut: "42\n", + expectedErr: "yaml node in path is not a dictionary", + }, + { + name: "not all path elements are dictionaries", + in: "foo:\n bar: [1, 2, 3]\n", + path: []string{"foo", "bar", "baz"}, + newKey: "qux", + expectedOut: "foo:\n bar: [1, 2, 3]\n", + expectedErr: "yaml node in path is not a dictionary", + }, + { + name: "new key exists", + in: "foo: 5\nbar: 7\n", + path: []string{"foo"}, + newKey: "bar", + expectedOut: "foo: 5\nbar: 7\n", + expectedErr: "new key `bar' already exists", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + out, actualErr := RenameYamlKey([]byte(test.in), test.path, test.newKey) + if test.expectedErr == "" { + assert.NoError(t, actualErr) + } else { + assert.EqualError(t, actualErr, test.expectedErr) + } + + assert.Equal(t, test.expectedOut, string(out)) + }) + } +} From c4a2749a99fd15044d002e1202ac0f9c7fc82a34 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 24 Jun 2023 13:26:51 +0200 Subject: [PATCH 11/11] Avoid rewriting the file if nothing changed This avoids changing the indentation or number of blank lines etc unnecessarily if nothing has changed. --- pkg/utils/yaml_utils/yaml_utils.go | 29 ++++++++++++++----------- pkg/utils/yaml_utils/yaml_utils_test.go | 16 ++++++++++++++ 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/pkg/utils/yaml_utils/yaml_utils.go b/pkg/utils/yaml_utils/yaml_utils.go index 4363dedf3..9d96fa7a7 100644 --- a/pkg/utils/yaml_utils/yaml_utils.go +++ b/pkg/utils/yaml_utils/yaml_utils.go @@ -29,7 +29,7 @@ func UpdateYamlValue(yamlBytes []byte, path []string, value string) ([]byte, err return yamlBytes, errors.New("yaml document is not a dictionary") } - if err := updateYamlNode(body, path, value); err != nil { + if didChange, err := updateYamlNode(body, path, value); err != nil || !didChange { return yamlBytes, err } @@ -43,17 +43,20 @@ func UpdateYamlValue(yamlBytes []byte, path []string, value string) ([]byte, err } // Recursive function to update the YAML node. -func updateYamlNode(node *yaml.Node, path []string, value string) error { +func updateYamlNode(node *yaml.Node, path []string, value string) (bool, error) { if len(path) == 0 { if node.Kind != yaml.ScalarNode { - return errors.New("yaml node is not a scalar") + return false, errors.New("yaml node is not a scalar") } - node.Value = value - return nil + if node.Value != value { + node.Value = value + return true, nil + } + return false, nil } if node.Kind != yaml.MappingNode { - return errors.New("yaml node in path is not a dictionary") + return false, errors.New("yaml node in path is not a dictionary") } key := path[0] @@ -72,7 +75,7 @@ func updateYamlNode(node *yaml.Node, path []string, value string) error { Kind: yaml.ScalarNode, Value: value, }) - return nil + return true, nil } // otherwise, create the missing intermediate node and continue @@ -113,7 +116,7 @@ func RenameYamlKey(yamlBytes []byte, path []string, newKey string) ([]byte, erro body := node.Content[0] - if err := renameYamlKey(body, path, newKey); err != nil { + if didRename, err := renameYamlKey(body, path, newKey); err != nil || !didRename { return yamlBytes, err } @@ -127,25 +130,25 @@ func RenameYamlKey(yamlBytes []byte, path []string, newKey string) ([]byte, erro } // Recursive function to rename the YAML key. -func renameYamlKey(node *yaml.Node, path []string, newKey string) error { +func renameYamlKey(node *yaml.Node, path []string, newKey string) (bool, error) { if node.Kind != yaml.MappingNode { - return errors.New("yaml node in path is not a dictionary") + return false, errors.New("yaml node in path is not a dictionary") } keyNode, valueNode := lookupKey(node, path[0]) if keyNode == nil { - return nil + return false, nil } // end of path reached: rename key if len(path) == 1 { // Check that new key doesn't exist yet if newKeyNode, _ := lookupKey(node, newKey); newKeyNode != nil { - return fmt.Errorf("new key `%s' already exists", newKey) + return false, fmt.Errorf("new key `%s' already exists", newKey) } keyNode.Value = newKey - return nil + return true, nil } return renameYamlKey(valueNode, path[1:], newKey) diff --git a/pkg/utils/yaml_utils/yaml_utils_test.go b/pkg/utils/yaml_utils/yaml_utils_test.go index 2e29fce75..7f9dc20f7 100644 --- a/pkg/utils/yaml_utils/yaml_utils_test.go +++ b/pkg/utils/yaml_utils/yaml_utils_test.go @@ -64,6 +64,14 @@ func TestUpdateYamlValue(t *testing.T) { expectedOut: "foo:\n bar:\n baz: qux\n", expectedErr: "", }, + { + name: "don't rewrite file if value didn't change", + in: "foo:\n bar: baz\n", + path: []string{"foo", "bar"}, + value: "baz", + expectedOut: "foo:\n bar: baz\n", + expectedErr: "", + }, // Error cases { @@ -142,6 +150,14 @@ func TestRenameYamlKey(t *testing.T) { expectedOut: "qux:\n bar: 5\n", expectedErr: "", }, + { + name: "don't rewrite file if value didn't change", + in: "foo:\n bar: 5\n", + path: []string{"nonExistingKey"}, + newKey: "qux", + expectedOut: "foo:\n bar: 5\n", + expectedErr: "", + }, // Error cases {