Set engine based on state; refactor commands #3797
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
denik
requested review from
andrewnester,
anton-107,
jefferycheng1,
kanterov,
lennartkats-db,
pietern and
shreyas-goenka
as code owners
Collaborator
16 failing tests:
|
denik
changed the title
denik
force-pushed
the
denik/require-engine-init
branch
from
27cb3b3 to
0be1903
Compare
denik
force-pushed
the
denik/require-engine-init
branch
from
9326a2b to
bc34c50
Compare
denik
force-pushed
the
denik/require-engine-init
branch
from
bc34c50 to
84de40b
Compare
denik
changed the title
denik
force-pushed
the
denik/require-engine-init
branch
from
a47f91a to
c64329d
Compare
denik
force-pushed
the
denik/require-engine-init
branch
from
c64329d to
4cfc80b
Compare
denik
force-pushed
the
denik/require-engine-init
branch
from
d382efa to
c64cb09
Compare
denik
force-pushed
the
denik/require-engine-init
branch
from
5bac25e to
6b4d7ea
Compare
andrewnester
approved these changes
Oct 30, 2025
| }() | ||
|
|
||
| bundle.ApplySeqContext(ctx, b, | ||
| statemgmt.StatePull(), |
Contributor
There was a problem hiding this comment.
Why don't we pull state anymore?
Contributor
Author
There was a problem hiding this comment.
we do, it's a function called PullResourcesState()
denik
added a commit
that referenced
this pull request
Oct 31, 2025
## Why Now that the engine is selected based on the actual state available, it's safe to use allow users to specify DATABRICKS_BUNDLE_ENGINE="direct", only new bundles will be affected. Depends on #3797
pietern
added a commit
that referenced
this pull request
Oct 31, 2025
The test TestParseResourcesStateWithExistingStateFile was creating a terraform.tfstate file in the test source directory instead of in the temporary directory. This was introduced in PR #3797 when the code was changed from using b.StateLocalPath(ctx) to b.StateFilenameTerraform(ctx). The bug was that StateFilenameTerraform returns two values (remote filename, local path), but the test only captured the first value (filename) instead of the second value (full local path). This fix: - Captures the second return value (full local path) instead of the first - Creates the parent directory structure with os.MkdirAll - Writes the file to the correct location in the temp directory - Uses proper file permissions consistent with the codebase (0o700/0o600)
pietern
added a commit
that referenced
this pull request
Nov 3, 2025
## Changes PR #3797 replaced a function that returns a state file path with one that returns both a filename and a path. The test used only the filename, causing the state file to be created in the wrong directory. In the current state, it creates the file in `$PWD`, equal to the directory where the test file is located. The path in `localPath` is located inside the bundle cache directory, which is initialized to somewhere inside `t.TempDir()` above.
This was referenced Nov 6, 2025
shreyas-goenka
added a commit
that referenced
this pull request
Nov 6, 2025
## Changes Removes validation that incorrectly rejected bundles using the `definitions` field in `databricks.yml`. ## Why PR #3797 inadvertently applied validation to reject OSS Spark Declarative Pipelines to all Databricks CLI bundles. This error was only meant for the pipelines CLI. Customers commonly use the `definitions` field for YAML anchors (e.g., to reuse cluster configurations), and this validation blocked their deployments with the error: ``` Error: databricks.yml seems to be formatted for open-source Spark Declarative Pipelines. Pipelines CLI currently only supports Lakeflow Declarative Pipelines development. ``` The validation has been removed entirely. The Pipelines CLI is not yet in active use, and the pipelines team can add a better context-aware check later that only applies when running as the Pipelines CLI. ## Tests - New acceptance test: `acceptance/bundle/validate/definitions_yaml_anchors/` verifies bundles with `definitions` field validate successfully - Existing test: `acceptance/pipelines/deploy/oss-spark-error/` ensures the error continues to work for the pipelines CLI.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 7, 2025
## Changes ### User visible - Since #3797 this env var is only consulted if there is no state. Now it is always consulted and must match the state, otherwise error is raised. - New “bundle debug states” to print info about available state files. - Stricter state validation in case both terraform and direct state files are present. If serial numbers are the same, error rather than preferring direct. - Better error messages when state validation fails. ### Internal - Use string enum for engine instead if bool and update all functions to use the enum. - Move all engine parsing/configuration to bundle/config/engine package. - Engine configuration is done in top level command handler rather than inside PullResourcesState. - PullResourcesState() returns winning StateDesc object. ## Why Ensure users get the engine they select via env var (and in the future this will extend to config setting). ## Tests New acceptance tests. New test helper print_state.py to print state as is. --------- Co-authored-by: Pieter Noordhuis <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Engine selection
Engine is now selected based on available state, rather than env var. The env var is still consulted if there are no local or remote state files.
Since we need remote state, this is done during state-pull. This means we don't know engine in bundle validate/summary and in deploy/destroy until we reached that stage.
Since we don't know if remote is migrated, we pull both terraform state and direct state all the time and decide based on serial number which to use.
Command refactoring
There are many commands that needed refactoring, so I extracted common bundle steps in cmd/bundle/utils/process.go. This allows to enforce certain order on how things are run and encode assumptions in one place. For example, you cannot pull state until you called phases.Initialize() because certain paths are not initialized.
Why
This makes bundle engine setting sticky, once migrated to direct it'll stay on direct. This will be important for subsequent 'bundle deployment migrate' command.