From 542156c30b043c8689a44a20621f53d035d0c97f Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Thu, 18 Apr 2024 11:56:16 +0200 Subject: [PATCH] Resolve variable references inside variable lookup fields (#1368) ## Changes Allows for the syntax below ``` variables: service_principal_app_id: description: 'The app id of the service principal for running workflows as.' lookup: service_principal: "sp-${bundle.environment}" ``` Fixes #1259 ## Tests Added regression test --- .../resolve_resource_references_test.go | 61 +++++++++++++ .../mutator/resolve_variable_references.go | 90 +++++++++++++------ bundle/phases/initialize.go | 1 + 3 files changed, 127 insertions(+), 25 deletions(-) diff --git a/bundle/config/mutator/resolve_resource_references_test.go b/bundle/config/mutator/resolve_resource_references_test.go index 16934ff3..60636bcc 100644 --- a/bundle/config/mutator/resolve_resource_references_test.go +++ b/bundle/config/mutator/resolve_resource_references_test.go @@ -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", + }, 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") +} diff --git a/bundle/config/mutator/resolve_variable_references.go b/bundle/config/mutator/resolve_variable_references.go index 0738c9bc..f7fce6c8 100644 --- a/bundle/config/mutator/resolve_variable_references.go +++ b/bundle/config/mutator/resolve_variable_references.go @@ -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" @@ -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", + }, 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 { @@ -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) { + // 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"), + ) } - } - return dyn.InvalidValue, dynvar.ErrSkipResolution + // 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 + }) }) + if err != nil { return dyn.InvalidValue, err } diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index 6761ffab..d6a1b95d 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -28,6 +28,7 @@ func Initialize() bundle.Mutator { mutator.ExpandWorkspaceRoot(), mutator.DefineDefaultWorkspacePaths(), mutator.SetVariables(), + mutator.ResolveVariableReferencesInLookup(), mutator.ResolveResourceReferences(), mutator.ResolveVariableReferences( "bundle",