From b1d8b590d64087f03122ac7268b879b363410a3c Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Thu, 20 Jun 2024 21:05:39 +0200 Subject: [PATCH 01/11] PythonMutator: support omitempty in PyDABs --- .../config/mutator/python/python_mutator.go | 35 ++++++-- .../mutator/python/python_mutator_test.go | 81 ++++++++++++++++++- libs/dyn/merge/override.go | 22 ++++- libs/dyn/merge/override_test.go | 17 +++- 4 files changed, 143 insertions(+), 12 deletions(-) diff --git a/bundle/config/mutator/python/python_mutator.go b/bundle/config/mutator/python/python_mutator.go index 73ddf95296..fa3d2d3de7 100644 --- a/bundle/config/mutator/python/python_mutator.go +++ b/bundle/config/mutator/python/python_mutator.go @@ -241,8 +241,12 @@ func createLoadOverrideVisitor(ctx context.Context) merge.OverrideVisitor { jobsPath := dyn.NewPath(dyn.Key("resources"), dyn.Key("jobs")) return merge.OverrideVisitor{ - VisitDelete: func(valuePath dyn.Path, left dyn.Value) error { - return fmt.Errorf("unexpected change at %q (delete)", valuePath.String()) + VisitDelete: func(valuePath dyn.Path, left dyn.Value) (dyn.Value, error) { + if isOmitemptyDelete(left) { + return left, nil + } + + return dyn.InvalidValue, fmt.Errorf("unexpected change at %q (delete)", valuePath.String()) }, VisitInsert: func(valuePath dyn.Path, right dyn.Value) (dyn.Value, error) { if !valuePath.HasPrefix(jobsPath) { @@ -274,21 +278,25 @@ func createInitOverrideVisitor(ctx context.Context) merge.OverrideVisitor { jobsPath := dyn.NewPath(dyn.Key("resources"), dyn.Key("jobs")) return merge.OverrideVisitor{ - VisitDelete: func(valuePath dyn.Path, left dyn.Value) error { + VisitDelete: func(valuePath dyn.Path, left dyn.Value) (dyn.Value, error) { + if isOmitemptyDelete(left) { + return left, nil + } + if !valuePath.HasPrefix(jobsPath) { - return fmt.Errorf("unexpected change at %q (delete)", valuePath.String()) + return dyn.InvalidValue, fmt.Errorf("unexpected change at %q (delete)", valuePath.String()) } deleteResource := len(valuePath) == len(jobsPath)+1 if deleteResource { - return fmt.Errorf("unexpected change at %q (delete)", valuePath.String()) + return dyn.InvalidValue, fmt.Errorf("unexpected change at %q (delete)", valuePath.String()) } // deleting properties is allowed because it only changes an existing resource log.Debugf(ctx, "Delete value at %q", valuePath.String()) - return nil + return dyn.NilValue, nil }, VisitInsert: func(valuePath dyn.Path, right dyn.Value) (dyn.Value, error) { if !valuePath.HasPrefix(jobsPath) { @@ -311,6 +319,21 @@ func createInitOverrideVisitor(ctx context.Context) merge.OverrideVisitor { } } +func isOmitemptyDelete(left dyn.Value) bool { + // PyDABs output can omit empty sequences/mappings, because we don't track them as optional, + // there is no semantic difference between empty and missing, so we keep them as they were before. + + if left.Kind() == dyn.KindMap && left.MustMap().Len() == 0 { + return true + } + + if left.Kind() == dyn.KindSequence && len(left.MustSequence()) == 0 { + return true + } + + return false +} + // interpreterPath returns platform-specific path to Python interpreter in the virtual environment. func interpreterPath(venvPath string) string { if runtime.GOOS == "windows" { diff --git a/bundle/config/mutator/python/python_mutator_test.go b/bundle/config/mutator/python/python_mutator_test.go index e2c20386ab..aaad28a197 100644 --- a/bundle/config/mutator/python/python_mutator_test.go +++ b/bundle/config/mutator/python/python_mutator_test.go @@ -384,12 +384,13 @@ func TestCreateOverrideVisitor(t *testing.T) { if tc.deletePath != nil { t.Run(tc.name+"-delete", func(t *testing.T) { - err := visitor.VisitDelete(tc.deletePath, left) + out, err := visitor.VisitDelete(tc.deletePath, left) if tc.deleteError != nil { assert.Equal(t, tc.deleteError, err) } else { assert.NoError(t, err) + assert.Equal(t, dyn.NilValue, out) } }) } @@ -409,6 +410,84 @@ func TestCreateOverrideVisitor(t *testing.T) { } } +type overrideVisitorOmitemptyTestCase struct { + name string + path dyn.Path + left dyn.Value + phases []phase + expected dyn.Value +} + +func TestCreateOverrideVisitor_omitempty(t *testing.T) { + // PyDABs output can omit empty sequences/mappings, because we don't track them as optional, + // there is no semantic difference between empty and missing, so we keep them as they were before. + + allPhases := []phase{PythonMutatorPhaseLoad, PythonMutatorPhaseInit} + location := dyn.Location{ + File: "databricks.yml", + Line: 10, + Column: 20, + } + + testCases := []overrideVisitorOmitemptyTestCase{ + { + // this is not happening, but adding for completeness + name: "empty variables", + path: dyn.MustPathFromString("variables"), + left: dyn.NewValue([]dyn.Value{}, location), + expected: dyn.NewValue([]dyn.Value{}, location), + phases: allPhases, + }, + { + name: "empty job clusters", + path: dyn.MustPathFromString("resources.jobs.job0.job_clusters"), + left: dyn.NewValue([]dyn.Value{}, location), + expected: dyn.NewValue([]dyn.Value{}, location), + phases: allPhases, + }, + { + name: "non-empty job clusters", + path: dyn.MustPathFromString("resources.jobs.job0.job_clusters"), + left: dyn.NewValue([]dyn.Value{dyn.NewValue("abc", location)}, location), + expected: dyn.NilValue, + // deletions aren't allowed in 'load' phase + phases: []phase{PythonMutatorPhaseInit}, + }, + { + name: "empty tags", + path: dyn.MustPathFromString("resources.jobs.job0.tags"), + left: dyn.NewValue(map[string]dyn.Value{}, location), + expected: dyn.NewValue(map[string]dyn.Value{}, location), + phases: allPhases, + }, + { + name: "non-empty tags", + path: dyn.MustPathFromString("resources.jobs.job0.tags"), + left: dyn.NewValue( + map[string]dyn.Value{"dev": dyn.NewValue("true", location)}, + location, + ), + expected: dyn.NilValue, + // deletions aren't allowed in 'load' phase + phases: []phase{PythonMutatorPhaseInit}, + }, + } + + for _, tc := range testCases { + for _, phase := range tc.phases { + t.Run(tc.name+"-"+string(phase), func(t *testing.T) { + visitor, err := createOverrideVisitor(context.Background(), phase) + require.NoError(t, err) + + out, err := visitor.VisitDelete(tc.path, tc.left) + + assert.NoError(t, err) + assert.Equal(t, tc.expected, out) + }) + } + } +} + func TestInterpreterPath(t *testing.T) { if runtime.GOOS == "windows" { assert.Equal(t, "venv\\Scripts\\python3.exe", interpreterPath("venv")) diff --git a/libs/dyn/merge/override.go b/libs/dyn/merge/override.go index 97e8f10098..b84b3c74e4 100644 --- a/libs/dyn/merge/override.go +++ b/libs/dyn/merge/override.go @@ -13,11 +13,17 @@ import ( // For instance, it can disallow changes outside the specific path(s), or update // the location of the effective value. // +// Values returned by 'VisitDelete', 'VisitInsert' and 'VisitUpdate' are used as +// the final value of the node. 'VisitDelete' should return dyn.NilValue to +// do the delete, or can return 'left' to undo it. +// +// TODO 'VisitDelete' and 'VisitInsert' should support dyn.NilValue as well +// // 'VisitDelete' is called when a value is removed from mapping or sequence // 'VisitInsert' is called when a new value is added to mapping or sequence // 'VisitUpdate' is called when a leaf value is updated type OverrideVisitor struct { - VisitDelete func(valuePath dyn.Path, left dyn.Value) error + VisitDelete func(valuePath dyn.Path, left dyn.Value) (dyn.Value, error) VisitInsert func(valuePath dyn.Path, right dyn.Value) (dyn.Value, error) VisitUpdate func(valuePath dyn.Path, left dyn.Value, right dyn.Value) (dyn.Value, error) } @@ -111,11 +117,16 @@ func overrideMapping(basePath dyn.Path, leftMapping dyn.Mapping, rightMapping dy if _, ok := rightMapping.GetPair(leftPair.Key); !ok { path := basePath.Append(dyn.Key(leftPair.Key.MustString())) - err := visitor.VisitDelete(path, leftPair.Value) + deleteOut, err := visitor.VisitDelete(path, leftPair.Value) if err != nil { return dyn.NewMapping(), err } + + // if 'delete' was undone, add it back + if deleteOut != dyn.NilValue { + out.Set(leftPair.Key, deleteOut) + } } } @@ -186,11 +197,16 @@ func overrideSequence(basePath dyn.Path, left []dyn.Value, right []dyn.Value, vi } else if len(left) > len(right) { for i := minLen; i < len(left); i++ { path := basePath.Append(dyn.Index(i)) - err := visitor.VisitDelete(path, left[i]) + out, err := visitor.VisitDelete(path, left[i]) if err != nil { return nil, err } + + // if 'delete' was undone, add it back + if out != dyn.NilValue { + values = append(values, out) + } } } diff --git a/libs/dyn/merge/override_test.go b/libs/dyn/merge/override_test.go index a34f234243..4d39e79a79 100644 --- a/libs/dyn/merge/override_test.go +++ b/libs/dyn/merge/override_test.go @@ -392,6 +392,13 @@ func TestOverride_Primitive(t *testing.T) { assert.NoError(t, err) assert.Equal(t, expected, actual) } + + for _, removed := range s.removed { + actual, err := dyn.GetByPath(out, dyn.MustPathFromString(removed)) + + assert.NoError(t, err) + assert.Equal(t, expected, actual) + } }) } } @@ -467,10 +474,16 @@ func createVisitor(opts visitorOpts) (*visitorState, OverrideVisitor) { return right, nil } }, - VisitDelete: func(valuePath dyn.Path, left dyn.Value) error { + VisitDelete: func(valuePath dyn.Path, left dyn.Value) (dyn.Value, error) { s.removed = append(s.removed, valuePath.String()) - return opts.error + if opts.error != nil { + return dyn.InvalidValue, opts.error + } else if opts.returnValue != nil { + return *opts.returnValue, nil + } else { + return dyn.NilValue, nil + } }, VisitInsert: func(valuePath dyn.Path, right dyn.Value) (dyn.Value, error) { s.added = append(s.added, valuePath.String()) From 7a3e961196e56e8f98a6bbd9c30e75070588f370 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Tue, 2 Jul 2024 11:39:55 +0200 Subject: [PATCH 02/11] Use err to undo deletes --- .../config/mutator/python/python_mutator.go | 16 ++++---- libs/dyn/merge/override.go | 40 ++++++++++--------- libs/dyn/merge/override_test.go | 36 ++++++++++++----- 3 files changed, 54 insertions(+), 38 deletions(-) diff --git a/bundle/config/mutator/python/python_mutator.go b/bundle/config/mutator/python/python_mutator.go index fa3d2d3de7..b519e8989d 100644 --- a/bundle/config/mutator/python/python_mutator.go +++ b/bundle/config/mutator/python/python_mutator.go @@ -241,12 +241,12 @@ func createLoadOverrideVisitor(ctx context.Context) merge.OverrideVisitor { jobsPath := dyn.NewPath(dyn.Key("resources"), dyn.Key("jobs")) return merge.OverrideVisitor{ - VisitDelete: func(valuePath dyn.Path, left dyn.Value) (dyn.Value, error) { + VisitDelete: func(valuePath dyn.Path, left dyn.Value) error { if isOmitemptyDelete(left) { - return left, nil + return merge.ErrOverrideUndoDelete } - return dyn.InvalidValue, fmt.Errorf("unexpected change at %q (delete)", valuePath.String()) + return fmt.Errorf("unexpected change at %q (delete)", valuePath.String()) }, VisitInsert: func(valuePath dyn.Path, right dyn.Value) (dyn.Value, error) { if !valuePath.HasPrefix(jobsPath) { @@ -278,25 +278,25 @@ func createInitOverrideVisitor(ctx context.Context) merge.OverrideVisitor { jobsPath := dyn.NewPath(dyn.Key("resources"), dyn.Key("jobs")) return merge.OverrideVisitor{ - VisitDelete: func(valuePath dyn.Path, left dyn.Value) (dyn.Value, error) { + VisitDelete: func(valuePath dyn.Path, left dyn.Value) error { if isOmitemptyDelete(left) { - return left, nil + return merge.ErrOverrideUndoDelete } if !valuePath.HasPrefix(jobsPath) { - return dyn.InvalidValue, fmt.Errorf("unexpected change at %q (delete)", valuePath.String()) + return fmt.Errorf("unexpected change at %q (delete)", valuePath.String()) } deleteResource := len(valuePath) == len(jobsPath)+1 if deleteResource { - return dyn.InvalidValue, fmt.Errorf("unexpected change at %q (delete)", valuePath.String()) + return fmt.Errorf("unexpected change at %q (delete)", valuePath.String()) } // deleting properties is allowed because it only changes an existing resource log.Debugf(ctx, "Delete value at %q", valuePath.String()) - return dyn.NilValue, nil + return nil }, VisitInsert: func(valuePath dyn.Path, right dyn.Value) (dyn.Value, error) { if !valuePath.HasPrefix(jobsPath) { diff --git a/libs/dyn/merge/override.go b/libs/dyn/merge/override.go index b84b3c74e4..6af0330a51 100644 --- a/libs/dyn/merge/override.go +++ b/libs/dyn/merge/override.go @@ -1,6 +1,7 @@ package merge import ( + "errors" "fmt" "github.com/databricks/cli/libs/dyn" @@ -14,20 +15,20 @@ import ( // the location of the effective value. // // Values returned by 'VisitDelete', 'VisitInsert' and 'VisitUpdate' are used as -// the final value of the node. 'VisitDelete' should return dyn.NilValue to -// do the delete, or can return 'left' to undo it. -// -// TODO 'VisitDelete' and 'VisitInsert' should support dyn.NilValue as well +// the final value of the node. 'VisitDelete' can return ErrOverrideUndoDelete +// to undo delete. // // 'VisitDelete' is called when a value is removed from mapping or sequence // 'VisitInsert' is called when a new value is added to mapping or sequence // 'VisitUpdate' is called when a leaf value is updated type OverrideVisitor struct { - VisitDelete func(valuePath dyn.Path, left dyn.Value) (dyn.Value, error) + VisitDelete func(valuePath dyn.Path, left dyn.Value) error VisitInsert func(valuePath dyn.Path, right dyn.Value) (dyn.Value, error) VisitUpdate func(valuePath dyn.Path, left dyn.Value, right dyn.Value) (dyn.Value, error) } +var ErrOverrideUndoDelete = errors.New("undo delete operation") + // Override overrides value 'leftRoot' with 'rightRoot', keeping 'location' if values // haven't changed. Preserving 'location' is important to preserve the original source of the value // for error reporting. @@ -117,15 +118,18 @@ func overrideMapping(basePath dyn.Path, leftMapping dyn.Mapping, rightMapping dy if _, ok := rightMapping.GetPair(leftPair.Key); !ok { path := basePath.Append(dyn.Key(leftPair.Key.MustString())) - deleteOut, err := visitor.VisitDelete(path, leftPair.Value) - - if err != nil { - return dyn.NewMapping(), err - } + err := visitor.VisitDelete(path, leftPair.Value) // if 'delete' was undone, add it back - if deleteOut != dyn.NilValue { - out.Set(leftPair.Key, deleteOut) + if errors.Is(err, ErrOverrideUndoDelete) { + err = nil + err := out.Set(leftPair.Key, leftPair.Value) + + if err != nil { + return dyn.NewMapping(), err + } + } else if err != nil { + return dyn.NewMapping(), err } } } @@ -197,15 +201,13 @@ func overrideSequence(basePath dyn.Path, left []dyn.Value, right []dyn.Value, vi } else if len(left) > len(right) { for i := minLen; i < len(left); i++ { path := basePath.Append(dyn.Index(i)) - out, err := visitor.VisitDelete(path, left[i]) - - if err != nil { - return nil, err - } + err := visitor.VisitDelete(path, left[i]) // if 'delete' was undone, add it back - if out != dyn.NilValue { - values = append(values, out) + if errors.Is(err, ErrOverrideUndoDelete) { + values = append(values, left[i]) + } else if err != nil { + return nil, err } } } diff --git a/libs/dyn/merge/override_test.go b/libs/dyn/merge/override_test.go index 4d39e79a79..db58882fbe 100644 --- a/libs/dyn/merge/override_test.go +++ b/libs/dyn/merge/override_test.go @@ -5,6 +5,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + "github.com/databricks/cli/libs/dyn" assert "github.com/databricks/cli/libs/dyn/dynassert" ) @@ -392,14 +394,25 @@ func TestOverride_Primitive(t *testing.T) { assert.NoError(t, err) assert.Equal(t, expected, actual) } + }) - for _, removed := range s.removed { - actual, err := dyn.GetByPath(out, dyn.MustPathFromString(removed)) + if len(tc.state.removed) > 0 { + t.Run(tc.name+" - visitor can undo delete", func(t *testing.T) { + s, visitor := createVisitor(visitorOpts{deleteError: ErrOverrideUndoDelete}) + out, err := override(dyn.EmptyPath, tc.left, tc.right, visitor) + require.NoError(t, err) - assert.NoError(t, err) - assert.Equal(t, expected, actual) - } - }) + for _, removed := range s.removed { + expected, err := dyn.GetByPath(tc.left, dyn.MustPathFromString(removed)) + require.NoError(t, err) + + actual, err := dyn.GetByPath(out, dyn.MustPathFromString(removed)) + + assert.NoError(t, err) + assert.Equal(t, expected, actual) + } + }) + } } } } @@ -456,6 +469,7 @@ type visitorState struct { type visitorOpts struct { error error + deleteError error returnValue *dyn.Value } @@ -474,15 +488,15 @@ func createVisitor(opts visitorOpts) (*visitorState, OverrideVisitor) { return right, nil } }, - VisitDelete: func(valuePath dyn.Path, left dyn.Value) (dyn.Value, error) { + VisitDelete: func(valuePath dyn.Path, left dyn.Value) error { s.removed = append(s.removed, valuePath.String()) if opts.error != nil { - return dyn.InvalidValue, opts.error - } else if opts.returnValue != nil { - return *opts.returnValue, nil + return opts.error + } else if opts.deleteError != nil { + return opts.deleteError } else { - return dyn.NilValue, nil + return nil } }, VisitInsert: func(valuePath dyn.Path, right dyn.Value) (dyn.Value, error) { From 370b286edeac10bb2ca9511df19f4020bb3e4a6f Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Tue, 2 Jul 2024 11:56:14 +0200 Subject: [PATCH 03/11] Fix tests --- .../mutator/python/python_mutator_test.go | 61 +++++++++---------- 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/bundle/config/mutator/python/python_mutator_test.go b/bundle/config/mutator/python/python_mutator_test.go index aaad28a197..76e4e3dcee 100644 --- a/bundle/config/mutator/python/python_mutator_test.go +++ b/bundle/config/mutator/python/python_mutator_test.go @@ -3,6 +3,7 @@ package python import ( "context" "fmt" + "github.com/databricks/cli/libs/dyn/merge" "os" "os/exec" "path/filepath" @@ -384,13 +385,12 @@ func TestCreateOverrideVisitor(t *testing.T) { if tc.deletePath != nil { t.Run(tc.name+"-delete", func(t *testing.T) { - out, err := visitor.VisitDelete(tc.deletePath, left) + err := visitor.VisitDelete(tc.deletePath, left) if tc.deleteError != nil { assert.Equal(t, tc.deleteError, err) } else { assert.NoError(t, err) - assert.Equal(t, dyn.NilValue, out) } }) } @@ -411,11 +411,11 @@ func TestCreateOverrideVisitor(t *testing.T) { } type overrideVisitorOmitemptyTestCase struct { - name string - path dyn.Path - left dyn.Value - phases []phase - expected dyn.Value + name string + path dyn.Path + left dyn.Value + phases []phase + expectedErr error } func TestCreateOverrideVisitor_omitempty(t *testing.T) { @@ -432,42 +432,42 @@ func TestCreateOverrideVisitor_omitempty(t *testing.T) { testCases := []overrideVisitorOmitemptyTestCase{ { // this is not happening, but adding for completeness - name: "empty variables", - path: dyn.MustPathFromString("variables"), - left: dyn.NewValue([]dyn.Value{}, location), - expected: dyn.NewValue([]dyn.Value{}, location), - phases: allPhases, + name: "undo delete of empty variables", + path: dyn.MustPathFromString("variables"), + left: dyn.NewValue([]dyn.Value{}, location), + expectedErr: merge.ErrOverrideUndoDelete, + phases: allPhases, }, { - name: "empty job clusters", - path: dyn.MustPathFromString("resources.jobs.job0.job_clusters"), - left: dyn.NewValue([]dyn.Value{}, location), - expected: dyn.NewValue([]dyn.Value{}, location), - phases: allPhases, + name: "undo delete of empty job clusters", + path: dyn.MustPathFromString("resources.jobs.job0.job_clusters"), + left: dyn.NewValue([]dyn.Value{}, location), + expectedErr: merge.ErrOverrideUndoDelete, + phases: allPhases, }, { - name: "non-empty job clusters", - path: dyn.MustPathFromString("resources.jobs.job0.job_clusters"), - left: dyn.NewValue([]dyn.Value{dyn.NewValue("abc", location)}, location), - expected: dyn.NilValue, + name: "allow delete of non-empty job clusters", + path: dyn.MustPathFromString("resources.jobs.job0.job_clusters"), + left: dyn.NewValue([]dyn.Value{dyn.NewValue("abc", location)}, location), + expectedErr: nil, // deletions aren't allowed in 'load' phase phases: []phase{PythonMutatorPhaseInit}, }, { - name: "empty tags", - path: dyn.MustPathFromString("resources.jobs.job0.tags"), - left: dyn.NewValue(map[string]dyn.Value{}, location), - expected: dyn.NewValue(map[string]dyn.Value{}, location), - phases: allPhases, + name: "undo delete of empty tags", + path: dyn.MustPathFromString("resources.jobs.job0.tags"), + left: dyn.NewValue(map[string]dyn.Value{}, location), + expectedErr: merge.ErrOverrideUndoDelete, + phases: allPhases, }, { - name: "non-empty tags", + name: "allow delete of non-empty tags", path: dyn.MustPathFromString("resources.jobs.job0.tags"), left: dyn.NewValue( map[string]dyn.Value{"dev": dyn.NewValue("true", location)}, location, ), - expected: dyn.NilValue, + expectedErr: nil, // deletions aren't allowed in 'load' phase phases: []phase{PythonMutatorPhaseInit}, }, @@ -479,10 +479,9 @@ func TestCreateOverrideVisitor_omitempty(t *testing.T) { visitor, err := createOverrideVisitor(context.Background(), phase) require.NoError(t, err) - out, err := visitor.VisitDelete(tc.path, tc.left) + err = visitor.VisitDelete(tc.path, tc.left) - assert.NoError(t, err) - assert.Equal(t, tc.expected, out) + assert.Equal(t, tc.expectedErr, err) }) } } From 838c116fab419c3f86dd55289571bb6fde657f59 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Tue, 2 Jul 2024 11:56:27 +0200 Subject: [PATCH 04/11] fmt --- bundle/config/mutator/python/python_mutator_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bundle/config/mutator/python/python_mutator_test.go b/bundle/config/mutator/python/python_mutator_test.go index 76e4e3dcee..e53f12e402 100644 --- a/bundle/config/mutator/python/python_mutator_test.go +++ b/bundle/config/mutator/python/python_mutator_test.go @@ -3,7 +3,6 @@ package python import ( "context" "fmt" - "github.com/databricks/cli/libs/dyn/merge" "os" "os/exec" "path/filepath" @@ -11,6 +10,8 @@ import ( "runtime" "testing" + "github.com/databricks/cli/libs/dyn/merge" + "github.com/databricks/cli/bundle/env" "github.com/stretchr/testify/require" From 214577b02c499cc94e910bb19ba0d3d1a3e5988d Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Tue, 2 Jul 2024 11:57:44 +0200 Subject: [PATCH 05/11] Update comment --- libs/dyn/merge/override.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libs/dyn/merge/override.go b/libs/dyn/merge/override.go index 6af0330a51..c4a470ee08 100644 --- a/libs/dyn/merge/override.go +++ b/libs/dyn/merge/override.go @@ -14,9 +14,8 @@ import ( // For instance, it can disallow changes outside the specific path(s), or update // the location of the effective value. // -// Values returned by 'VisitDelete', 'VisitInsert' and 'VisitUpdate' are used as -// the final value of the node. 'VisitDelete' can return ErrOverrideUndoDelete -// to undo delete. +// Values returned by 'VisitInsert' and 'VisitUpdate' are used as the final value +// of the node. 'VisitDelete' can return ErrOverrideUndoDelete to undo delete. // // 'VisitDelete' is called when a value is removed from mapping or sequence // 'VisitInsert' is called when a new value is added to mapping or sequence From f0670291de4e9f68c81e3620cb6dced122ce2c40 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Tue, 2 Jul 2024 11:57:49 +0200 Subject: [PATCH 06/11] fmt --- bundle/config/mutator/python/python_mutator_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/bundle/config/mutator/python/python_mutator_test.go b/bundle/config/mutator/python/python_mutator_test.go index e53f12e402..76e4e3dcee 100644 --- a/bundle/config/mutator/python/python_mutator_test.go +++ b/bundle/config/mutator/python/python_mutator_test.go @@ -3,6 +3,7 @@ package python import ( "context" "fmt" + "github.com/databricks/cli/libs/dyn/merge" "os" "os/exec" "path/filepath" @@ -10,8 +11,6 @@ import ( "runtime" "testing" - "github.com/databricks/cli/libs/dyn/merge" - "github.com/databricks/cli/bundle/env" "github.com/stretchr/testify/require" From d58f374304a107356b0bf0c6b530f29c5fac106f Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Tue, 2 Jul 2024 13:22:44 +0200 Subject: [PATCH 07/11] more fmt --- bundle/config/mutator/python/python_mutator_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bundle/config/mutator/python/python_mutator_test.go b/bundle/config/mutator/python/python_mutator_test.go index 76e4e3dcee..e53f12e402 100644 --- a/bundle/config/mutator/python/python_mutator_test.go +++ b/bundle/config/mutator/python/python_mutator_test.go @@ -3,7 +3,6 @@ package python import ( "context" "fmt" - "github.com/databricks/cli/libs/dyn/merge" "os" "os/exec" "path/filepath" @@ -11,6 +10,8 @@ import ( "runtime" "testing" + "github.com/databricks/cli/libs/dyn/merge" + "github.com/databricks/cli/bundle/env" "github.com/stretchr/testify/require" From 0ffa1b6ced18bf1eec9fec96293b708a246d8e89 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Tue, 2 Jul 2024 13:24:00 +0200 Subject: [PATCH 08/11] Remove redundant assignment --- libs/dyn/merge/override.go | 1 - 1 file changed, 1 deletion(-) diff --git a/libs/dyn/merge/override.go b/libs/dyn/merge/override.go index c4a470ee08..141e65b6aa 100644 --- a/libs/dyn/merge/override.go +++ b/libs/dyn/merge/override.go @@ -121,7 +121,6 @@ func overrideMapping(basePath dyn.Path, leftMapping dyn.Mapping, rightMapping dy // if 'delete' was undone, add it back if errors.Is(err, ErrOverrideUndoDelete) { - err = nil err := out.Set(leftPair.Key, leftPair.Value) if err != nil { From dd4583e33186ad1720f623573e5e4bedb5a2bd62 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Tue, 2 Jul 2024 17:39:30 +0200 Subject: [PATCH 09/11] Address more comments --- bundle/config/mutator/python/python_mutator.go | 10 ++++++++-- bundle/config/mutator/python/python_mutator_test.go | 12 ++++++++++-- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/bundle/config/mutator/python/python_mutator.go b/bundle/config/mutator/python/python_mutator.go index 93142265d4..db3fb1102a 100644 --- a/bundle/config/mutator/python/python_mutator.go +++ b/bundle/config/mutator/python/python_mutator.go @@ -391,8 +391,9 @@ func createInitOverrideVisitor(ctx context.Context) merge.OverrideVisitor { } func isOmitemptyDelete(left dyn.Value) bool { - // PyDABs output can omit empty sequences/mappings, because we don't track them as optional, - // there is no semantic difference between empty and missing, so we keep them as they were before. + // PyDABs can omit empty sequences/mappings in output, because we don't track them as optional, + // there is no semantic difference between empty and missing, so we keep them as they were before + // PyDABs deleted them. if left.Kind() == dyn.KindMap && left.MustMap().Len() == 0 { return true @@ -402,6 +403,11 @@ func isOmitemptyDelete(left dyn.Value) bool { return true } + // map/sequence can be nil, for instance, bad YAML like: `foo:` + if left.Kind() == dyn.KindNil { + return true + } + return false } diff --git a/bundle/config/mutator/python/python_mutator_test.go b/bundle/config/mutator/python/python_mutator_test.go index 621050d0f2..64a2a1a655 100644 --- a/bundle/config/mutator/python/python_mutator_test.go +++ b/bundle/config/mutator/python/python_mutator_test.go @@ -430,8 +430,9 @@ type overrideVisitorOmitemptyTestCase struct { } func TestCreateOverrideVisitor_omitempty(t *testing.T) { - // PyDABs output can omit empty sequences/mappings, because we don't track them as optional, - // there is no semantic difference between empty and missing, so we keep them as they were before. + // PyDABs can omit empty sequences/mappings in output, because we don't track them as optional, + // there is no semantic difference between empty and missing, so we keep them as they were before + // PyDABs deleted them. allPhases := []phase{PythonMutatorPhaseLoad, PythonMutatorPhaseInit} location := dyn.Location{ @@ -482,6 +483,13 @@ func TestCreateOverrideVisitor_omitempty(t *testing.T) { // deletions aren't allowed in 'load' phase phases: []phase{PythonMutatorPhaseInit}, }, + { + name: "undo delete of nil", + path: dyn.MustPathFromString("resources.jobs.job0.tags"), + left: dyn.NilValue.WithLocation(location), + expectedErr: merge.ErrOverrideUndoDelete, + phases: allPhases, + }, } for _, tc := range testCases { From 03994b6a8cd615af08ccd6d89346606a02bf7c72 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Wed, 3 Jul 2024 09:11:30 +0200 Subject: [PATCH 10/11] Update libs/dyn/merge/override.go Co-authored-by: Pieter Noordhuis --- libs/dyn/merge/override.go | 1 - 1 file changed, 1 deletion(-) diff --git a/libs/dyn/merge/override.go b/libs/dyn/merge/override.go index 2285b17ddb..823fb19331 100644 --- a/libs/dyn/merge/override.go +++ b/libs/dyn/merge/override.go @@ -120,7 +120,6 @@ func overrideMapping(basePath dyn.Path, leftMapping dyn.Mapping, rightMapping dy // if 'delete' was undone, add it back if errors.Is(err, ErrOverrideUndoDelete) { err := out.Set(leftPair.Key, leftPair.Value) - if err != nil { return dyn.NewMapping(), err } From e575c97d43a277dd5fc341838e5d4faa69ce2a65 Mon Sep 17 00:00:00 2001 From: Gleb Kanterov Date: Wed, 3 Jul 2024 09:14:24 +0200 Subject: [PATCH 11/11] Use switch --- .../config/mutator/python/python_mutator.go | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/bundle/config/mutator/python/python_mutator.go b/bundle/config/mutator/python/python_mutator.go index db3fb1102a..26b6c54fc0 100644 --- a/bundle/config/mutator/python/python_mutator.go +++ b/bundle/config/mutator/python/python_mutator.go @@ -395,20 +395,20 @@ func isOmitemptyDelete(left dyn.Value) bool { // there is no semantic difference between empty and missing, so we keep them as they were before // PyDABs deleted them. - if left.Kind() == dyn.KindMap && left.MustMap().Len() == 0 { - return true - } + switch left.Kind() { + case dyn.KindMap: + return left.MustMap().Len() == 0 - if left.Kind() == dyn.KindSequence && len(left.MustSequence()) == 0 { - return true - } + case dyn.KindSequence: + return len(left.MustSequence()) == 0 - // map/sequence can be nil, for instance, bad YAML like: `foo:` - if left.Kind() == dyn.KindNil { + case dyn.KindNil: + // map/sequence can be nil, for instance, bad YAML like: `foo:` return true - } - return false + default: + return false + } } // interpreterPath returns platform-specific path to Python interpreter in the virtual environment.