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

Improve resolution of complex variables within complex variables #2157

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

denik
Copy link
Contributor

@denik denik commented Jan 15, 2025

Changes

  • Remove ResolveVariableReferencesInComplexVariables - it blocked complex-within-complex for no good reason.
  • Repeat regular resolution twice, it helps with a couple test cases we have.

There may be a case for running it 3 times or more in a loop, but there is no test case for that, so this PR is simple incremental improvement.

Tests

Existing acceptance tests. Previously all unit tests for complex variables were converted to acceptance tests, to capture this change and ensure nothing breaks.

denik added 2 commits January 15, 2025 16:09
This is the last test referencing ResolveVariableReferencesInComplexVariables.
- Remove ResolveVariableReferencesInComplexVariables - it blocked complex-within-complex for no good reason.
- Repeat regular resolution twice, it helps with a couple test cases we have.

There may be a case for running it 3 times or more in a loop, but there is no test case for that, so this PR
is simple incremental improvement.
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

This means a case where this goes one level deeper won't error anymore, but also won't resolve. Can we have a test case for this as well?

@denik
Copy link
Contributor Author

denik commented Jan 15, 2025

This means a case where this goes one level deeper won't error anymore, but also won't resolve. Can we have a test case for this as well?

There is one: https://github.com/databricks/cli/blob/main/acceptance/bundle/variables/complex-transitive-deeper/databricks.yml

@denik
Copy link
Contributor Author

denik commented Jan 15, 2025

Pl also stamp #2156 (part of the stack)

Base automatically changed from denik/migrate-complex-var-ref to main January 15, 2025 16:52
@pietern
Copy link
Contributor

pietern commented Jan 15, 2025

Thanks!

@denik
Copy link
Contributor Author

denik commented Jan 15, 2025

Although I feel something else is going in that case - there is an error and nothing is substituted. We should come up with a test where it is resolved but partially.

@denik denik merged commit 30dec59 into main Jan 15, 2025
7 of 8 checks passed
@denik denik deleted the denik/cleanup-resolver-complex branch January 15, 2025 17:03
@denik
Copy link
Contributor Author

denik commented Jan 16, 2025

@pietern a targeted test for need for the need for more passes: #2163

github-merge-queue bot pushed a commit that referenced this pull request Jan 16, 2025
Follow up to #2157. That PR repeated variable resolution. This test
still does not resolve fully but would resolve with 3 passes. This is
slightly different from complex-transitive-deeper - this test does not
show any errors, the issue is purely not enough passes.
andrewnester added a commit that referenced this pull request Jan 16, 2025
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)).
github-merge-queue bot pushed a commit that referenced this pull request Jan 16, 2025
### 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants