Skip to content

Commit

Permalink
Fixed full variable override detection (#1787)
Browse files Browse the repository at this point in the history
## Changes
Fixes #1786 

## Tests
All valid override combinations are added as test cases
  • Loading branch information
andrewnester authored Sep 25, 2024
1 parent 3d9decd commit b3a3071
Show file tree
Hide file tree
Showing 2 changed files with 114 additions and 8 deletions.
37 changes: 29 additions & 8 deletions bundle/config/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -418,22 +418,43 @@ func isFullVariableOverrideDef(v dyn.Value) bool {
return false
}

// If the map has more than 2 keys, it is not a full variable override.
if mv.Len() > 2 {
// If the map has more than 3 keys, it is not a full variable override.
if mv.Len() > 3 {
return false
}

// If the map has 2 keys, one of them should be "default" and the other is "type"
// If the map has 3 keys, they should be "description", "type" and "default" or "lookup"
if mv.Len() == 3 {
if _, ok := mv.GetByString("type"); ok {
if _, ok := mv.GetByString("description"); ok {
if _, ok := mv.GetByString("default"); ok {
return true
}
}
}

return false
}

// If the map has 2 keys, one of them should be "default" or "lookup" and the other is "type" or "description"
if mv.Len() == 2 {
if _, ok := mv.GetByString("type"); !ok {
return false
if _, ok := mv.GetByString("type"); ok {
if _, ok := mv.GetByString("default"); ok {
return true
}
}

if _, ok := mv.GetByString("default"); !ok {
return false
if _, ok := mv.GetByString("description"); ok {
if _, ok := mv.GetByString("default"); ok {
return true
}

if _, ok := mv.GetByString("lookup"); ok {
return true
}
}

return true
return false
}

for _, keyword := range variableKeywords {
Expand Down
85 changes: 85 additions & 0 deletions bundle/config/root_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"testing"

"github.com/databricks/cli/bundle/config/variable"
"github.com/databricks/cli/libs/dyn"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -169,3 +170,87 @@ func TestRootMergeTargetOverridesWithVariables(t *testing.T) {
assert.Equal(t, "complex var", root.Variables["complex"].Description)

}

func TestIsFullVariableOverrideDef(t *testing.T) {
testCases := []struct {
value dyn.Value
expected bool
}{
{
value: dyn.V(map[string]dyn.Value{
"type": dyn.V("string"),
"default": dyn.V("foo"),
"description": dyn.V("foo var"),
}),
expected: true,
},
{
value: dyn.V(map[string]dyn.Value{
"type": dyn.V("string"),
"lookup": dyn.V("foo"),
"description": dyn.V("foo var"),
}),
expected: false,
},
{
value: dyn.V(map[string]dyn.Value{
"type": dyn.V("string"),
"default": dyn.V("foo"),
}),
expected: true,
},
{
value: dyn.V(map[string]dyn.Value{
"type": dyn.V("string"),
"lookup": dyn.V("foo"),
}),
expected: false,
},
{
value: dyn.V(map[string]dyn.Value{
"description": dyn.V("string"),
"default": dyn.V("foo"),
}),
expected: true,
},
{
value: dyn.V(map[string]dyn.Value{
"description": dyn.V("string"),
"lookup": dyn.V("foo"),
}),
expected: true,
},
{
value: dyn.V(map[string]dyn.Value{
"default": dyn.V("foo"),
}),
expected: true,
},
{
value: dyn.V(map[string]dyn.Value{
"lookup": dyn.V("foo"),
}),
expected: true,
},
{
value: dyn.V(map[string]dyn.Value{
"type": dyn.V("string"),
}),
expected: false,
},
{
value: dyn.V(map[string]dyn.Value{
"type": dyn.V("string"),
"default": dyn.V("foo"),
"description": dyn.V("foo var"),
"lookup": dyn.V("foo"),
}),
expected: false,
},
}

for i, tc := range testCases {
assert.Equal(t, tc.expected, isFullVariableOverrideDef(tc.value), "test case %d", i)
}

}

0 comments on commit b3a3071

Please sign in to comment.