Resolve variables in a loop by denik · Pull Request #2164 · databricks/cli

denik · 2025-01-16T12:13:40Z

Changes

Instead of doing 2 passes on variable resolution, do a loop until there are no more updates (or we reach count 100).
Stacked on top of Add a test for complex variable resolution with 3 levels #2163 which is a regression test for this: acceptance/bundle/variables/complex-transitive-deep

Tests

Existing tests, new regression tests.

These tests already passed before, added for completeness:

acceptance/bundle/variables/cycle
acceptance/bundle/variables/complex-cross-ref

andrewnester · 2025-01-16T12:19:51Z

 	"github.com/databricks/cli/libs/dyn/dynvar"
 )

+const maxResolutionRounds = 100


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 ?

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

andrewnester · 2025-01-16T12:34:36Z

@@ -1,3 +1,3 @@
 {
- "spark.databricks.sql.initial.catalog.name": "${var.catalog}"


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

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

andrewnester · 2025-01-16T12:35:49Z

+ }
+
+ if round >= maxResolutionRounds-1 {
+ diags = diags.Append(diag.Diagnostic{


shall we break here as well?

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

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

good catch! it's a bug. c3ac311

andrewnester · 2025-01-16T12:48:23Z

+ name: cycle
+
+variables:
+ a:


I meant a deeper level of nesting for cycle :) something like
variables: a: default: ${var.complex.a} b: default: ${var.a} c: default: ${var.b} complex: default: a: ${var.c} b: foo

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

andrewnester · 2025-01-16T13:12:34Z

 	"github.com/databricks/cli/libs/dyn/dynvar"
 )

+const maxResolutionRounds = 12


Can you add a comment why 12? It might not be obvious for someone reading the code later

Added a comment

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)

… added for completeness)

denik · 2025-01-16T14:37:31Z

After changing to 11 rounds, times on CI:

ubuntu: acceptance.TestAccept/bundle/variables/complex-cycle (1.28s)
mac: acceptance.TestAccept/bundle/variables/complex-cycle (0.64s)
win: acceptance.TestAccept/bundle/variables/complex-cycle (1.65s)

with 12 it was:
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)

New feature announcement. You can now manage Databricks Apps using DABs by defining an `app` resource in your bundle configuration. For more information see Databricks documentation https://docs.databricks.com/en/dev-tools/bundles/resources.html#app CLI: * Filter out system clusters in cluster picker ([#2131](#2131)). * Process all the fields in top level request object even if it contains request body ([#2155](#2155)). Bundles: * Added support for Databricks Apps in DABs ([#1928](#1928)). * Allow artifact path to be located outside the sync root ([#2128](#2128)). * Retry app deployment if there is an active deployment in progress ([#2153](#2153)). * Resolve variables in a loop ([#2164](#2164)). * Improve resolution of complex variables within complex variables ([#2157](#2157)). * Added output message to warn about slower deployments with apps ([#2161](#2161)). * Patch references to UC schemas to capture dependencies automatically ([#1989](#1989)). * Format default-python template ([#2110](#2110)). * Encourage the use of root_path in production to ensure single deployment ([#1712](#1712)). * Log warnings to stderr for "bundle validate -o json" ([#2109](#2109)). Internal: * Move merge fix-ups after variable resolution ([#2125](#2125)). * Enable linter 'unconvert' and fix the issues found ([#2136](#2136)). * Coverage for acceptance tests ([#2123](#2123)). * Add acceptance tests for builtin templates ([#2135](#2135)). * Add a unique schema for recreate pipeline test ([#2159](#2159)). * Migrate resolution tests to acceptance tests ([#2143](#2143)). * Update runner for the publish-winget job ([#2105](#2105)). * Add a test for complex variable resolution with 3 levels ([#2163](#2163)). API Changes: * Changed `databricks account federation-policy update` command with new required argument order. * Changed `databricks account service-principal-federation-policy update` command with new required argument order. OpenAPI commit 779817ed8d63031f5ea761fbd25ee84f38feec0d (2025-01-08) Dependency updates: * Upgrade TF provider to 1.63.0 ([#2162](#2162)). * Bump golangci-lint version to v1.63.4 from v1.63.1 ([#2114](#2114)). * Bump astral-sh/setup-uv from 4 to 5 ([#2116](#2116)). * Bump golang.org/x/oauth2 from 0.24.0 to 0.25.0 ([#2080](#2080)). * Bump github.com/hashicorp/hc-install from 0.9.0 to 0.9.1 ([#2079](#2079)). * Bump golang.org/x/term from 0.27.0 to 0.28.0 ([#2078](#2078)). * Bump github.com/databricks/databricks-sdk-go from 0.54.0 to 0.55.0 ([#2126](#2126)).

### New feature announcement #### Databricks Apps support You can now manage Databricks Apps using DABs by defining an `app` resource in your bundle configuration. For more information see Databricks documentation https://docs.databricks.com/en/dev-tools/bundles/resources.html#app #### Referencing complex variables in complex variables You can now reference complex variables within other complex variables. For more details see #2157 CLI: * Filter out system clusters in cluster picker ([#2131](#2131)). * Add command line flags for fields that are not in the API request body ([#2155](#2155)). Bundles: * Added support for Databricks Apps in DABs ([#1928](#1928)). * Allow artifact path to be located outside the sync root ([#2128](#2128)). * Retry app deployment if there is an active deployment in progress ([#2153](#2153)). * Resolve variables in a loop ([#2164](#2164)). * Improve resolution of complex variables within complex variables ([#2157](#2157)). * Added output message to warn about slower deployments with apps ([#2161](#2161)). * Patch references to UC schemas to capture dependencies automatically ([#1989](#1989)). * Format default-python template ([#2110](#2110)). * Encourage the use of root_path in production to ensure single deployment ([#1712](#1712)). * Log warnings to stderr for "bundle validate -o json" ([#2109](#2109)). API Changes: * Changed `databricks account federation-policy update` command with new required argument order. * Changed `databricks account service-principal-federation-policy update` command with new required argument order. OpenAPI commit 779817ed8d63031f5ea761fbd25ee84f38feec0d (2025-01-08) Dependency updates: * Upgrade TF provider to 1.63.0 ([#2162](#2162)). * Bump golangci-lint version to v1.63.4 from v1.63.1 ([#2114](#2114)). * Bump astral-sh/setup-uv from 4 to 5 ([#2116](#2116)). * Bump golang.org/x/oauth2 from 0.24.0 to 0.25.0 ([#2080](#2080)). * Bump github.com/hashicorp/hc-install from 0.9.0 to 0.9.1 ([#2079](#2079)). * Bump golang.org/x/term from 0.27.0 to 0.28.0 ([#2078](#2078)). * Bump github.com/databricks/databricks-sdk-go from 0.54.0 to 0.55.0 ([#2126](#2126)). --------- Co-authored-by: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com>

## Changes - Instead of doing 2 passes on variable resolution, do a loop until there are no more updates (or we reach count 100). - Stacked on top of #2163 which is a regression test for this: acceptance/bundle/variables/complex-transitive-deep ## Tests Existing tests, new regression tests. These tests already passed before, added for completeness: - acceptance/bundle/variables/cycle - acceptance/bundle/variables/complex-cross-ref

### New feature announcement #### Databricks Apps support You can now manage Databricks Apps using DABs by defining an `app` resource in your bundle configuration. For more information see Databricks documentation https://docs.databricks.com/en/dev-tools/bundles/resources.html#app #### Referencing complex variables in complex variables You can now reference complex variables within other complex variables. For more details see #2157 CLI: * Filter out system clusters in cluster picker ([#2131](#2131)). * Add command line flags for fields that are not in the API request body ([#2155](#2155)). Bundles: * Added support for Databricks Apps in DABs ([#1928](#1928)). * Allow artifact path to be located outside the sync root ([#2128](#2128)). * Retry app deployment if there is an active deployment in progress ([#2153](#2153)). * Resolve variables in a loop ([#2164](#2164)). * Improve resolution of complex variables within complex variables ([#2157](#2157)). * Added output message to warn about slower deployments with apps ([#2161](#2161)). * Patch references to UC schemas to capture dependencies automatically ([#1989](#1989)). * Format default-python template ([#2110](#2110)). * Encourage the use of root_path in production to ensure single deployment ([#1712](#1712)). * Log warnings to stderr for "bundle validate -o json" ([#2109](#2109)). API Changes: * Changed `databricks account federation-policy update` command with new required argument order. * Changed `databricks account service-principal-federation-policy update` command with new required argument order. OpenAPI commit 779817ed8d63031f5ea761fbd25ee84f38feec0d (2025-01-08) Dependency updates: * Upgrade TF provider to 1.63.0 ([#2162](#2162)). * Bump golangci-lint version to v1.63.4 from v1.63.1 ([#2114](#2114)). * Bump astral-sh/setup-uv from 4 to 5 ([#2116](#2116)). * Bump golang.org/x/oauth2 from 0.24.0 to 0.25.0 ([#2080](#2080)). * Bump github.com/hashicorp/hc-install from 0.9.0 to 0.9.1 ([#2079](#2079)). * Bump golang.org/x/term from 0.27.0 to 0.28.0 ([#2078](#2078)). * Bump github.com/databricks/databricks-sdk-go from 0.54.0 to 0.55.0 ([#2126](#2126)). --------- Co-authored-by: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com>

denik requested review from andrewnester, pietern and shreyas-goenka as code owners January 16, 2025 12:13

denik temporarily deployed to test-trigger-is January 16, 2025 12:13 — with GitHub Actions Inactive

denik mentioned this pull request Jan 16, 2025
Add a test for complex variable resolution with 3 levels #2163
Merged

denik temporarily deployed to test-trigger-is January 16, 2025 12:18 — with GitHub Actions Inactive

andrewnester reviewed Jan 16, 2025
View reviewed changes

Base automatically changed from denik/complex-vars-3 to mainJanuary 16, 2025 12:21

denik requested a review from andrewnester January 16, 2025 12:23

denik added 2 commits January 16, 2025 13:31

Resolve variables in a loop
cefd5bf

use bool for hasUpdates instead of int
154a4bf

denik force-pushed the denik/complex-vars-loop branch from 12aa6bc to 154a4bfCompare January 16, 2025 12:32

denik temporarily deployed to test-trigger-is January 16, 2025 12:32 — with GitHub Actions Inactive

andrewnester reviewed Jan 16, 2025
View reviewed changes

add a test for cycle
d7dba5e

denik temporarily deployed to test-trigger-is January 16, 2025 12:44 — with GitHub Actions Inactive

andrewnester reviewed Jan 16, 2025
View reviewed changes

denik temporarily deployed to test-trigger-is January 16, 2025 13:06 — with GitHub Actions Inactive

andrewnester reviewed Jan 16, 2025
View reviewed changes

add another cycle test with one variable
9d91d72

denik temporarily deployed to test-trigger-is January 16, 2025 13:14 — with GitHub Actions Inactive

fix maxRounds check
c3ac311

denik temporarily deployed to test-trigger-is January 16, 2025 13:21 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is January 16, 2025 13:25 — with GitHub Actions Inactive

andrewnester approved these changes Jan 16, 2025
View reviewed changes

add acceptance/bundle/variables/complex-cross-ref (it passes on main,…
56267a7
… added for completeness)

denik temporarily deployed to test-trigger-is January 16, 2025 13:30 — with GitHub Actions Inactive

fix tests 12 -> 11
1e37279

denik temporarily deployed to test-trigger-is January 16, 2025 13:48 — with GitHub Actions Inactive

denik added this pull request to the merge queue Jan 16, 2025

Merged via the queue into main with commit 2e70558Jan 16, 2025

denik deleted the denik/complex-vars-loop branch January 16, 2025 14:45

andrewnester mentioned this pull request Jan 16, 2025
[Release] Release v0.239.0 #2167
Merged

Navigation Menu

Uh oh!

Resolve variables in a loop - #2164

Resolve variables in a loop#2164
denik merged 9 commits into
mainfrom
denik/complex-vars-loop

denik commented Jan 16, 2025•
edited
Loading

Uh oh!

andrewnesterJan 16, 2025

Uh oh!

denikJan 16, 2025

Uh oh!

andrewnesterJan 16, 2025

Uh oh!

denikJan 16, 2025

Uh oh!

andrewnesterJan 16, 2025

Uh oh!

denikJan 16, 2025

Uh oh!

andrewnesterJan 16, 2025

Uh oh!

denikJan 16, 2025

Uh oh!

andrewnesterJan 16, 2025•
edited
Loading

Uh oh!

andrewnesterJan 16, 2025

Uh oh!

denikJan 16, 2025

Uh oh!

denik commented Jan 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -1,3 +1,3 @@
		{
		"spark.databricks.sql.initial.catalog.name": "${var.catalog}"

Uh oh!

Conversation

denik commented Jan 16, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewnesterJan 16, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denik commented Jan 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

denik commented Jan 16, 2025•
edited
Loading

andrewnesterJan 16, 2025•
edited
Loading