Skip to content

Commit b3a3071

Browse files
authored
Fixed full variable override detection (#1787)
## Changes Fixes #1786 ## Tests All valid override combinations are added as test cases
1 parent 3d9decd commit b3a3071

File tree

2 files changed

+114
-8
lines changed

2 files changed

+114
-8
lines changed

bundle/config/root.go

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -418,22 +418,43 @@ func isFullVariableOverrideDef(v dyn.Value) bool {
418418
return false
419419
}
420420

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

426-
// If the map has 2 keys, one of them should be "default" and the other is "type"
426+
// If the map has 3 keys, they should be "description", "type" and "default" or "lookup"
427+
if mv.Len() == 3 {
428+
if _, ok := mv.GetByString("type"); ok {
429+
if _, ok := mv.GetByString("description"); ok {
430+
if _, ok := mv.GetByString("default"); ok {
431+
return true
432+
}
433+
}
434+
}
435+
436+
return false
437+
}
438+
439+
// If the map has 2 keys, one of them should be "default" or "lookup" and the other is "type" or "description"
427440
if mv.Len() == 2 {
428-
if _, ok := mv.GetByString("type"); !ok {
429-
return false
441+
if _, ok := mv.GetByString("type"); ok {
442+
if _, ok := mv.GetByString("default"); ok {
443+
return true
444+
}
430445
}
431446

432-
if _, ok := mv.GetByString("default"); !ok {
433-
return false
447+
if _, ok := mv.GetByString("description"); ok {
448+
if _, ok := mv.GetByString("default"); ok {
449+
return true
450+
}
451+
452+
if _, ok := mv.GetByString("lookup"); ok {
453+
return true
454+
}
434455
}
435456

436-
return true
457+
return false
437458
}
438459

439460
for _, keyword := range variableKeywords {

bundle/config/root_test.go

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"testing"
77

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

171172
}
173+
174+
func TestIsFullVariableOverrideDef(t *testing.T) {
175+
testCases := []struct {
176+
value dyn.Value
177+
expected bool
178+
}{
179+
{
180+
value: dyn.V(map[string]dyn.Value{
181+
"type": dyn.V("string"),
182+
"default": dyn.V("foo"),
183+
"description": dyn.V("foo var"),
184+
}),
185+
expected: true,
186+
},
187+
{
188+
value: dyn.V(map[string]dyn.Value{
189+
"type": dyn.V("string"),
190+
"lookup": dyn.V("foo"),
191+
"description": dyn.V("foo var"),
192+
}),
193+
expected: false,
194+
},
195+
{
196+
value: dyn.V(map[string]dyn.Value{
197+
"type": dyn.V("string"),
198+
"default": dyn.V("foo"),
199+
}),
200+
expected: true,
201+
},
202+
{
203+
value: dyn.V(map[string]dyn.Value{
204+
"type": dyn.V("string"),
205+
"lookup": dyn.V("foo"),
206+
}),
207+
expected: false,
208+
},
209+
{
210+
value: dyn.V(map[string]dyn.Value{
211+
"description": dyn.V("string"),
212+
"default": dyn.V("foo"),
213+
}),
214+
expected: true,
215+
},
216+
{
217+
value: dyn.V(map[string]dyn.Value{
218+
"description": dyn.V("string"),
219+
"lookup": dyn.V("foo"),
220+
}),
221+
expected: true,
222+
},
223+
{
224+
value: dyn.V(map[string]dyn.Value{
225+
"default": dyn.V("foo"),
226+
}),
227+
expected: true,
228+
},
229+
{
230+
value: dyn.V(map[string]dyn.Value{
231+
"lookup": dyn.V("foo"),
232+
}),
233+
expected: true,
234+
},
235+
{
236+
value: dyn.V(map[string]dyn.Value{
237+
"type": dyn.V("string"),
238+
}),
239+
expected: false,
240+
},
241+
{
242+
value: dyn.V(map[string]dyn.Value{
243+
"type": dyn.V("string"),
244+
"default": dyn.V("foo"),
245+
"description": dyn.V("foo var"),
246+
"lookup": dyn.V("foo"),
247+
}),
248+
expected: false,
249+
},
250+
}
251+
252+
for i, tc := range testCases {
253+
assert.Equal(t, tc.expected, isFullVariableOverrideDef(tc.value), "test case %d", i)
254+
}
255+
256+
}

0 commit comments

Comments
 (0)