From cefd5bf8312497a28a9181bc8c31bb205de691fc Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 16 Jan 2025 13:05:12 +0100 Subject: [PATCH 1/9] Resolve variables in a loop --- .../complex-transitive-deep/output.txt | 2 +- .../mutator/resolve_variable_references.go | 49 ++++++++++++++++--- bundle/phases/initialize.go | 5 -- 3 files changed, 44 insertions(+), 12 deletions(-) diff --git a/acceptance/bundle/variables/complex-transitive-deep/output.txt b/acceptance/bundle/variables/complex-transitive-deep/output.txt index a031e04971..29c41cda50 100644 --- a/acceptance/bundle/variables/complex-transitive-deep/output.txt +++ b/acceptance/bundle/variables/complex-transitive-deep/output.txt @@ -1,3 +1,3 @@ { - "spark.databricks.sql.initial.catalog.name": "${var.catalog}" + "spark.databricks.sql.initial.catalog.name": "hive_metastore" } diff --git a/bundle/config/mutator/resolve_variable_references.go b/bundle/config/mutator/resolve_variable_references.go index 11ac529d08..610836b46b 100644 --- a/bundle/config/mutator/resolve_variable_references.go +++ b/bundle/config/mutator/resolve_variable_references.go @@ -3,6 +3,7 @@ package mutator import ( "context" "errors" + "fmt" "github.com/databricks/cli/bundle" "github.com/databricks/cli/bundle/config" @@ -13,15 +14,22 @@ import ( "github.com/databricks/cli/libs/dyn/dynvar" ) +const maxResolutionRounds = 100 + 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 { @@ -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 == 0 { + break + } + + if round >= maxResolutionRounds-1 { + diags = diags.Append(diag.Diagnostic{ + 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) (int, diag.Diagnostics) { + var diags diag.Diagnostics + hasUpdates := 0 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. @@ -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 += 1 return m.lookupFn(normalized, path, b) } } @@ -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 } diff --git a/bundle/phases/initialize.go b/bundle/phases/initialize.go index 0328cc2ff6..3ecdc2ee9b 100644 --- a/bundle/phases/initialize.go +++ b/bundle/phases/initialize.go @@ -66,11 +66,6 @@ func Initialize() bundle.Mutator { "workspace", "variables", ), - mutator.ResolveVariableReferences( - "bundle", - "workspace", - "variables", - ), mutator.MergeJobClusters(), mutator.MergeJobParameters(), From 154a4bf9b86c000d19e44f4fa2b41c0aca05f6cd Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 16 Jan 2025 13:18:27 +0100 Subject: [PATCH 2/9] use bool for hasUpdates instead of int --- bundle/config/mutator/resolve_variable_references.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bundle/config/mutator/resolve_variable_references.go b/bundle/config/mutator/resolve_variable_references.go index 610836b46b..fdebdad966 100644 --- a/bundle/config/mutator/resolve_variable_references.go +++ b/bundle/config/mutator/resolve_variable_references.go @@ -104,7 +104,7 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle) break } - if hasUpdates == 0 { + if !hasUpdates { break } @@ -119,9 +119,9 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle) return diags } -func (m *resolveVariableReferences) resolveOnce(b *bundle.Bundle, prefixes []dyn.Path, varPath dyn.Path) (int, diag.Diagnostics) { +func (m *resolveVariableReferences) resolveOnce(b *bundle.Bundle, prefixes []dyn.Path, varPath dyn.Path) (bool, diag.Diagnostics) { var diags diag.Diagnostics - hasUpdates := 0 + 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. @@ -164,7 +164,7 @@ func (m *resolveVariableReferences) resolveOnce(b *bundle.Bundle, prefixes []dyn if m.skipFn != nil && m.skipFn(v) { return dyn.InvalidValue, dynvar.ErrSkipResolution } - hasUpdates += 1 + hasUpdates = true return m.lookupFn(normalized, path, b) } } From d7dba5e8d80e231c9a497b535f5681d81bd2bbf7 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 16 Jan 2025 13:44:03 +0100 Subject: [PATCH 3/9] add a test for cycle --- acceptance/bundle/variables/cycle/databricks.yml | 8 ++++++++ acceptance/bundle/variables/cycle/output.txt | 14 ++++++++++++++ acceptance/bundle/variables/cycle/script | 1 + 3 files changed, 23 insertions(+) create mode 100644 acceptance/bundle/variables/cycle/databricks.yml create mode 100644 acceptance/bundle/variables/cycle/output.txt create mode 100644 acceptance/bundle/variables/cycle/script diff --git a/acceptance/bundle/variables/cycle/databricks.yml b/acceptance/bundle/variables/cycle/databricks.yml new file mode 100644 index 0000000000..b35196671d --- /dev/null +++ b/acceptance/bundle/variables/cycle/databricks.yml @@ -0,0 +1,8 @@ +bundle: + name: cycle + +variables: + a: + default: ${var.b} + b: + default: ${var.a} diff --git a/acceptance/bundle/variables/cycle/output.txt b/acceptance/bundle/variables/cycle/output.txt new file mode 100644 index 0000000000..ea9c95cd46 --- /dev/null +++ b/acceptance/bundle/variables/cycle/output.txt @@ -0,0 +1,14 @@ +Error: cycle detected in field resolution: variables.a.default -> var.b -> var.a -> var.b + +{ + "a": { + "default": "${var.b}", + "value": "${var.b}" + }, + "b": { + "default": "${var.a}", + "value": "${var.a}" + } +} + +Exit code: 1 diff --git a/acceptance/bundle/variables/cycle/script b/acceptance/bundle/variables/cycle/script new file mode 100644 index 0000000000..0e53f237e8 --- /dev/null +++ b/acceptance/bundle/variables/cycle/script @@ -0,0 +1 @@ +$CLI bundle validate -o json | jq .variables From 2aae75936a01dd23ac4c4d2a3a676c0178ad387a Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 16 Jan 2025 13:52:56 +0100 Subject: [PATCH 4/9] add a test for complex-cycle; reduce rounds to 12 maxRounds vs test time 9 0.10s 10 0.13s 11 0.27s 12 0.68s 13 1.98s 14 6.28s 15 21.70s 16 78.16s ~/work/cli/acceptance/bundle/variables/complex-cycle % time testme + go test ../../.. -v -run ^TestAccept$/^bundle$/^variables$/^complex-cycle$ === RUN TestAccept acceptance_test.go:289: [go build -mod vendor -o /Users/denis.bilenko/work/cli/acceptance/build/databricks] took 459.577542ms acceptance_test.go:64: $TMPHOME=/var/folders/5y/9kkdnjw91p11vsqwk0cvmk200000gp/T/TestAccept3684472427/001 === RUN TestAccept/bundle/variables/complex-cycle === PAUSE TestAccept/bundle/variables/complex-cycle === CONT TestAccept/bundle/variables/complex-cycle acceptance_test.go:212: Error Trace: /Users/denis.bilenko/work/cli/libs/testdiff/testdiff.go:24 /Users/denis.bilenko/work/cli/acceptance/acceptance_test.go:212 /Users/denis.bilenko/work/cli/acceptance/acceptance_test.go:162 /Users/denis.bilenko/work/cli/acceptance/acceptance_test.go:100 Error: Not equal: expected: "Warning: Detected unresolved variables after 12 resolution rounds\n\nName: cycle\nTarget: default\nWorkspace:\n User: $USERNAME\n Path: /Workspace/Users/$USERNAME/.bundle/cycle/default\n\nFound 1 warning\n" actual : "Warning: Detected unresolved variables after 14 resolution rounds\n\nName: cycle\nTarget: default\nWorkspace:\n User: $USERNAME\n Path: /Workspace/Users/$USERNAME/.bundle/cycle/default\n\nFound 1 warning\n" Diff: --- Expected +++ Actual @@ -1,2 +1,2 @@ -Warning: Detected unresolved variables after 12 resolution rounds +Warning: Detected unresolved variables after 14 resolution rounds Test: TestAccept/bundle/variables/complex-cycle Messages: bundle/variables/complex-cycle/output.txt vs script output --- FAIL: TestAccept (0.93s) --- FAIL: TestAccept/bundle/variables/complex-cycle (6.28s) FAIL FAIL github.com/databricks/cli/acceptance 7.585s FAIL testme 14.04s user 7.50s system 257% cpu 8.370 total --- .../bundle/variables/complex-cycle/databricks.yml | 10 ++++++++++ acceptance/bundle/variables/complex-cycle/output.txt | 9 +++++++++ acceptance/bundle/variables/complex-cycle/script | 1 + bundle/config/mutator/resolve_variable_references.go | 2 +- 4 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 acceptance/bundle/variables/complex-cycle/databricks.yml create mode 100644 acceptance/bundle/variables/complex-cycle/output.txt create mode 100644 acceptance/bundle/variables/complex-cycle/script diff --git a/acceptance/bundle/variables/complex-cycle/databricks.yml b/acceptance/bundle/variables/complex-cycle/databricks.yml new file mode 100644 index 0000000000..9784a4e258 --- /dev/null +++ b/acceptance/bundle/variables/complex-cycle/databricks.yml @@ -0,0 +1,10 @@ +bundle: + name: cycle + +variables: + a: + default: + hello: ${var.b} + b: + default: + hello: ${var.a} diff --git a/acceptance/bundle/variables/complex-cycle/output.txt b/acceptance/bundle/variables/complex-cycle/output.txt new file mode 100644 index 0000000000..ed724e19de --- /dev/null +++ b/acceptance/bundle/variables/complex-cycle/output.txt @@ -0,0 +1,9 @@ +Warning: Detected unresolved variables after 12 resolution rounds + +Name: cycle +Target: default +Workspace: + User: $USERNAME + Path: /Workspace/Users/$USERNAME/.bundle/cycle/default + +Found 1 warning diff --git a/acceptance/bundle/variables/complex-cycle/script b/acceptance/bundle/variables/complex-cycle/script new file mode 100644 index 0000000000..72555b332a --- /dev/null +++ b/acceptance/bundle/variables/complex-cycle/script @@ -0,0 +1 @@ +$CLI bundle validate diff --git a/bundle/config/mutator/resolve_variable_references.go b/bundle/config/mutator/resolve_variable_references.go index fdebdad966..23d35952e4 100644 --- a/bundle/config/mutator/resolve_variable_references.go +++ b/bundle/config/mutator/resolve_variable_references.go @@ -14,7 +14,7 @@ import ( "github.com/databricks/cli/libs/dyn/dynvar" ) -const maxResolutionRounds = 100 +const maxResolutionRounds = 12 type resolveVariableReferences struct { prefixes []string From 9d91d72cc5ddd3e4618cb129a6715ba52c4d4e72 Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 16 Jan 2025 14:14:10 +0100 Subject: [PATCH 5/9] add another cycle test with one variable --- .../bundle/variables/complex-cycle-self/databricks.yml | 7 +++++++ .../bundle/variables/complex-cycle-self/output.txt | 9 +++++++++ acceptance/bundle/variables/complex-cycle-self/script | 1 + 3 files changed, 17 insertions(+) create mode 100644 acceptance/bundle/variables/complex-cycle-self/databricks.yml create mode 100644 acceptance/bundle/variables/complex-cycle-self/output.txt create mode 100644 acceptance/bundle/variables/complex-cycle-self/script diff --git a/acceptance/bundle/variables/complex-cycle-self/databricks.yml b/acceptance/bundle/variables/complex-cycle-self/databricks.yml new file mode 100644 index 0000000000..bb461795cc --- /dev/null +++ b/acceptance/bundle/variables/complex-cycle-self/databricks.yml @@ -0,0 +1,7 @@ +bundle: + name: cycle + +variables: + a: + default: + hello: ${var.a} diff --git a/acceptance/bundle/variables/complex-cycle-self/output.txt b/acceptance/bundle/variables/complex-cycle-self/output.txt new file mode 100644 index 0000000000..ed724e19de --- /dev/null +++ b/acceptance/bundle/variables/complex-cycle-self/output.txt @@ -0,0 +1,9 @@ +Warning: Detected unresolved variables after 12 resolution rounds + +Name: cycle +Target: default +Workspace: + User: $USERNAME + Path: /Workspace/Users/$USERNAME/.bundle/cycle/default + +Found 1 warning diff --git a/acceptance/bundle/variables/complex-cycle-self/script b/acceptance/bundle/variables/complex-cycle-self/script new file mode 100644 index 0000000000..72555b332a --- /dev/null +++ b/acceptance/bundle/variables/complex-cycle-self/script @@ -0,0 +1 @@ +$CLI bundle validate From c3ac311e9c48f2059a05a4794fe84529e64acc8e Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 16 Jan 2025 14:20:36 +0100 Subject: [PATCH 6/9] fix maxRounds check --- bundle/config/mutator/resolve_variable_references.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/bundle/config/mutator/resolve_variable_references.go b/bundle/config/mutator/resolve_variable_references.go index 23d35952e4..84c5c741c5 100644 --- a/bundle/config/mutator/resolve_variable_references.go +++ b/bundle/config/mutator/resolve_variable_references.go @@ -94,8 +94,9 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle) varPath := dyn.NewPath(dyn.Key("var")) var diags diag.Diagnostics + maxRounds := 1 + m.extraRounds - for round := range 1 + m.extraRounds { + for round := range maxRounds { hasUpdates, newDiags := m.resolveOnce(b, prefixes, varPath) diags = diags.Extend(newDiags) @@ -108,12 +109,13 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle) break } - if round >= maxResolutionRounds-1 { + if round >= maxRounds-1 { diags = diags.Append(diag.Diagnostic{ 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 }) + break } } return diags From d2c79c81531ff479fbffc94b45e5f1a7aeeb131d Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 16 Jan 2025 14:23:45 +0100 Subject: [PATCH 7/9] Reduce rounds to 11. Add a comment about timings. On CI timings for complex-cycle are worse than my laptop (for 12 rounds): ubuntu: acceptance.TestAccept/bundle/variables/complex-cycle (2.57s) mac: acceptance.TestAccept/bundle/variables/complex-cycle (1.54s) windows: acceptance.TestAccept/bundle/variables/complex-cycle (2.80s) --- .../mutator/resolve_variable_references.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/bundle/config/mutator/resolve_variable_references.go b/bundle/config/mutator/resolve_variable_references.go index 84c5c741c5..9aa93791f8 100644 --- a/bundle/config/mutator/resolve_variable_references.go +++ b/bundle/config/mutator/resolve_variable_references.go @@ -14,7 +14,22 @@ import ( "github.com/databricks/cli/libs/dyn/dynvar" ) -const maxResolutionRounds = 12 +/* +For pathological cases, output and time grow exponentially. + +On my laptop, timings for acceptance/bundle/variables/complex-cycle: +rounds time + + 9 0.10s + 10 0.13s + 11 0.27s + 12 0.68s + 13 1.98s + 14 6.28s + 15 21.70s + 16 78.16s +*/ +const maxResolutionRounds = 11 type resolveVariableReferences struct { prefixes []string From 56267a7350adc16d4eec74554131dfcf7a6210bf Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 16 Jan 2025 14:30:20 +0100 Subject: [PATCH 8/9] add acceptance/bundle/variables/complex-cross-ref (it passes on main, added for completeness) --- .../complex-cross-ref/databricks.yml | 12 ++++++++++ .../variables/complex-cross-ref/output.txt | 22 +++++++++++++++++++ .../bundle/variables/complex-cross-ref/script | 1 + 3 files changed, 35 insertions(+) create mode 100644 acceptance/bundle/variables/complex-cross-ref/databricks.yml create mode 100644 acceptance/bundle/variables/complex-cross-ref/output.txt create mode 100644 acceptance/bundle/variables/complex-cross-ref/script diff --git a/acceptance/bundle/variables/complex-cross-ref/databricks.yml b/acceptance/bundle/variables/complex-cross-ref/databricks.yml new file mode 100644 index 0000000000..4459f44df9 --- /dev/null +++ b/acceptance/bundle/variables/complex-cross-ref/databricks.yml @@ -0,0 +1,12 @@ +bundle: + name: complex-cross-ref + +variables: + a: + default: + a_1: 500 + a_2: ${var.b.b_2} + b: + default: + b_1: ${var.a.a_1} + b_2: 2.5 diff --git a/acceptance/bundle/variables/complex-cross-ref/output.txt b/acceptance/bundle/variables/complex-cross-ref/output.txt new file mode 100644 index 0000000000..f1b624d29f --- /dev/null +++ b/acceptance/bundle/variables/complex-cross-ref/output.txt @@ -0,0 +1,22 @@ +{ + "a": { + "default": { + "a_1": 500, + "a_2": 2.5 + }, + "value": { + "a_1": 500, + "a_2": 2.5 + } + }, + "b": { + "default": { + "b_1": 500, + "b_2": 2.5 + }, + "value": { + "b_1": 500, + "b_2": 2.5 + } + } +} diff --git a/acceptance/bundle/variables/complex-cross-ref/script b/acceptance/bundle/variables/complex-cross-ref/script new file mode 100644 index 0000000000..0e53f237e8 --- /dev/null +++ b/acceptance/bundle/variables/complex-cross-ref/script @@ -0,0 +1 @@ +$CLI bundle validate -o json | jq .variables From 1e37279566bbc882d94eafbbd0e1e96047145c3e Mon Sep 17 00:00:00 2001 From: Denis Bilenko Date: Thu, 16 Jan 2025 14:48:01 +0100 Subject: [PATCH 9/9] fix tests 12 -> 11 --- acceptance/bundle/variables/complex-cycle-self/output.txt | 2 +- acceptance/bundle/variables/complex-cycle/output.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/acceptance/bundle/variables/complex-cycle-self/output.txt b/acceptance/bundle/variables/complex-cycle-self/output.txt index ed724e19de..fa80154ca6 100644 --- a/acceptance/bundle/variables/complex-cycle-self/output.txt +++ b/acceptance/bundle/variables/complex-cycle-self/output.txt @@ -1,4 +1,4 @@ -Warning: Detected unresolved variables after 12 resolution rounds +Warning: Detected unresolved variables after 11 resolution rounds Name: cycle Target: default diff --git a/acceptance/bundle/variables/complex-cycle/output.txt b/acceptance/bundle/variables/complex-cycle/output.txt index ed724e19de..fa80154ca6 100644 --- a/acceptance/bundle/variables/complex-cycle/output.txt +++ b/acceptance/bundle/variables/complex-cycle/output.txt @@ -1,4 +1,4 @@ -Warning: Detected unresolved variables after 12 resolution rounds +Warning: Detected unresolved variables after 11 resolution rounds Name: cycle Target: default