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

Resolve variable references inside variable lookup fields #1368

Merged
merged 4 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
61 changes: 61 additions & 0 deletions bundle/config/mutator/resolve_resource_references_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,3 +133,64 @@ func TestResolveServicePrincipal(t *testing.T) {
require.NoError(t, diags.Error())
require.Equal(t, "app-1234", *b.Config.Variables["my-sp"].Value)
}

func TestResolveVariableReferencesInVariableLookups(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}",
},
},
},
},
}

m := mocks.NewMockWorkspaceClient(t)
b.SetWorkpaceClient(m.WorkspaceClient)
clusterApi := m.GetMockClustersAPI()
clusterApi.EXPECT().GetByClusterName(mock.Anything, "cluster-bar-dev").Return(&compute.ClusterDetails{
ClusterId: "1234-5678-abcd",
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
}, nil)

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

func TestResolveLookupVariableReferencesInVariableLookups(t *testing.T) {
b := &bundle.Bundle{
Config: config.Root{
Variables: map[string]*variable.Variable{
"another_lookup": {
Lookup: &variable.Lookup{
Cluster: "cluster",
},
},
"lookup": {
Lookup: &variable.Lookup{
Cluster: "cluster-${var.another_lookup}",
},
},
},
},
}

m := mocks.NewMockWorkspaceClient(t)
b.SetWorkpaceClient(m.WorkspaceClient)

diags := bundle.Apply(context.Background(), b, bundle.Seq(ResolveVariableReferencesInLookup(), ResolveResourceReferences()))
require.ErrorContains(t, diags.Error(), "lookup variables cannot contain references to another lookup variables")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: shouldn't these tests live next to the variable resolver instead of the resource resolver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about this too but decided to leave them here because at the higher level we test that lookup variables are resolved to correct values even if they contain another variable references

90 changes: 65 additions & 25 deletions bundle/config/mutator/resolve_variable_references.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ package mutator

import (
"context"
"fmt"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/config/variable"
"github.com/databricks/cli/libs/diag"
"github.com/databricks/cli/libs/dyn"
"github.com/databricks/cli/libs/dyn/convert"
Expand All @@ -13,10 +15,50 @@ import (

type resolveVariableReferences struct {
prefixes []string
pattern dyn.Pattern
lookupFn func(dyn.Value, dyn.Path) (dyn.Value, error)
}

func ResolveVariableReferences(prefixes ...string) bundle.Mutator {
return &resolveVariableReferences{prefixes: prefixes}
return &resolveVariableReferences{prefixes: prefixes, lookupFn: lookup}
}

func ResolveVariableReferencesInLookup() bundle.Mutator {
return &resolveVariableReferences{prefixes: []string{
"bundle",
"workspace",
"variables",
Copy link
Contributor

Choose a reason for hiding this comment

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

For parity these prefixes should be pushed down into a constructor for the main case as well. Now they are split between the phases code and here.

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 actually use different prefixes in build and deploy phases, so we need to keep prefixes as constructor params

}, pattern: dyn.NewPattern(dyn.Key("variables"), dyn.AnyKey(), dyn.Key("lookup")), lookupFn: lookupForVariables}
}

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 lookupForVariables(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.Lookup != nil && vv.Lookup.String() != "" {
return dyn.InvalidValue, fmt.Errorf("lookup variables cannot contain references to another lookup variables")
}

return lookup(v, path)
}

func (*resolveVariableReferences) Name() string {
Expand Down Expand Up @@ -48,37 +90,35 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle)
//
// This is consistent with the behavior prior to using the dynamic value system.
//
// We can ignore the diagnostics return valuebecause we know that the dynamic value
// We can ignore the diagnostics return value because we know that the dynamic value
// has already been normalized when it was first loaded from the configuration file.
//
normalized, _ := convert.Normalize(b.Config, root, convert.IncludeMissingFields)
lookup := func(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(normalized, path)
}

// Resolve variable references in all values.
root, err := dynvar.Resolve(root, 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(
dyn.Key("variables"),
path[1],
dyn.Key("value"),
)
}

// Perform resolution only if the path starts with one of the specified prefixes.
for _, prefix := range prefixes {
if path.HasPrefix(prefix) {
return lookup(path)
// If the pattern is nil, we resolve references in the entire configuration.
root, err := dyn.MapByPattern(root, m.pattern, func(p dyn.Path, v dyn.Value) (dyn.Value, error) {
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
// 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(
dyn.Key("variables"),
path[1],
dyn.Key("value"),
)
}

// Perform resolution only if the path starts with one of the specified prefixes.
for _, prefix := range prefixes {
if path.HasPrefix(prefix) {
return m.lookupFn(normalized, path)
}
}
}

return dyn.InvalidValue, dynvar.ErrSkipResolution
return dyn.InvalidValue, dynvar.ErrSkipResolution
})
})

if err != nil {
return dyn.InvalidValue, err
}
Expand Down
1 change: 1 addition & 0 deletions bundle/phases/initialize.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ func Initialize() bundle.Mutator {
mutator.ExpandWorkspaceRoot(),
mutator.DefineDefaultWorkspacePaths(),
mutator.SetVariables(),
mutator.ResolveVariableReferencesInLookup(),
mutator.ResolveResourceReferences(),
mutator.ResolveVariableReferences(
"bundle",
Expand Down
Loading