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.

Sign up for GitHub

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

Added support for complex variables #1467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
f8b286b
Change variable struct to use any type instead of string
andrewnester Jun 3, 2024
f8b7d64
addressed feedback
andrewnester Jun 4, 2024
26d1d5a
added support for complex variables
andrewnester Jun 4, 2024
4503657
support overrides and do not allow setting via vars or envs
andrewnester Jun 4, 2024
aff20cf
added tests and fixes
andrewnester Jun 17, 2024
f5d7127
Update libs/dyn/convert/normalize.go
andrewnester Jun 18, 2024
d8f50f2
Merge branch 'main' into feature/complex-variables
andrewnester Jun 19, 2024
f7eba5f
fixes
andrewnester Jun 19, 2024
443b338
Merge branch 'main' into feature/complex-variables
andrewnester Jun 24, 2024
ddd14eb
allow using variables inside complex variables
andrewnester Jun 24, 2024
0c4bf71
addressed feedback
andrewnester Jun 24, 2024
44b17cd
fix test
andrewnester Jun 24, 2024
95c0032
allow nested indexed refs
andrewnester Jun 24, 2024
59dba11
fixed test
andrewnester Jun 25, 2024
327d05e
added test for referencing complex in complex error
andrewnester Jun 25, 2024
2e3c97b
added missing test
andrewnester Jun 26, 2024
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
32 changes: 11 additions & 21 deletions bundle/config/mutator/resolve_resource_references_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func TestResolveClusterReference(t *testing.T) {
},
},
"some-variable": {
Value: &justString,
Value: justString,
},
},
},
Expand All @@ -53,8 +53,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 @@ -69,7 +69,7 @@ func TestResolveNonExistentClusterReference(t *testing.T) {
},
},
"some-variable": {
Value: &justString,
Value: justString,
},
},
},
Expand Down Expand Up @@ -105,7 +105,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 @@ -132,22 +132,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 @@ -168,7 +165,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 Expand Up @@ -197,22 +194,15 @@ func TestResolveLookupVariableReferencesInVariableLookups(t *testing.T) {
}

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

b := &bundle.Bundle{
Config: config.Root{
Bundle: config.Bundle{
Target: "dev",
},
Variables: map[string]*variable.Variable{
"foo": {
Value: s("bar"),
},
"lookup": {
Lookup: &variable.Lookup{
Cluster: "cluster-${var.foo}-${bundle.target}",
Cluster: "cluster-${bundle.target}",
},
},
},
Expand All @@ -227,5 +217,5 @@ func TestNoResolveLookupIfVariableSetWithEnvVariable(t *testing.T) {

diags := bundle.Apply(ctx, b, bundle.Seq(SetVariables(), ResolveVariableReferencesInLookup(), ResolveResourceReferences()))
require.NoError(t, diags.Error())
require.Equal(t, "1234-5678-abcd", *b.Config.Variables["lookup"].Value)
require.Equal(t, "1234-5678-abcd", b.Config.Variables["lookup"].Value)
}
55 changes: 53 additions & 2 deletions bundle/config/mutator/resolve_variable_references.go
View file
Open in desktop
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 @@ -31,13 +32,53 @@ func ResolveVariableReferencesInLookup() bundle.Mutator {
}, pattern: dyn.NewPattern(dyn.Key("variables"), dyn.AnyKey(), dyn.Key("lookup")), lookupFn: lookupForVariables}
}

func ResolveVariableReferencesInComplexVariables() bundle.Mutator {
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
return &resolveVariableReferences{prefixes: []string{
"bundle",
"workspace",
"variables",
},
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) {
// Future opportunity: if we lookup this path in both the given root
// and the synthesized root, we know if it was explicitly set or implied to be empty.
// Then we can emit a warning if it was not explicitly set.
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 @@ -100,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
185 changes: 180 additions & 5 deletions bundle/config/mutator/resolve_variable_references_test.go
View file
Open in desktop
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 Expand Up @@ -195,3 +191,182 @@ 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,
},
Type: variable.VariableTypeComplex,
},
},

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

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,
},
Type: variable.VariableTypeComplex,
},
},

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

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)
}

func TestResolveComplexVariableReferencesWithComplexVariablesError(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,
"spark_conf": "${var.spark_conf}",
},
Type: variable.VariableTypeComplex,
},
"spark_conf": {
Value: map[string]any{
"spark.executor.memory": "4g",
"spark.executor.cores": "2",
},
Type: variable.VariableTypeComplex,
},
},

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

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, bundle.Seq(ResolveVariableReferencesInComplexVariables(), ResolveVariableReferences("bundle", "workspace", "variables")))
require.ErrorContains(t, diags.Error(), "complex variables cannot contain references to another complex variables")
}
Loading
Loading