From 6ebfadc19d8f48662f8d7641d141b7416ac759e9 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 16 Apr 2024 14:24:47 +0200 Subject: [PATCH 1/4] Resolve variable references inside variable lookup fields --- .../mutator/resolve_resource_references.go | 33 +++++++++++++++++ .../resolve_resource_references_test.go | 35 +++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/bundle/config/mutator/resolve_resource_references.go b/bundle/config/mutator/resolve_resource_references.go index 89eaa346c6..1329bf0763 100644 --- a/bundle/config/mutator/resolve_resource_references.go +++ b/bundle/config/mutator/resolve_resource_references.go @@ -6,6 +6,9 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" + "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/dyn/convert" + "github.com/databricks/cli/libs/dyn/dynvar" "github.com/databricks/cli/libs/log" "golang.org/x/sync/errgroup" ) @@ -18,6 +21,36 @@ func ResolveResourceReferences() bundle.Mutator { func (m *resolveResourceReferences) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { errs, errCtx := errgroup.WithContext(ctx) + varPath := dyn.NewPath(dyn.Key("var")) + p := dyn.NewPattern( + dyn.Key("variables"), + dyn.AnyKey(), + dyn.Key("lookup"), + ) + + err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { + normalized, _ := convert.Normalize(b.Config, v, convert.IncludeMissingFields) + + // Resolve all variable references in the lookup field. + return dyn.MapByPattern(v, p, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { + 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.GetByPath(normalized, path) + }) + }) + }) + + if err != nil { + return diag.FromErr(err) + } for k := range b.Config.Variables { v := b.Config.Variables[k] diff --git a/bundle/config/mutator/resolve_resource_references_test.go b/bundle/config/mutator/resolve_resource_references_test.go index 16934ff38b..78044e9935 100644 --- a/bundle/config/mutator/resolve_resource_references_test.go +++ b/bundle/config/mutator/resolve_resource_references_test.go @@ -133,3 +133,38 @@ 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{ + Environment: "dev", + }, + Variables: map[string]*variable.Variable{ + "foo": { + Value: s("bar"), + }, + "lookup": { + Lookup: &variable.Lookup{ + Cluster: "cluster-${var.foo}-${bundle.environment}", + }, + }, + }, + }, + } + + 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, ResolveResourceReferences()) + require.NoError(t, diags.Error()) + require.Equal(t, "cluster-bar-dev", b.Config.Variables["lookup"].Lookup.Cluster) +} From 9e6506dce48dcc4150a124bb1d3875392f242ad5 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 16 Apr 2024 16:07:21 +0200 Subject: [PATCH 2/4] fix --- .../mutator/resolve_resource_references.go | 34 ---------- .../resolve_resource_references_test.go | 39 ++++++++++- .../mutator/resolve_variable_references.go | 67 ++++++++++++++----- bundle/phases/initialize.go | 7 ++ 4 files changed, 95 insertions(+), 52 deletions(-) diff --git a/bundle/config/mutator/resolve_resource_references.go b/bundle/config/mutator/resolve_resource_references.go index 1329bf0763..a712b77b11 100644 --- a/bundle/config/mutator/resolve_resource_references.go +++ b/bundle/config/mutator/resolve_resource_references.go @@ -6,9 +6,6 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" - "github.com/databricks/cli/libs/dyn" - "github.com/databricks/cli/libs/dyn/convert" - "github.com/databricks/cli/libs/dyn/dynvar" "github.com/databricks/cli/libs/log" "golang.org/x/sync/errgroup" ) @@ -21,37 +18,6 @@ func ResolveResourceReferences() bundle.Mutator { func (m *resolveResourceReferences) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { errs, errCtx := errgroup.WithContext(ctx) - varPath := dyn.NewPath(dyn.Key("var")) - p := dyn.NewPattern( - dyn.Key("variables"), - dyn.AnyKey(), - dyn.Key("lookup"), - ) - - err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) { - normalized, _ := convert.Normalize(b.Config, v, convert.IncludeMissingFields) - - // Resolve all variable references in the lookup field. - return dyn.MapByPattern(v, p, func(p dyn.Path, v dyn.Value) (dyn.Value, error) { - 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.GetByPath(normalized, path) - }) - }) - }) - - if err != nil { - return diag.FromErr(err) - } - for k := range b.Config.Variables { v := b.Config.Variables[k] if v == nil || v.Lookup == nil { diff --git a/bundle/config/mutator/resolve_resource_references_test.go b/bundle/config/mutator/resolve_resource_references_test.go index 78044e9935..70ca030377 100644 --- a/bundle/config/mutator/resolve_resource_references_test.go +++ b/bundle/config/mutator/resolve_resource_references_test.go @@ -8,6 +8,7 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/variable" + "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -164,7 +165,43 @@ func TestResolveVariableReferencesInVariableLookups(t *testing.T) { ClusterId: "1234-5678-abcd", }, nil) - diags := bundle.Apply(context.Background(), b, ResolveResourceReferences()) + diags := bundle.Apply(context.Background(), b, bundle.Seq(ResolveVariableReferencesFor( + dyn.NewPattern(dyn.Key("variables"), dyn.AnyKey(), dyn.Key("lookup")), + "bundle", + "variables", + ), ResolveResourceReferences())) require.NoError(t, diags.Error()) require.Equal(t, "cluster-bar-dev", b.Config.Variables["lookup"].Lookup.Cluster) } + +func TestResolveLookupVariableReferencesInVariableLookups(t *testing.T) { + b := &bundle.Bundle{ + Config: config.Root{ + Bundle: config.Bundle{ + Environment: "dev", + }, + 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(ResolveVariableReferencesFor( + dyn.NewPattern(dyn.Key("variables"), dyn.AnyKey(), dyn.Key("lookup")), + "bundle", + "variables", + ), 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 0738c9bcb5..b86b23fb7a 100644 --- a/bundle/config/mutator/resolve_variable_references.go +++ b/bundle/config/mutator/resolve_variable_references.go @@ -2,6 +2,7 @@ package mutator import ( "context" + "fmt" "github.com/databricks/cli/bundle" "github.com/databricks/cli/libs/diag" @@ -13,12 +14,17 @@ import ( type resolveVariableReferences struct { prefixes []string + pattern dyn.Pattern } func ResolveVariableReferences(prefixes ...string) bundle.Mutator { return &resolveVariableReferences{prefixes: prefixes} } +func ResolveVariableReferencesFor(p dyn.Pattern, prefixes ...string) bundle.Mutator { + return &resolveVariableReferences{prefixes: prefixes, pattern: p} +} + func (*resolveVariableReferences) Name() string { return "ResolveVariableReferences" } @@ -56,29 +62,56 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle) // 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) - } + v, err := dyn.GetByPath(normalized, path) + if err != nil { + return dyn.InvalidValue, err + } - // 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"), - ) + // Check if the variable we're looking up has a lookup field in it + lookupV, err := dyn.GetByPath(normalized, path[:len(path)-1].Append(dyn.Key("lookup"))) + + // If nothing is found, it means the variable is not a lookup variable, so we can just return the value + if err != nil || lookupV == dyn.InvalidValue { + return v, nil } - // Perform resolution only if the path starts with one of the specified prefixes. - for _, prefix := range prefixes { - if path.HasPrefix(prefix) { - return lookup(path) - } + // Check that the lookup variable was resolved to non-empty string already + sv, ok := v.AsString() + if !ok { + return v, nil + } + + // If the lookup variable is empty, it means it was not resolved yet which means it is used inside another lookup variable + if sv == "" { + return dyn.InvalidValue, fmt.Errorf("lookup variables cannot contain references to another lookup variables") } - return dyn.InvalidValue, dynvar.ErrSkipResolution + return v, nil + } + + 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"), + ) + } + + // Perform resolution only if the path starts with one of the specified prefixes. + for _, prefix := range prefixes { + if path.HasPrefix(prefix) { + return lookup(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 6761ffabc6..27f6b8617b 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/bundle/permissions" "github.com/databricks/cli/bundle/python" "github.com/databricks/cli/bundle/scripts" + "github.com/databricks/cli/libs/dyn" ) // The initialize phase fills in defaults and connects to the workspace. @@ -28,6 +29,12 @@ func Initialize() bundle.Mutator { mutator.ExpandWorkspaceRoot(), mutator.DefineDefaultWorkspacePaths(), mutator.SetVariables(), + mutator.ResolveVariableReferencesFor( + dyn.NewPattern(dyn.Key("variables"), dyn.AnyKey(), dyn.Key("lookup")), + "bundle", + "workspace", + "variables", + ), mutator.ResolveResourceReferences(), mutator.ResolveVariableReferences( "bundle", From 9ed317d43e3949c9792a7f79d4dd0725ee725f8b Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Tue, 16 Apr 2024 16:08:28 +0200 Subject: [PATCH 3/4] fix typo --- bundle/config/mutator/resolve_resource_references.go | 1 + 1 file changed, 1 insertion(+) diff --git a/bundle/config/mutator/resolve_resource_references.go b/bundle/config/mutator/resolve_resource_references.go index a712b77b11..89eaa346c6 100644 --- a/bundle/config/mutator/resolve_resource_references.go +++ b/bundle/config/mutator/resolve_resource_references.go @@ -18,6 +18,7 @@ func ResolveResourceReferences() bundle.Mutator { func (m *resolveResourceReferences) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { errs, errCtx := errgroup.WithContext(ctx) + for k := range b.Config.Variables { v := b.Config.Variables[k] if v == nil || v.Lookup == nil { From a6a5b40d7d2a48bc54ee7ee16b303d1ac1d5fb44 Mon Sep 17 00:00:00 2001 From: Andrew Nester Date: Wed, 17 Apr 2024 17:28:47 +0200 Subject: [PATCH 4/4] fix --- .../resolve_resource_references_test.go | 21 ++--- .../mutator/resolve_variable_references.go | 77 ++++++++++--------- bundle/phases/initialize.go | 8 +- 3 files changed, 48 insertions(+), 58 deletions(-) diff --git a/bundle/config/mutator/resolve_resource_references_test.go b/bundle/config/mutator/resolve_resource_references_test.go index 70ca030377..60636bcc68 100644 --- a/bundle/config/mutator/resolve_resource_references_test.go +++ b/bundle/config/mutator/resolve_resource_references_test.go @@ -8,7 +8,6 @@ import ( "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" "github.com/databricks/cli/bundle/config/variable" - "github.com/databricks/cli/libs/dyn" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -143,7 +142,7 @@ func TestResolveVariableReferencesInVariableLookups(t *testing.T) { b := &bundle.Bundle{ Config: config.Root{ Bundle: config.Bundle{ - Environment: "dev", + Target: "dev", }, Variables: map[string]*variable.Variable{ "foo": { @@ -151,7 +150,7 @@ func TestResolveVariableReferencesInVariableLookups(t *testing.T) { }, "lookup": { Lookup: &variable.Lookup{ - Cluster: "cluster-${var.foo}-${bundle.environment}", + Cluster: "cluster-${var.foo}-${bundle.target}", }, }, }, @@ -165,21 +164,15 @@ func TestResolveVariableReferencesInVariableLookups(t *testing.T) { ClusterId: "1234-5678-abcd", }, nil) - diags := bundle.Apply(context.Background(), b, bundle.Seq(ResolveVariableReferencesFor( - dyn.NewPattern(dyn.Key("variables"), dyn.AnyKey(), dyn.Key("lookup")), - "bundle", - "variables", - ), ResolveResourceReferences())) + 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{ - Bundle: config.Bundle{ - Environment: "dev", - }, Variables: map[string]*variable.Variable{ "another_lookup": { Lookup: &variable.Lookup{ @@ -198,10 +191,6 @@ func TestResolveLookupVariableReferencesInVariableLookups(t *testing.T) { m := mocks.NewMockWorkspaceClient(t) b.SetWorkpaceClient(m.WorkspaceClient) - diags := bundle.Apply(context.Background(), b, bundle.Seq(ResolveVariableReferencesFor( - dyn.NewPattern(dyn.Key("variables"), dyn.AnyKey(), dyn.Key("lookup")), - "bundle", - "variables", - ), ResolveResourceReferences())) + 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 b86b23fb7a..f7fce6c821 100644 --- a/bundle/config/mutator/resolve_variable_references.go +++ b/bundle/config/mutator/resolve_variable_references.go @@ -5,6 +5,7 @@ import ( "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" @@ -15,14 +16,49 @@ 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 ResolveVariableReferencesFor(p dyn.Pattern, prefixes ...string) bundle.Mutator { - return &resolveVariableReferences{prefixes: prefixes, pattern: p} +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 { @@ -54,41 +90,12 @@ 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. - v, err := dyn.GetByPath(normalized, path) - if err != nil { - return dyn.InvalidValue, err - } - - // Check if the variable we're looking up has a lookup field in it - lookupV, err := dyn.GetByPath(normalized, path[:len(path)-1].Append(dyn.Key("lookup"))) - - // If nothing is found, it means the variable is not a lookup variable, so we can just return the value - if err != nil || lookupV == dyn.InvalidValue { - return v, nil - } - - // Check that the lookup variable was resolved to non-empty string already - sv, ok := v.AsString() - if !ok { - return v, nil - } - - // If the lookup variable is empty, it means it was not resolved yet which means it is used inside another lookup variable - if sv == "" { - return dyn.InvalidValue, fmt.Errorf("lookup variables cannot contain references to another lookup variables") - } - - return v, nil - } + // 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) { @@ -104,7 +111,7 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle) // Perform resolution only if the path starts with one of the specified prefixes. for _, prefix := range prefixes { if path.HasPrefix(prefix) { - return lookup(path) + return m.lookupFn(normalized, path) } } diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index 27f6b8617b..d6a1b95da9 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -9,7 +9,6 @@ import ( "github.com/databricks/cli/bundle/permissions" "github.com/databricks/cli/bundle/python" "github.com/databricks/cli/bundle/scripts" - "github.com/databricks/cli/libs/dyn" ) // The initialize phase fills in defaults and connects to the workspace. @@ -29,12 +28,7 @@ func Initialize() bundle.Mutator { mutator.ExpandWorkspaceRoot(), mutator.DefineDefaultWorkspacePaths(), mutator.SetVariables(), - mutator.ResolveVariableReferencesFor( - dyn.NewPattern(dyn.Key("variables"), dyn.AnyKey(), dyn.Key("lookup")), - "bundle", - "workspace", - "variables", - ), + mutator.ResolveVariableReferencesInLookup(), mutator.ResolveResourceReferences(), mutator.ResolveVariableReferences( "bundle",