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 variables in a loop #2164

Merged
merged 9 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from 2 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
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"spark.databricks.sql.initial.catalog.name": "${var.catalog}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for cases when there are nested cycle dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See acceptance/bundle/variables/complex-cycle/databricks.yml

"spark.databricks.sql.initial.catalog.name": "hive_metastore"
}
49 changes: 43 additions & 6 deletions bundle/config/mutator/resolve_variable_references.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package mutator
import (
"context"
"errors"
"fmt"

"github.com/databricks/cli/bundle"
"github.com/databricks/cli/bundle/config"
Expand All @@ -13,15 +14,22 @@ import (
"github.com/databricks/cli/libs/dyn/dynvar"
)

const maxResolutionRounds = 100
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we really should support this, it looks like all configurations can be refactored into 2 level deep anyways. Maybe warning about more deep nesting instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's easier to support this than explain why only 2 levels.


type resolveVariableReferences struct {
prefixes []string
pattern dyn.Pattern
lookupFn func(dyn.Value, dyn.Path, *bundle.Bundle) (dyn.Value, error)
skipFn func(dyn.Value) bool
prefixes []string
pattern dyn.Pattern
lookupFn func(dyn.Value, dyn.Path, *bundle.Bundle) (dyn.Value, error)
skipFn func(dyn.Value) bool
extraRounds int
}

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

func ResolveVariableReferencesInLookup() bundle.Mutator {
Expand Down Expand Up @@ -87,6 +95,33 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle)

var diags diag.Diagnostics

for round := range 1 + m.extraRounds {
hasUpdates, newDiags := m.resolveOnce(b, prefixes, varPath)

diags = diags.Extend(newDiags)

if diags.HasError() {
break
}

if !hasUpdates {
break
}

if round >= maxResolutionRounds-1 {
diags = diags.Append(diag.Diagnostic{
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we break here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it's the last stmt in the last loop iteration, so it does not matter.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's not necessarily last one if I pass extraRounds bigger than maxResolutionRounds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! it's a bug. c3ac311

Severity: diag.Warning,
Summary: fmt.Sprintf("Detected unresolved variables after %d resolution rounds", round+1),
// Would be nice to include names of the variables there, but that would complicate things more
})
}
}
return diags
}

func (m *resolveVariableReferences) resolveOnce(b *bundle.Bundle, prefixes []dyn.Path, varPath dyn.Path) (bool, diag.Diagnostics) {
var diags diag.Diagnostics
hasUpdates := false
err := b.Config.Mutate(func(root dyn.Value) (dyn.Value, error) {
// Synthesize a copy of the root that has all fields that are present in the type
// but not set in the dynamic value set to their corresponding empty value.
Expand Down Expand Up @@ -129,6 +164,7 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle)
if m.skipFn != nil && m.skipFn(v) {
return dyn.InvalidValue, dynvar.ErrSkipResolution
}
hasUpdates = true
return m.lookupFn(normalized, path, b)
}
}
Expand All @@ -149,5 +185,6 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle)
if err != nil {
diags = diags.Extend(diag.FromErr(err))
}
return diags

return hasUpdates, diags
}
5 changes: 0 additions & 5 deletions bundle/phases/initialize.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,6 @@ func Initialize() bundle.Mutator {
"workspace",
"variables",
),
mutator.ResolveVariableReferences(
"bundle",
"workspace",
"variables",
),

mutator.MergeJobClusters(),
mutator.MergeJobParameters(),
Expand Down
Loading