Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for complex variables #1467

Merged
merged 16 commits into from
Jun 26, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 9 additions & 12 deletions bundle/config/mutator/resolve_resource_references_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func TestResolveClusterReference(t *testing.T) {
},
},
"some-variable": {
Value: &justString,
Value: justString,
},
},
},
Expand All @@ -52,8 +52,8 @@ func TestResolveClusterReference(t *testing.T) {

diags := bundle.Apply(context.Background(), b, ResolveResourceReferences())
require.NoError(t, diags.Error())
require.Equal(t, "1234-5678-abcd", *b.Config.Variables["my-cluster-id-1"].Value)
require.Equal(t, "9876-5432-xywz", *b.Config.Variables["my-cluster-id-2"].Value)
require.Equal(t, "1234-5678-abcd", b.Config.Variables["my-cluster-id-1"].Value)
require.Equal(t, "9876-5432-xywz", b.Config.Variables["my-cluster-id-2"].Value)
}

func TestResolveNonExistentClusterReference(t *testing.T) {
Expand All @@ -68,7 +68,7 @@ func TestResolveNonExistentClusterReference(t *testing.T) {
},
},
"some-variable": {
Value: &justString,
Value: justString,
},
},
},
Expand Down Expand Up @@ -104,7 +104,7 @@ func TestNoLookupIfVariableIsSet(t *testing.T) {

diags := bundle.Apply(context.Background(), b, ResolveResourceReferences())
require.NoError(t, diags.Error())
require.Equal(t, "random value", *b.Config.Variables["my-cluster-id"].Value)
require.Equal(t, "random value", b.Config.Variables["my-cluster-id"].Value)
}

func TestResolveServicePrincipal(t *testing.T) {
Expand All @@ -131,22 +131,19 @@ func TestResolveServicePrincipal(t *testing.T) {

diags := bundle.Apply(context.Background(), b, ResolveResourceReferences())
require.NoError(t, diags.Error())
require.Equal(t, "app-1234", *b.Config.Variables["my-sp"].Value)
require.Equal(t, "app-1234", b.Config.Variables["my-sp"].Value)
}

func TestResolveVariableReferencesInVariableLookups(t *testing.T) {
s := func(s string) *string {
return &s
}

s := "bar"
b := &bundle.Bundle{
Config: config.Root{
Bundle: config.Bundle{
Target: "dev",
},
Variables: map[string]*variable.Variable{
"foo": {
Value: s("bar"),
Value: s,
},
"lookup": {
Lookup: &variable.Lookup{
Expand All @@ -167,7 +164,7 @@ func TestResolveVariableReferencesInVariableLookups(t *testing.T) {
diags := bundle.Apply(context.Background(), b, bundle.Seq(ResolveVariableReferencesInLookup(), ResolveResourceReferences()))
require.NoError(t, diags.Error())
require.Equal(t, "cluster-bar-dev", b.Config.Variables["lookup"].Lookup.Cluster)
require.Equal(t, "1234-5678-abcd", *b.Config.Variables["lookup"].Value)
require.Equal(t, "1234-5678-abcd", b.Config.Variables["lookup"].Value)
}

func TestResolveLookupVariableReferencesInVariableLookups(t *testing.T) {
Expand Down
6 changes: 1 addition & 5 deletions bundle/config/mutator/resolve_variable_references_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,6 @@ func TestResolveVariableReferences(t *testing.T) {
}

func TestResolveVariableReferencesToBundleVariables(t *testing.T) {
s := func(s string) *string {
return &s
}

b := &bundle.Bundle{
Config: config.Root{
Bundle: config.Bundle{
Expand All @@ -57,7 +53,7 @@ func TestResolveVariableReferencesToBundleVariables(t *testing.T) {
},
Variables: map[string]*variable.Variable{
"foo": {
Value: s("bar"),
Value: "bar",
},
},
},
Expand Down
4 changes: 2 additions & 2 deletions bundle/config/mutator/set_variables.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ func setVariable(ctx context.Context, v *variable.Variable, name string) diag.Di

// case: Set the variable to its default value
if v.HasDefault() {
err := v.Set(*v.Default)
err := v.Set(v.Default)
if err != nil {
return diag.Errorf(`failed to assign default value from config "%s" to variable %s with error: %v`, *v.Default, name, err)
return diag.Errorf(`failed to assign default value from config "%s" to variable %s with error: %v`, v.Default, name, err)
}
return nil
}
Expand Down
32 changes: 16 additions & 16 deletions bundle/config/mutator/set_variables_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,52 +15,52 @@ func TestSetVariableFromProcessEnvVar(t *testing.T) {
defaultVal := "default"
variable := variable.Variable{
Description: "a test variable",
Default: &defaultVal,
Default: defaultVal,
}

// set value for variable as an environment variable
t.Setenv("BUNDLE_VAR_foo", "process-env")

diags := setVariable(context.Background(), &variable, "foo")
require.NoError(t, diags.Error())
assert.Equal(t, *variable.Value, "process-env")
assert.Equal(t, variable.Value, "process-env")
}

func TestSetVariableUsingDefaultValue(t *testing.T) {
defaultVal := "default"
variable := variable.Variable{
Description: "a test variable",
Default: &defaultVal,
Default: defaultVal,
}

diags := setVariable(context.Background(), &variable, "foo")
require.NoError(t, diags.Error())
assert.Equal(t, *variable.Value, "default")
assert.Equal(t, variable.Value, "default")
}

func TestSetVariableWhenAlreadyAValueIsAssigned(t *testing.T) {
defaultVal := "default"
val := "assigned-value"
variable := variable.Variable{
Description: "a test variable",
Default: &defaultVal,
Value: &val,
Default: defaultVal,
Value: val,
}

// since a value is already assigned to the variable, it would not be overridden
// by the default value
diags := setVariable(context.Background(), &variable, "foo")
require.NoError(t, diags.Error())
assert.Equal(t, *variable.Value, "assigned-value")
assert.Equal(t, variable.Value, "assigned-value")
}

func TestSetVariableEnvVarValueDoesNotOverridePresetValue(t *testing.T) {
defaultVal := "default"
val := "assigned-value"
variable := variable.Variable{
Description: "a test variable",
Default: &defaultVal,
Value: &val,
Default: defaultVal,
Value: val,
}

// set value for variable as an environment variable
Expand All @@ -70,7 +70,7 @@ func TestSetVariableEnvVarValueDoesNotOverridePresetValue(t *testing.T) {
// by the value from environment
diags := setVariable(context.Background(), &variable, "foo")
require.NoError(t, diags.Error())
assert.Equal(t, *variable.Value, "assigned-value")
assert.Equal(t, variable.Value, "assigned-value")
}

func TestSetVariablesErrorsIfAValueCouldNotBeResolved(t *testing.T) {
Expand All @@ -92,15 +92,15 @@ func TestSetVariablesMutator(t *testing.T) {
Variables: map[string]*variable.Variable{
"a": {
Description: "resolved to default value",
Default: &defaultValForA,
Default: defaultValForA,
},
"b": {
Description: "resolved from environment vairables",
Default: &defaultValForB,
Default: defaultValForB,
},
"c": {
Description: "has already been assigned a value",
Value: &valForC,
Value: valForC,
},
},
},
Expand All @@ -110,7 +110,7 @@ func TestSetVariablesMutator(t *testing.T) {

diags := bundle.Apply(context.Background(), b, SetVariables())
require.NoError(t, diags.Error())
assert.Equal(t, "default-a", *b.Config.Variables["a"].Value)
assert.Equal(t, "env-var-b", *b.Config.Variables["b"].Value)
assert.Equal(t, "assigned-val-c", *b.Config.Variables["c"].Value)
assert.Equal(t, "default-a", b.Config.Variables["a"].Value)
assert.Equal(t, "env-var-b", b.Config.Variables["b"].Value)
assert.Equal(t, "assigned-val-c", b.Config.Variables["c"].Value)
}
8 changes: 4 additions & 4 deletions bundle/config/root_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func TestInitializeVariables(t *testing.T) {
root := &Root{
Variables: map[string]*variable.Variable{
"foo": {
Default: &fooDefault,
Default: fooDefault,
Description: "an optional variable since default is defined",
},
"bar": {
Expand All @@ -62,8 +62,8 @@ func TestInitializeVariables(t *testing.T) {

err := root.InitializeVariables([]string{"foo=123", "bar=456"})
assert.NoError(t, err)
assert.Equal(t, "123", *(root.Variables["foo"].Value))
assert.Equal(t, "456", *(root.Variables["bar"].Value))
assert.Equal(t, "123", (root.Variables["foo"].Value))
assert.Equal(t, "456", (root.Variables["bar"].Value))
}

func TestInitializeVariablesWithAnEqualSignInValue(t *testing.T) {
Expand All @@ -77,7 +77,7 @@ func TestInitializeVariablesWithAnEqualSignInValue(t *testing.T) {

err := root.InitializeVariables([]string{"foo=123=567"})
assert.NoError(t, err)
assert.Equal(t, "123=567", *(root.Variables["foo"].Value))
assert.Equal(t, "123=567", (root.Variables["foo"].Value))
}

func TestInitializeVariablesInvalidFormat(t *testing.T) {
Expand Down
12 changes: 7 additions & 5 deletions bundle/config/variable/variable.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import (
"fmt"
)

type VariableValue = any
andrewnester marked this conversation as resolved.
Show resolved Hide resolved

// An input variable for the bundle config
type Variable struct {
// A default value which then makes the variable optional
Default *string `json:"default,omitempty"`
Default VariableValue `json:"default,omitempty"`

// Documentation for this input variable
Description string `json:"description,omitempty"`
Expand All @@ -21,7 +23,7 @@ type Variable struct {
// 4. Default value defined in variable definition
// 5. Throw error, since if no default value is defined, then the variable
// is required
Value *string `json:"value,omitempty" bundle:"readonly"`
Value VariableValue `json:"value,omitempty" bundle:"readonly"`

// The value of this field will be used to lookup the resource by name
// And assign the value of the variable to ID of the resource found.
Expand All @@ -39,10 +41,10 @@ func (v *Variable) HasValue() bool {
return v.Value != nil
}

func (v *Variable) Set(val string) error {
func (v *Variable) Set(val VariableValue) error {
if v.HasValue() {
return fmt.Errorf("variable has already been assigned value: %s", *v.Value)
return fmt.Errorf("variable has already been assigned value: %s", v.Value)
}
v.Value = &val
v.Value = val
return nil
}
13 changes: 13 additions & 0 deletions bundle/tests/complex_variables_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package config_tests

import (
"testing"

"github.com/stretchr/testify/require"
)

func TestComplexVariables(t *testing.T) {
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
t.Skip("Skipping until complex variables are implemented")
_, diags := loadTargetWithDiags("variables/complex", "default")
require.Empty(t, diags)
}
23 changes: 23 additions & 0 deletions bundle/tests/variables/complex/databricks.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
bundle:
name: complex-variables

resources:
jobs:
my_job:
job_clusters:
- job_cluster_key: key
new_cluster: ${var.cluster}
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
tasks:
- task_key: test
job_cluster_key: key

variables:
cluster:
description: "A cluster definition"
default:
spark_version: "13.2.x-scala2.11"
node_type_id: "Standard_DS3_v2"
num_workers: 2
spark_conf:
spark.speculation: true
spark.databricks.delta.retentionDurationCheck.enabled: false
4 changes: 2 additions & 2 deletions bundle/tests/variables_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ func TestVariablesWithoutDefinition(t *testing.T) {
require.NoError(t, diags.Error())
require.True(t, b.Config.Variables["a"].HasValue())
require.True(t, b.Config.Variables["b"].HasValue())
assert.Equal(t, "foo", *b.Config.Variables["a"].Value)
assert.Equal(t, "bar", *b.Config.Variables["b"].Value)
assert.Equal(t, "foo", b.Config.Variables["a"].Value)
assert.Equal(t, "bar", b.Config.Variables["b"].Value)
}

func TestVariablesWithTargetLookupOverrides(t *testing.T) {
Expand Down
12 changes: 11 additions & 1 deletion libs/dyn/convert/from_typed.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,18 @@ func fromTypedStruct(src reflect.Value, ref dyn.Value) (dyn.Value, error) {
refv = dyn.NilValue
}

var options []fromTypedOptions
if v.Kind() == reflect.Interface {
options = append(options, includeZeroValuedScalars)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to capture a variable value equal to 0, right?

Previously this would have been done through the non-nil pointer and dereferencing that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It otherwise fails to assign dynamic complex value containing zero valued scalars

// Convert the field taking into account the reference value (may be equal to config.NilValue).
nv, err := fromTyped(v.Interface(), refv)
vint := v.Interface()
if vint == nil {
continue
}
andrewnester marked this conversation as resolved.
Show resolved Hide resolved

nv, err := fromTyped(vint, refv, options...)
if err != nil {
return dyn.InvalidValue, err
}
Expand Down
32 changes: 32 additions & 0 deletions libs/dyn/convert/from_typed_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -619,3 +619,35 @@ func TestFromTypedFloatTypeError(t *testing.T) {
_, err := FromTyped(src, ref)
require.Error(t, err)
}

func TestFromTypedAny(t *testing.T) {
type Tmp struct {
Foo any `json:"foo"`
Bar any `json:"bar"`
Foz any `json:"foz"`
Baz any `json:"baz"`
}

src := Tmp{
Foo: "foo",
Bar: false,
Foz: 0,
Baz: map[string]any{
"foo": "foo",
"bar": 1234,
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
},
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
}

ref := dyn.NilValue
nv, err := FromTyped(src, ref)
require.NoError(t, err)
assert.Equal(t, dyn.V(map[string]dyn.Value{
"foo": dyn.V("foo"),
"bar": dyn.V(false),
"foz": dyn.V(int64(0)),
"baz": dyn.V(map[string]dyn.Value{
"foo": dyn.V("foo"),
"bar": dyn.V(int64(1234)),
}),
}), nv)
}
7 changes: 7 additions & 0 deletions libs/dyn/convert/normalize.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ func (n normalizeOptions) normalizeType(typ reflect.Type, src dyn.Value, seen []
return n.normalizeInt(typ, src, path)
case reflect.Float32, reflect.Float64:
return n.normalizeFloat(typ, src, path)
case reflect.Interface:
return n.normalizeInterface(typ, src, path)
}

return dyn.InvalidValue, diag.Errorf("unsupported type: %s", typ.Kind())
Expand Down Expand Up @@ -371,3 +373,8 @@ func (n normalizeOptions) normalizeFloat(typ reflect.Type, src dyn.Value, path d

return dyn.NewValue(out, src.Location()), diags
}

func (n normalizeOptions) normalizeInterface(typ reflect.Type, src dyn.Value, path dyn.Path) (dyn.Value, diag.Diagnostics) {
out := src.AsAny()
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
return dyn.NewValue(out, src.Location()), nil
}
Loading
Loading