From 63b4b8c778e5963482b50df924a717ae1ea6980d Mon Sep 17 00:00:00 2001 From: Carl Montanari Date: Fri, 5 Aug 2022 12:19:43 -0700 Subject: [PATCH 1/2] fix: fix op patching to not add elements if key already exists --- e2e/tests/config/config.go | 45 +++++++++++ .../devspace.yaml | 16 ++++ pkg/devspace/config/loader/patch/operation.go | 75 +++++++++++++------ 3 files changed, 112 insertions(+), 24 deletions(-) create mode 100644 e2e/tests/config/testdata/patch-add-dont-overwrite-existing-key/devspace.yaml diff --git a/e2e/tests/config/config.go b/e2e/tests/config/config.go index 39706af4bc..0a4c846e0c 100644 --- a/e2e/tests/config/config.go +++ b/e2e/tests/config/config.go @@ -244,6 +244,51 @@ var _ = DevSpaceDescribe("config", func() { framework.ExpectEqual(latestConfig.Deployments["test2"].Name, "test2") }) + ginkgo.It("should not be able to add in patch if key already exists", func() { + tempDir, err := framework.CopyToTempDir("tests/config/testdata/patch-add-dont-overwrite-existing-key") + framework.ExpectNoError(err) + defer framework.CleanupTempDir(initialDir, tempDir) + + configBuffer := &bytes.Buffer{} + printCmd := &cmd.PrintCmd{ + GlobalFlags: &flags.GlobalFlags{ + ConfigPath: "devspace.yaml", + Profiles: []string{"deploy"}, + }, + Out: configBuffer, + SkipInfo: true, + } + + err = printCmd.Run(f) + framework.ExpectError(err) + }) + + ginkgo.It("should be able to add in patch if key does not already exists", func() { + tempDir, err := framework.CopyToTempDir("tests/config/testdata/patch-add-dont-overwrite-existing-key") + framework.ExpectNoError(err) + defer framework.CleanupTempDir(initialDir, tempDir) + + configBuffer := &bytes.Buffer{} + printCmd := &cmd.PrintCmd{ + GlobalFlags: &flags.GlobalFlags{ + ConfigPath: "devspace.yaml", + Profiles: []string{"patch-ok"}, + }, + Out: configBuffer, + SkipInfo: true, + } + + err = printCmd.Run(f) + framework.ExpectNoError(err) + + latestConfig := &latest.Config{} + err = yaml.Unmarshal(configBuffer.Bytes(), latestConfig) + framework.ExpectNoError(err) + + // validate config + framework.ExpectEqual(string(latestConfig.Images["importme"].RebuildStrategy), "ignoreContextChanges") + }) + ginkgo.It("should load profile cached and uncached", func() { tempDir, err := framework.CopyToTempDir("tests/config/testdata/profile") framework.ExpectNoError(err) diff --git a/e2e/tests/config/testdata/patch-add-dont-overwrite-existing-key/devspace.yaml b/e2e/tests/config/testdata/patch-add-dont-overwrite-existing-key/devspace.yaml new file mode 100644 index 0000000000..4d9b601d9e --- /dev/null +++ b/e2e/tests/config/testdata/patch-add-dont-overwrite-existing-key/devspace.yaml @@ -0,0 +1,16 @@ +version: v1beta11 +images: + importme: + image: golang:1.17 + context: ../.. +profiles: + - name: deploy + patches: + - op: add + path: images.importme.context + value: ../.. + - name: patch-ok + patches: + - op: add + path: images.importme.rebuildStrategy + value: ignoreContextChanges \ No newline at end of file diff --git a/pkg/devspace/config/loader/patch/operation.go b/pkg/devspace/config/loader/patch/operation.go index 0cf71ff495..6b927bf52c 100644 --- a/pkg/devspace/config/loader/patch/operation.go +++ b/pkg/devspace/config/loader/patch/operation.go @@ -2,7 +2,6 @@ package patch import ( "fmt" - "github.com/vmware-labs/yaml-jsonpath/pkg/yamlpath" yaml "gopkg.in/yaml.v3" ) @@ -35,38 +34,66 @@ func (op *Operation) Perform(doc *yaml.Node) error { return err } - if len(matches) == 0 { - if op.Op == opAdd { - matches, err = getParents(doc, op.Path) - if err != nil { - return fmt.Errorf("could not add using path: %s", op.Path) + if len(matches) == 0 && op.Op != opAdd { + return fmt.Errorf("%s operation does not apply: doc is missing path: %s", op.Op, op.Path) + } + + var f func(parent *yaml.Node, match *yaml.Node) + + switch op.Op { + case opAdd: + f = op.add + + var pathMatches bool + + if len(matches) > 0 { + pathMatches = true + + if matches[0].Kind == yaml.MappingNode || matches[0].Kind == yaml.SequenceNode { + break } + } - parentPath := op.Path.getParentPath() - propertName := op.Path.getChildName() - if op.Value != nil { - propertyValue := op.Value.Content[0] - op.Value = createMappingNode(propertName, propertyValue) + originalMatches := matches + + matches, err = getParents(doc, op.Path) + if err != nil { + return fmt.Errorf("could not add using path: %s", op.Path) + } + + if len(matches) > 0 && pathMatches { + if matches[0].Kind == yaml.SequenceNode { + matches = originalMatches + break + } else { + // we are trying to overwrite an existing key in a map, don't do that! + return fmt.Errorf( + "attempting add operation for non array/object path '%s' which already exists", + op.Path, + ) } - op.Path = OpPath(parentPath) - } else { - return fmt.Errorf("%s operation does not apply: doc is missing path: %s", op.Op, op.Path) } + + parentPath := op.Path.getParentPath() + propertyName := op.Path.getChildName() + if op.Value != nil { + propertyValue := op.Value.Content[0] + op.Value = createMappingNode(propertyName, propertyValue) + } + op.Path = OpPath(parentPath) + + case opRemove: + f = op.remove + case opReplace: + f = op.replace + default: + return fmt.Errorf("unexpected op: %s", op.Op) } for _, match := range matches { parent := find(doc, containsChild(match)) - switch op.Op { - case opAdd: - op.add(parent, match) - case opRemove: - op.remove(parent, match) - case opReplace: - op.replace(parent, match) - default: - return fmt.Errorf("unexpected op: %s", op.Op) - } + f(parent, match) } return nil From 70af82f517b0e0b117cfa8bbbec6e729c905b0e1 Mon Sep 17 00:00:00 2001 From: Carl Montanari Date: Mon, 8 Aug 2022 15:59:35 -0700 Subject: [PATCH 2/2] refactor: clean up patch operation Perform func --- pkg/devspace/config/loader/patch/operation.go | 29 +++++++++---------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/pkg/devspace/config/loader/patch/operation.go b/pkg/devspace/config/loader/patch/operation.go index 6b927bf52c..0e736a2e9e 100644 --- a/pkg/devspace/config/loader/patch/operation.go +++ b/pkg/devspace/config/loader/patch/operation.go @@ -38,17 +38,14 @@ func (op *Operation) Perform(doc *yaml.Node) error { return fmt.Errorf("%s operation does not apply: doc is missing path: %s", op.Op, op.Path) } - var f func(parent *yaml.Node, match *yaml.Node) + // function that will actually perform the patch operation + var opFunc func(parent *yaml.Node, match *yaml.Node) switch op.Op { case opAdd: - f = op.add - - var pathMatches bool + opFunc = op.add if len(matches) > 0 { - pathMatches = true - if matches[0].Kind == yaml.MappingNode || matches[0].Kind == yaml.SequenceNode { break } @@ -61,17 +58,17 @@ func (op *Operation) Perform(doc *yaml.Node) error { return fmt.Errorf("could not add using path: %s", op.Path) } - if len(matches) > 0 && pathMatches { + if len(matches) > 0 && len(originalMatches) > 0 { if matches[0].Kind == yaml.SequenceNode { matches = originalMatches break - } else { - // we are trying to overwrite an existing key in a map, don't do that! - return fmt.Errorf( - "attempting add operation for non array/object path '%s' which already exists", - op.Path, - ) } + + // we are trying to overwrite an existing key in a map, don't do that! + return fmt.Errorf( + "attempting add operation for non array/object path '%s' which already exists", + op.Path, + ) } parentPath := op.Path.getParentPath() @@ -83,9 +80,9 @@ func (op *Operation) Perform(doc *yaml.Node) error { op.Path = OpPath(parentPath) case opRemove: - f = op.remove + opFunc = op.remove case opReplace: - f = op.replace + opFunc = op.replace default: return fmt.Errorf("unexpected op: %s", op.Op) } @@ -93,7 +90,7 @@ func (op *Operation) Perform(doc *yaml.Node) error { for _, match := range matches { parent := find(doc, containsChild(match)) - f(parent, match) + opFunc(parent, match) } return nil