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
49 changes: 46 additions & 3 deletions bundle/config/mutator/resolve_variable_references.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type resolveVariableReferences struct {
prefixes []string
pattern dyn.Pattern
lookupFn func(dyn.Value, dyn.Path) (dyn.Value, error)
skipFn func(dyn.Value) bool
}

func ResolveVariableReferences(prefixes ...string) bundle.Mutator {
Expand All @@ -36,7 +37,11 @@ func ResolveVariableReferencesInComplexVariables() bundle.Mutator {
"bundle",
"workspace",
"variables",
}, pattern: dyn.NewPattern(dyn.Key("variables"), dyn.AnyKey(), dyn.Key("value")), lookupFn: lookupForVariables}
},
pattern: dyn.NewPattern(dyn.Key("variables"), dyn.AnyKey(), dyn.Key("value")),
lookupFn: lookupForComplexVariables,
skipFn: skipResolvingInNonComplexVariables,
}
}

func lookup(v dyn.Value, path dyn.Path) (dyn.Value, error) {
Expand All @@ -46,6 +51,34 @@ func lookup(v dyn.Value, path dyn.Path) (dyn.Value, error) {
return dyn.GetByPath(v, path)
}

func lookupForComplexVariables(v dyn.Value, path dyn.Path) (dyn.Value, error) {
if path[0].Key() != "variables" {
return lookup(v, path)
}

varV, err := dyn.GetByPath(v, path[:len(path)-1])
if err != nil {
return dyn.InvalidValue, err
}

var vv variable.Variable
err = convert.ToTyped(&vv, varV)
if err != nil {
return dyn.InvalidValue, err
}

if vv.Type == variable.VariableTypeComplex {
return dyn.InvalidValue, fmt.Errorf("complex variables cannot contain references to another complex variables")
}

return lookup(v, path)
}

func skipResolvingInNonComplexVariables(v dyn.Value) bool {
_, ok := v.AsMap()
return !ok
}

func lookupForVariables(v dyn.Value, path dyn.Path) (dyn.Value, error) {
if path[0].Key() != "variables" {
return lookup(v, path)
Expand Down Expand Up @@ -108,17 +141,27 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle)
// Resolve variable references in all values.
return dynvar.Resolve(v, func(path dyn.Path) (dyn.Value, error) {
// Rewrite the shorthand path ${var.foo} into ${variables.foo.value}.
if path.HasPrefix(varPath) && len(path) == 2 {
path = dyn.NewPath(
if path.HasPrefix(varPath) {
newPath := dyn.NewPath(
dyn.Key("variables"),
path[1],
dyn.Key("value"),
)

if len(path) > 2 {
newPath = newPath.Append(path[2:]...)
}

path = newPath
}

// Perform resolution only if the path starts with one of the specified prefixes.
for _, prefix := range prefixes {
if path.HasPrefix(prefix) {
// Skip resolution if there is a skip function and it returns true.
if m.skipFn != nil && m.skipFn(v) {
return dyn.InvalidValue, dynvar.ErrSkipResolution
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be moved outside of dynvar.Resolve.

return m.lookupFn(normalized, path)
}
}
Expand Down
115 changes: 115 additions & 0 deletions bundle/config/mutator/resolve_variable_references_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,3 +191,118 @@ func TestResolveVariableReferencesForPrimitiveNonStringFields(t *testing.T) {
assert.Equal(t, 2, b.Config.Resources.Jobs["job1"].JobSettings.Tasks[0].NewCluster.Autoscale.MaxWorkers)
assert.Equal(t, 0.5, b.Config.Resources.Jobs["job1"].JobSettings.Tasks[0].NewCluster.AzureAttributes.SpotBidMaxPrice)
}

func TestResolveComplexVariable(t *testing.T) {
b := &bundle.Bundle{
Config: config.Root{
Bundle: config.Bundle{
Name: "example",
},
Variables: map[string]*variable.Variable{
"cluster": {
Value: map[string]any{
"node_type_id": "Standard_DS3_v2",
"num_workers": 2,
},
},
},

Resources: config.Resources{
Jobs: map[string]*resources.Job{
"job1": {
JobSettings: &jobs.JobSettings{
JobClusters: []jobs.JobCluster{
{
NewCluster: compute.ClusterSpec{
NodeTypeId: "random",
},
},
},
},
},
},
},
},
}

ctx := context.Background()

// Assign the variables to the dynamic configuration.
diags := bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) {
var p dyn.Path
var err error

// Set the notification settings.
p = dyn.MustPathFromString("resources.jobs.job1.job_clusters[0]")
v, err = dyn.SetByPath(v, p.Append(dyn.Key("new_cluster")), dyn.V("${var.cluster}"))
require.NoError(t, err)

return v, nil
})
return diag.FromErr(err)
})
require.NoError(t, diags.Error())

diags = bundle.Apply(ctx, b, ResolveVariableReferences("bundle", "workspace", "variables"))
require.NoError(t, diags.Error())
require.Equal(t, "Standard_DS3_v2", b.Config.Resources.Jobs["job1"].JobSettings.JobClusters[0].NewCluster.NodeTypeId)
require.Equal(t, 2, b.Config.Resources.Jobs["job1"].JobSettings.JobClusters[0].NewCluster.NumWorkers)
}

func TestResolveComplexVariableReferencesToFields(t *testing.T) {
b := &bundle.Bundle{
Config: config.Root{
Bundle: config.Bundle{
Name: "example",
},
Variables: map[string]*variable.Variable{
"cluster": {
Value: map[string]any{
"node_type_id": "Standard_DS3_v2",
"num_workers": 2,
},
},
},

Resources: config.Resources{
Jobs: map[string]*resources.Job{
"job1": {
JobSettings: &jobs.JobSettings{
JobClusters: []jobs.JobCluster{
{
NewCluster: compute.ClusterSpec{
NodeTypeId: "random",
},
},
},
},
},
},
},
},
}

ctx := context.Background()

// Assign the variables to the dynamic configuration.
diags := bundle.ApplyFunc(ctx, b, func(ctx context.Context, b *bundle.Bundle) diag.Diagnostics {
err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) {
var p dyn.Path
var err error

// Set the notification settings.
p = dyn.MustPathFromString("resources.jobs.job1.job_clusters[0].new_cluster")
v, err = dyn.SetByPath(v, p.Append(dyn.Key("node_type_id")), dyn.V("${var.cluster.node_type_id}"))
require.NoError(t, err)

return v, nil
})
return diag.FromErr(err)
})
require.NoError(t, diags.Error())

diags = bundle.Apply(ctx, b, ResolveVariableReferences("bundle", "workspace", "variables"))
require.NoError(t, diags.Error())
require.Equal(t, "Standard_DS3_v2", b.Config.Resources.Jobs["job1"].JobSettings.JobClusters[0].NewCluster.NodeTypeId)
}
2 changes: 2 additions & 0 deletions bundle/tests/complex_variables_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ func TestComplexVariables(t *testing.T) {
require.Contains(t, b.Config.Resources.Jobs["my_job"].Tasks[0].Libraries, compute.Library{
Whl: "/path/to/whl",
})

require.Equal(t, "task with spark version 13.2.x-scala2.11 and jar /path/to/jar", b.Config.Resources.Jobs["my_job"].Tasks[0].TaskKey)
}

func TestComplexVariablesOverride(t *testing.T) {
Expand Down
3 changes: 2 additions & 1 deletion bundle/tests/variables/complex/databricks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ resources:
tasks:
- task_key: test
job_cluster_key: key
libraries: ${var.libraries}
libraries: ${variables.libraries.value}
task_key: "task with spark version ${var.cluster.spark_version} and jar ${var.libraries[0].jar}"

variables:
node_type:
Expand Down
4 changes: 0 additions & 4 deletions libs/dyn/convert/from_typed.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,6 @@ func FromTyped(src any, ref dyn.Value) (dyn.Value, error) {
// Private implementation of FromTyped that allows for additional options not exposed
// in the public API.
func fromTyped(src any, ref dyn.Value, options ...fromTypedOptions) (dyn.Value, error) {
if src == nil {
return dyn.NilValue, nil
}

srcv := reflect.ValueOf(src)

// Dereference pointer if necessary
Expand Down
2 changes: 1 addition & 1 deletion libs/dyn/dynvar/ref.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import (
"github.com/databricks/cli/libs/dyn"
)

const VariableRegex = `\$\{([a-zA-Z]+([-_]?[a-zA-Z0-9]+)*(\.[a-zA-Z]+([-_]?[a-zA-Z0-9]+)*)*)\}`
const VariableRegex = `\$\{([a-zA-Z]+([-_]?[a-zA-Z0-9]+)*(\.[a-zA-Z]+([-_]?[a-zA-Z0-9]+)*(\[[0-9]+\])?)*(\[[0-9]+\])?)\}`
andrewnester marked this conversation as resolved.
Show resolved Hide resolved

var re = regexp.MustCompile(VariableRegex)

Expand Down
46 changes: 46 additions & 0 deletions libs/dyn/dynvar/resolve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,3 +247,49 @@ func TestResolveWithInterpolateAliasedRef(t *testing.T) {
assert.Equal(t, "a", getByPath(t, out, "b").MustString())
assert.Equal(t, "a", getByPath(t, out, "c").MustString())
}

func TestResolveIndexedRefs(t *testing.T) {
in := dyn.V(map[string]dyn.Value{
"slice": dyn.V([]dyn.Value{dyn.V("a"), dyn.V("b")}),
"a": dyn.V("a: ${slice[0]}"),
})

out, err := dynvar.Resolve(in, dynvar.DefaultLookup(in))
require.NoError(t, err)

assert.Equal(t, "a: a", getByPath(t, out, "a").MustString())
}

func TestResolveIndexedRefsFromMap(t *testing.T) {
in := dyn.V(map[string]dyn.Value{
"map": dyn.V(
map[string]dyn.Value{
"slice": dyn.V([]dyn.Value{dyn.V("a")}),
}),
"a": dyn.V("a: ${map.slice[0]}"),
})

out, err := dynvar.Resolve(in, dynvar.DefaultLookup(in))
require.NoError(t, err)

assert.Equal(t, "a: a", getByPath(t, out, "a").MustString())
}

func TestResolveMapFieldFromIndexedRefs(t *testing.T) {
in := dyn.V(map[string]dyn.Value{
"map": dyn.V(
map[string]dyn.Value{
"slice": dyn.V([]dyn.Value{
dyn.V(map[string]dyn.Value{
"value": dyn.V("a"),
}),
}),
}),
"a": dyn.V("a: ${map.slice[0].value}"),
})

out, err := dynvar.Resolve(in, dynvar.DefaultLookup(in))
require.NoError(t, err)

assert.Equal(t, "a: a", getByPath(t, out, "a").MustString())
}
4 changes: 2 additions & 2 deletions libs/dyn/path.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ func (c pathComponent) Index() int {
return c.index
}

func (c pathComponent) isKey() bool {
func (c pathComponent) IsKey() bool {
return c.key != ""
}

func (c pathComponent) isIndex() bool {
func (c pathComponent) IsIndex() bool {
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
return c.key == ""
}

Expand Down
4 changes: 2 additions & 2 deletions libs/dyn/visit.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func (component pathComponent) visit(v Value, prefix Path, suffix Pattern, opts
path := append(prefix, component)

switch {
case component.isKey():
case component.IsKey():
// Expect a map to be set if this is a key.
m, ok := v.AsMap()
if !ok {
Expand Down Expand Up @@ -101,7 +101,7 @@ func (component pathComponent) visit(v Value, prefix Path, suffix Pattern, opts
l: v.l,
}, nil

case component.isIndex():
case component.IsIndex():
// Expect a sequence to be set if this is an index.
s, ok := v.AsSequence()
if !ok {
Expand Down
4 changes: 2 additions & 2 deletions libs/dyn/visit_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func SetByPath(v Value, p Path, nv Value) (Value, error) {
path := append(prefix, component)

switch {
case component.isKey():
case component.IsKey():
// Expect a map to be set if this is a key.
m, ok := v.AsMap()
if !ok {
Expand All @@ -48,7 +48,7 @@ func SetByPath(v Value, p Path, nv Value) (Value, error) {
l: v.l,
}, nil

case component.isIndex():
case component.IsIndex():
// Expect a sequence to be set if this is an index.
s, ok := v.AsSequence()
if !ok {
Expand Down
Loading