Uh oh!
There was an error while loading. Please reload this page.
direct: Support "bundle deploy --plan" to deploy previously generated plans - #4134
Merged
Conversation
Collaborator
Commit: 74ee713
629 interesting tests: 600 MISS, 14 KNOWN, 6 flaky, 6 RECOVERED, 3 FAIL
Top 50 slowest tests (at least 2 minutes):
|
denik
marked this pull request as ready for review
December 15, 2025 10:16
denik
requested review from
andrewnester, anton-107, pietern and shreyas-goenka
as code ownersDecember 15, 2025 10:16
| ### Bundles | ||
| * engine/direct: Fix dependency-ordered deletion by persisting depends_on in state ([#4105](https://github.com/databricks/cli/pull/4105)) | ||
| * Pass SYSTEM_ACCESSTOKEN from env to the Terraform provider ([#4135](https://github.com/databricks/cli/pull/4135) | ||
| * Pass SYSTEM\_ACCESSTOKEN from env to the Terraform provider ([#4135](https://github.com/databricks/cli/pull/4135)) |
ContributorAuthor
There was a problem hiding this comment.
original is missing closing paren and my editor highlights _ as needing escape.
| } | ||
| // Validate plan version | ||
| if plan.PlanVersion != currentPlanVersion { |
Contributor
There was a problem hiding this comment.
Can we also store the CLI version that the plan was generated with and show it in the error message here? To give users more context on what might have gone wrong.
ContributorAuthor
There was a problem hiding this comment.
we do store it; good idea to include it in the error message
| cmd.Flags().BoolVar(&autoApprove, "auto-approve", false, "Skip interactive approvals that might be required for deployment.") | ||
| cmd.Flags().MarkDeprecated("compute-id", "use --cluster-id instead") | ||
| cmd.Flags().BoolVar(&verbose, "verbose", false, "Enable verbose output.") | ||
| cmd.Flags().StringVar(&readPlanPath, "readplan", "", "Path to a JSON plan file to apply instead of planning (direct engine only).") |
Contributor
There was a problem hiding this comment.
suggestion: What about --plan? The read prefix is implied.
ContributorAuthor
There was a problem hiding this comment.
--plan is ambiguous, you can also produce and write plan as part of deploy
Contributor
There was a problem hiding this comment.
Why would we want to write the plan as part of deploy?
In general, we should render the plan by default when people run deploy.
Contributor
There was a problem hiding this comment.
+1, if ambiguous then the flag should be more verbose, or split on word boundary --read-plan.
| // and value is either pure or multiple references string: "${resources.foo.jobs.id}" or "${a} ${b}" | ||
| Refs map[string]string `json:"vars,omitempty"` | ||
| valueCache any |
Contributor
There was a problem hiding this comment.
Please add a doc string for this. Could be renamed to just value as well, if we rename the previous value to ValueJson.
| // StructVar is a container holding a struct and a map of unresolved references inside this struct | ||
| type StructVar struct { | ||
| Value any `json:"value"` | ||
| Value json.RawMessage `json:"value"` |
ContributorAuthor
There was a problem hiding this comment.
then it's "value" on the wire and ValueJson in go, a bit of mismatch.
| }, | ||
| ) | ||
| if err != nil { | ||
| panic(err) |
Contributor
There was a problem hiding this comment.
require no error instead of panic? Panic can cause other tests in the test suite to bail out.
| logdiag.LogError(ctx, errors.New("--readplan is only supported with direct engine (set DATABRICKS_BUNDLE_ENGINE=direct)")) | ||
| return b, stateDesc, root.ErrAlreadyPrinted | ||
| } | ||
| opts.Build = false |
Contributor
There was a problem hiding this comment.
It's worth noting that this means the plan is not portable. Since we do not build here, the build artifacts have to be built during planning.
For example, you cannot have a workflow generate a plan, save it as an artifact, and then have another workflow on a different machine deploy the plan.
ContributorAuthor
There was a problem hiding this comment.
Yes, it's not meant to be portable, it records exact build artifacts you have.
| "$@" | ||
| } | ||
| readplanarg() { |
Contributor
There was a problem hiding this comment.
We could also add this as an environment variable, like DATABRICKS_BUNDLE_PLAN. No need for this in that case.
ContributorAuthor
There was a problem hiding this comment.
we need to include filename, which is different in every test. that's what function does. we also have READPLAN env var.
Contributor
There was a problem hiding this comment.
The environment variable would contain the path to the plan. DATABRICKS_BUNDLE_PLAN=/my/plan
| return selectedDirs - skippedDirs | ||
| } | ||
| func forbiddenEnvSet(envset []string) bool { |
Contributor
There was a problem hiding this comment.
Why do we need this if the readplan tests are only run on direct?
ContributorAuthor
There was a problem hiding this comment.
EnvMatrix by itself does not support exceptions, so it would run terraform with READPLAN as well.
Contributor
There was a problem hiding this comment.
The tests for the plan are mostly direct anyway? Only one test (the error case) is for TF right?
Contributor
There was a problem hiding this comment.
Why can't this be done by overriding EnvMatrix.DATABRICKS_BUNDLE_ENGINE?
ContributorAuthor
There was a problem hiding this comment.
I want to run
- engine=terraform readplan=
- engine=direct readplan=
- engine=direct readplan=1
Not possible with EnvMatrix alone which requires a full matrix.
I'm running TF as well to ensure that requests recorded are reasonable.
| @@ -1,6 +1,6 @@ | |||
| $CLI bundle plan -o json > out.plan_create.$DATABRICKS_BUNDLE_ENGINE.json | |||
| trace $CLI bundle deploy | |||
| $CLI bundle deploy $(readplanarg out.plan_create.direct.json) | |||
Contributor
There was a problem hiding this comment.
Better to keep the trace?
ContributorAuthor
There was a problem hiding this comment.
trace records the argument and that is different in READPLAN variants.
Contributor
There was a problem hiding this comment.
Summary
This PR implements --readplan functionality for the direct engine. Implementation is well-structured with comprehensive acceptance tests.
Issues Found
Missing Test Coverage
- No unit tests for LoadPlanFromFile - Critical deserialization function lacks unit tests
- No unit tests for Load/Save cycle - StructVar serialization round-trip not tested
- No unit tests for InitForApply - Plan loading orchestration not tested
Type Safety Issues
- Load() panic risk - libs/structs/structvar/structvar.go:49 will panic if typ is not a pointer type
- Load() type validation - No validation that typ matches JSON content type
Suggestions
- Add comment in resolveReferences explaining why Load is called
- Enhance CLI version mismatch warning
- Document READPLAN/terraform exclusion constraint
Review generated by reviewbot
Uh oh!
There was an error while loading. Please reload this page.
| return nil // Already loaded | ||
| } | ||
| ptr := reflect.New(typ.Elem()) | ||
| if err := json.Unmarshal(sv.Value, ptr.Interface()); err != nil { |
Contributor
There was a problem hiding this comment.
No validation that unmarshaled type matches typ parameter.
ContributorAuthor
There was a problem hiding this comment.
ptr.Interface() carries the type information, but I agree we can use strict decoder in this case. Updating.
ContributorAuthor
There was a problem hiding this comment.
I've updated and there is a test for it, but FYI strict decoder has no effect on SDK types because of custom marshaller.
denik
disabled auto-merge
December 18, 2025 12:03
Uh oh!
There was an error while loading. Please reload this page.
deco-sdk-taggingBot
added a commit
that referenced
this pull request
Dec 18, 2025
## Release v0.281.0 ### CLI * Fix lakeview publish to default `embed_credentials` to false ([#4066](#4066)) ### Bundles * Add support for configurable catalog/schema for dashboards ([#4130](#4130)) * Pass SYSTEM\_ACCESSTOKEN from env to the Terraform provider ([#4135](#4135)) * `bundle deployment migrate`: when running `bundle plan` propagate `-var` arguments. * engine/direct: New option --plan to `bundle deploy` to deploy previously saved plan (saved plan with `bundle plan -o json`) ([#4134](#4134)) * engine/direct: Fix dependency-ordered deletion by persisting depends\_on in state ([#4105](#4105)) ### Dependency updates * Upgrade Go SDK to 0.94.0 ([#4148](#4148)) * Upgrade Terraform provider to 1.100.0 ([#4150](#4150))
eng-dev-ecosystem-bot
commented
Dec 18, 2025
Collaborator
Commit: 8f66fb8
43 interesting tests: 14 KNOWN, 12 flaky, 10 FAIL, 6 RECOVERED, 1 SKIP
Top 50 slowest tests (at least 2 minutes):
|
denik added a commit
that referenced
this pull request
Dec 29, 2025
## Why In #4134 I added special case for engine/readplan vars directly into acceptance_test.go. This generalizes that, making this feature available to other env var combinations.
denik added a commit
that referenced
this pull request
Mar 6, 2026
## Why It must not be used, all actions must be performed based on plan, not config. Needed for "bundle deploy --plan" to work reliably (#4134). Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
denik added a commit
that referenced
this pull request
May 20, 2026
… plans (#4134) ## Changes New `--readplan` option to `bundle deploy` that allows executing previously saved plan (`bundle plan -o json > myplan.json`). In this case, configuration to deploy is taken from the plan, not from databricks.yml. ## Why Enable workflow where you can review the changes before that are deployed. It's also faster, does not repeat plan operation as part of deploy. Note, currently it still requires to pass -t/--target. It also parses databricks.yml and goes through Initialize phase because that where get host, remote folders etc. In the future we might store those on plan so that we can run this without databricks.yml at all. Implements #4094 ## Tests Some tests are modified to run with and without `--readplan`. There is env var matrix variable READPLAN that controls this. Additional test helper readplanarg that expands into --readplan based on env var. Test runner knows that terraform does not support it and skips that configuration.
denik pushed a commit
that referenced
this pull request
May 20, 2026
## Release v0.281.0 ### CLI * Fix lakeview publish to default `embed_credentials` to false ([#4066](#4066)) ### Bundles * Add support for configurable catalog/schema for dashboards ([#4130](#4130)) * Pass SYSTEM\_ACCESSTOKEN from env to the Terraform provider ([#4135](#4135)) * `bundle deployment migrate`: when running `bundle plan` propagate `-var` arguments. * engine/direct: New option --plan to `bundle deploy` to deploy previously saved plan (saved plan with `bundle plan -o json`) ([#4134](#4134)) * engine/direct: Fix dependency-ordered deletion by persisting depends\_on in state ([#4105](#4105)) ### Dependency updates * Upgrade Go SDK to 0.94.0 ([#4148](#4148)) * Upgrade Terraform provider to 1.100.0 ([#4150](#4150))
denik added a commit
that referenced
this pull request
May 20, 2026
## Why In #4134 I added special case for engine/readplan vars directly into acceptance_test.go. This generalizes that, making this feature available to other env var combinations.
denik added a commit
that referenced
this pull request
May 20, 2026
## Why It must not be used, all actions must be performed based on plan, not config. Needed for "bundle deploy --plan" to work reliably (#4134). Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
New
--readplanoption tobundle deploythat allows executing previously saved plan (bundle plan -o json > myplan.json). In this case, configuration to deploy is taken from the plan, not from databricks.yml.Why
Enable workflow where you can review the changes before that are deployed. It's also faster, does not repeat plan operation as part of deploy.
Note, currently it still requires to pass -t/--target. It also parses databricks.yml and goes through Initialize phase because that where get host, remote folders etc. In the future we might store those on plan so that we can run this without databricks.yml at all.
Implements #4094
Tests
Some tests are modified to run with and without
--readplan. There is env var matrix variable READPLAN that controls this. Additional test helper readplanarg that expands into --readplan based on env var. Test runner knows that terraform does not support it and skips that configuration.