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 5 commits
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
8 changes: 6 additions & 2 deletions bundle/config/mutator/set_variables.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ func setVariable(ctx context.Context, v *variable.Variable, name string) diag.Di
// case: read and set variable value from process environment
envVarName := bundleVarPrefix + name
if val, ok := env.Lookup(ctx, envVarName); ok {
if v.IsComplex() {
return diag.Errorf(`setting via environment variables (%s) is not supported for complex variable %s`, envVarName, name)
}

err := v.Set(val)
if err != nil {
return diag.Errorf(`failed to assign value "%s" to variable %s from environment variable %s with error: %v`, val, name, envVarName, err)
Expand All @@ -39,9 +43,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)
}
22 changes: 21 additions & 1 deletion bundle/config/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,11 @@ func (r *Root) InitializeVariables(vars []string) error {
if _, ok := r.Variables[name]; !ok {
return fmt.Errorf("variable %s has not been defined", name)
}

if r.Variables[name].IsComplex() {
return fmt.Errorf("setting variables of complex type via --var flag is not supported: %s", name)
}

err := r.Variables[name].Set(val)
if err != nil {
return fmt.Errorf("failed to assign %s to %s: %s", val, name, err)
Expand Down Expand Up @@ -415,7 +420,7 @@ func rewriteShorthands(v dyn.Value) (dyn.Value, error) {
}

// For each variable, normalize its contents if it is a single string.
return dyn.Map(target, "variables", dyn.Foreach(func(_ dyn.Path, variable dyn.Value) (dyn.Value, error) {
return dyn.Map(target, "variables", dyn.Foreach(func(p dyn.Path, variable dyn.Value) (dyn.Value, error) {
switch variable.Kind() {

case dyn.KindString, dyn.KindBool, dyn.KindFloat, dyn.KindInt:
Expand All @@ -426,6 +431,21 @@ func rewriteShorthands(v dyn.Value) (dyn.Value, error) {
"default": variable,
}, variable.Location()), nil

case dyn.KindMap, dyn.KindSequence:
// Check if the original definition of variable has a type field.
typeV, err := dyn.GetByPath(v, p.Append(dyn.Key("type")))
if err != nil {
return variable, nil
}

if typeV.MustString() == "complex" {
return dyn.NewValue(map[string]dyn.Value{
"default": variable,
}, variable.Location()), nil
}

return variable, nil

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the PR include test coverage for this, i.e. both ways of setting the variable, as well as defining a top level complex variable and then specifying a different type in the override?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you TAL at this?

default:
return variable, nil
}
Expand Down
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
39 changes: 34 additions & 5 deletions bundle/config/variable/variable.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,27 @@ package variable

import (
"fmt"
"reflect"
)

// We are using `any` because since introduction of complex variables,
// variables can be of any type.
// Type alias is used to make it easier to understand the code.
type VariableValue = any
andrewnester marked this conversation as resolved.
Show resolved Hide resolved

type VariableType string

const (
VariableTypeComplex VariableType = "complex"
)

// An input variable for the bundle config
type Variable struct {
// A type of the variable. This is used to validate the value of the variable
Type VariableType `json:"type,omitempty"`

Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want to constrain the type of primitive types as well?

And otherwise, could we get away with not storing this at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We indeed could get away with not having this but there are few benefits of having this

  1. It makes the intention of using complex variable explicit
  2. It allows for visual editing tools to easily identify that the variable is of complex type and disable editing
  3. It give the ability for future to extend the type with a known types, f.e. job_clusters and etc. I can imagine restricting the primitive types can be beneficial as well

// 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 +36,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 +54,24 @@ 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

rv := reflect.ValueOf(val)
switch rv.Kind() {
case reflect.Struct, reflect.Array, reflect.Slice, reflect.Map:
if v.Type != VariableTypeComplex {
return fmt.Errorf("variable type is not complex")
}
}

v.Value = val

return nil
}

func (v *Variable) IsComplex() bool {
return v.Type == VariableTypeComplex
}
44 changes: 44 additions & 0 deletions bundle/tests/complex_variables_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package config_tests

import (
"context"
"testing"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/config/mutator"
"github.com/stretchr/testify/require"
)

func TestComplexVariables(t *testing.T) {
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
b, diags := loadTargetWithDiags("variables/complex", "default")
require.Empty(t, diags)

diags = bundle.Apply(context.Background(), b, bundle.Seq(
mutator.SetVariables(),
mutator.ResolveVariableReferences(
"variables",
),
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
))
require.NoError(t, diags.Error())

require.Equal(t, "13.2.x-scala2.11", b.Config.Resources.Jobs["my_job"].JobClusters[0].NewCluster.SparkVersion)
require.Equal(t, 2, b.Config.Resources.Jobs["my_job"].JobClusters[0].NewCluster.NumWorkers)
require.Equal(t, "true", b.Config.Resources.Jobs["my_job"].JobClusters[0].NewCluster.SparkConf["spark.speculation"])
}

func TestComplexVariablesOverride(t *testing.T) {
b, diags := loadTargetWithDiags("variables/complex", "dev")
require.Empty(t, diags)

diags = bundle.Apply(context.Background(), b, bundle.Seq(
mutator.SetVariables(),
mutator.ResolveVariableReferences(
"variables",
),
))
require.NoError(t, diags.Error())

require.Equal(t, "14.2.x-scala2.11", b.Config.Resources.Jobs["my_job"].JobClusters[0].NewCluster.SparkVersion)
require.Equal(t, 4, b.Config.Resources.Jobs["my_job"].JobClusters[0].NewCluster.NumWorkers)
require.Equal(t, "false", b.Config.Resources.Jobs["my_job"].JobClusters[0].NewCluster.SparkConf["spark.speculation"])
}
Loading
Loading