Generalize terraform to bundle conversion, prepare for direct deployment by denik · Pull Request #3134 · databricks/cli

denik · 2025-06-27T09:16:48Z

Changes

Introduce ExportedResourcesMap type that is deployment-method neutral. Modify terraform.ParseResourcesState to return this type instead of struct matching terraform state format.
Modify terraform.TerraformToBundle() to rely on this new format.
Make terraform.TerraformToBundle() handle all resources automatically and operate on dynamic bundle structure.
Remove ModifedStatusUpdated constant and usage in apps.
Fix Secret scopes resource to properly set modified_status. Previously it was always "created".

Why

ExportedResourcesMap allows implementing terraform.Load() and thus bundle summary/run/open with direct deployment method (#2926) as it is not tied to terraform resource names or internal state structure.

When implementing new resource, we no longer need to add an entry in TerraformToBundle, this both saves time and avoids bugs.

ModifiedStatusUpdated is removed because it's not implemented. Only app resource ever sets it and it sets it always, not based on the actual situation.

Tests

summary/modified_status is expanded to handle both cases where resources: block is present and not present (regression during development of this PR).

## Tests For jobs, dashboards, secret scopes check that "id" and "modified_status" fields are set correctly by printing more output from "bundle summary". This highlights the issue with secret scopes - modified_status is not correct there. ## Why To support refactoring (#3134) and adding support for direct deployment to bundle summary.

eng-dev-ecosystem-bot · 2025-06-27T10:01:23Z

Run: 16020926487

	Env	✅‌pass	❌‌FAIL	🔄‌flaky	🙈‌skip
❌‌	aws linux	293	2		371
❌‌	aws windows	294	3	1	370
✅‌	aws-ucws linux	399			271
✅‌	aws-ucws windows	400			270
🔄‌	azure linux	295		2	370
✅‌	azure windows	298			369
🔄‌	azure-ucws linux	393		8	268
🔄‌	azure-ucws windows	399		3	267
✅‌	gcp linux	296			372
✅‌	gcp windows	297			371

14 failing tests:

Test Name	aws linux	aws windows	azure linux	azure-ucws linux	azure-ucws windows
TestAccept	✅‌pass	❌‌FAIL	🔄‌flaky	✅‌pass	🔄‌flaky
TestAccept/bundle/deploy/dashboard/detect-change	✅‌pass	✅‌pass	🔄‌flaky	🔄‌flaky	🔄‌flaky
TestAccept/bundle/deploy/dashboard/nested-folders	✅‌pass	✅‌pass	✅‌pass	🔄‌flaky	✅‌pass
TestAccept/bundle/deploy/dashboard/simple	✅‌pass	✅‌pass	✅‌pass	🔄‌flaky	✅‌pass
TestAccept/bundle/deploy/dashboard/simple_outside_bundle_root	✅‌pass	✅‌pass	✅‌pass	🔄‌flaky	✅‌pass
TestAccept/bundle/deploy/dashboard/simple_syncroot	✅‌pass	✅‌pass	✅‌pass	🔄‌flaky	✅‌pass
TestAccept/bundle/deployment/bind/dashboard	✅‌pass	✅‌pass	✅‌pass	🔄‌flaky	✅‌pass
TestAccept/bundle/deployment/bind/dashboard/recreation	✅‌pass	✅‌pass	✅‌pass	🔄‌flaky	✅‌pass
TestAccept/bundle/resources/pipelines	✅‌pass	❌‌FAIL	✅‌pass	✅‌pass	✅‌pass
TestAccept/bundle/resources/pipelines/DATABRICKS_CLI_DEPLOYMENT=direct-exp	✅‌pass	❌‌FAIL	✅‌pass	✅‌pass	✅‌pass
TestDashboardAssumptions_WorkspaceImport	✅‌pass	✅‌pass	✅‌pass	🔄‌flaky	🔄‌flaky
TestFetchRepositoryInfoAPI_FromRepo	❌‌FAIL	✅‌pass	✅‌pass	✅‌pass	✅‌pass
TestLock	❌‌FAIL	✅‌pass	✅‌pass	✅‌pass	✅‌pass
TestSyncEnsureRemotePathIsUsableIfRepoExists	✅‌pass	🔄‌flaky	✅‌pass	✅‌pass	✅‌pass

denik · 2025-06-30T10:27:49Z

~~Fails this test #3142 -- will investigate.~~

## Changes Add an acceptance test covering modified_status lifecycle: "created" -> "" -> "deleted". ## Why To support refactoring such as #3134 and direct deployment method support in summary. --------- Co-authored-by: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com>

pietern

Minor comment-related-comments.

Nice work, this simplifies things a lot.

pietern · 2025-07-02T08:39:32Z


 const (
 	ModifiedStatusCreated ModifiedStatus = "created"
-	ModifiedStatusUpdated ModifiedStatus = "updated"


@andrewnester This was only set by apps (in #1928). Did you use it for anything or just completeness?

No, not really, only for summary output but this is handled differently now

pietern · 2025-07-02T08:40:48Z

- }
+func ensureMap(v dyn.Value, path dyn.Path) (dyn.Value, error) {
+	item, _ := dyn.GetByPath(v, path)
+	if !item.IsValid() {


Panic or error if it is not a map to be defensive?

If any parents are of invalid type (should not be possible), SetByPath later will return an error. This seems enough.

pietern · 2025-07-02T08:44:00Z

+ idPath := dyn.Path{dyn.Key("id")}
+ statusPath := dyn.Path{dyn.Key("modified_status")}
+ id, _ := dyn.GetByPath(inner, idPath)
+ status, _ := dyn.GetByPath(inner, statusPath)


Can you include a comment here explaining that IDs are set for resources that have already been deployed and that the status is set for resources that were deleted? The remaining resources are yet to be deployed.

pietern · 2025-07-02T08:49:06Z

-	for _, src := range config.Resources.Apps {
- if src.ModifiedStatus == "" {
- src.ModifiedStatus = resources.ModifiedStatusCreated
+func TerraformToBundle(ctx context.Context, state ExportedResourcesMap, config *config.Root) error {


Reading the rest of this PR, I assume you want to extract this into a separate generic package?

It will be move to statemgmt #3161 and extend to support direct deployment.

pietern · 2025-07-02T08:51:45Z

+
+ switch groupName {
+ case "apps":
+ fallthrough


TIL, didn't know Go supported this. I would prefer duplication though, because updating secret scopes in isolation now affects apps as well. Duplication keeps it simple.

Fair enough, in this case duplication is okay as well ca58ff3

+1 to duplication

pietern · 2025-07-02T08:52:32Z

+// This is in a separate package to avoid import cycles because it is imported by both terraform and statemgmt.
+package resourcestate
+
+type ResourceState struct {


Please add type comment.

pietern · 2025-07-02T08:53:00Z


 func localStateFile(t *testing.T, ctx context.Context, b *bundle.Bundle) string {
-	dir, err := tf.Dir(ctx, b)
+	dir, err := b.CacheDir(ctx, "terraform")


Leftover from import cycle?

There was an import cycle between statemgmt and terraform but it was resolved by creating statemgmt/resourcestate package.
Still nice not to add this dependency if we can avoid, especially given that it's just a test.

pietern

Btw, please take a look at the PR title and summary.

Some names are out of sync with the actual implementation.

andrewnester · 2025-07-02T09:12:09Z

+
+ switch groupName {
+ case "apps":
+ fallthrough


+1 to duplication

## Changes Move terraform.Load to statemgmt.Load Move terraform.TerraformToBundle to statemgmt.StateToBundle. Move related unit tests. Pure move, no functional changes. Stacked on top of #3134 ## Why Preparation for making it work with direct deployment.

## Changes - Fix resource deletion in direct backend: resource removed from the config are now deleted. - Implement bundle summary/run/open in direct backend. ## Depends on - #3134 - #3161 ## Tests Enabled a bunch of tests & added more precise comments on remaining disabled.

## Tests For jobs, dashboards, secret scopes check that "id" and "modified_status" fields are set correctly by printing more output from "bundle summary". This highlights the issue with secret scopes - modified_status is not correct there. ## Why To support refactoring (#3134) and adding support for direct deployment to bundle summary.

## Changes Add an acceptance test covering modified_status lifecycle: "created" -> "" -> "deleted". ## Why To support refactoring such as #3134 and direct deployment method support in summary. --------- Co-authored-by: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com>

…ent (#3134) ## Changes - Introduce ExportedResourcesMap type that is deployment-method neutral. Modify terraform.ParseResourcesState to return this type instead of struct matching terraform state format. - Modify terraform.TerraformToBundle() to rely on this new format. - Make terraform.TerraformToBundle() handle all resources automatically and operate on dynamic bundle structure. - Remove ModifedStatusUpdated constant and usage in apps. - Fix Secret scopes resource to properly set modified_status. Previously it was always "created". ## Why ExportedResourcesMap allows implementing terraform.Load() and thus bundle summary/run/open with direct deployment method (#2926) as it is not tied to terraform resource names or internal state structure. When implementing new resource, we no longer need to add an entry in TerraformToBundle, this both saves time and avoids bugs. ModifiedStatusUpdated is removed because it's not implemented. Only app resource ever sets it and it sets it always, not based on the actual situation. ## Tests summary/modified_status is expanded to handle both cases where resources: block is present and not present (regression during development of this PR).

## Changes Move terraform.Load to statemgmt.Load Move terraform.TerraformToBundle to statemgmt.StateToBundle. Move related unit tests. Pure move, no functional changes. Stacked on top of #3134 ## Why Preparation for making it work with direct deployment.

## Changes - Fix resource deletion in direct backend: resource removed from the config are now deleted. - Implement bundle summary/run/open in direct backend. ## Depends on - #3134 - #3161 ## Tests Enabled a bunch of tests & added more precise comments on remaining disabled.

denik requested review from andrewnester, anton-107, pietern and shreyas-goenka as code owners June 27, 2025 09:16

denik temporarily deployed to test-trigger-is June 27, 2025 09:16 — with GitHub Actions Inactive

denik changed the title ~~Denik/improve terraform load~~Rewrite terraform.Load() to use dyn value and not depend on terraformJun 27, 2025

denik changed the title ~~Rewrite terraform.Load() to use dyn value and not depend on terraform~~Generic and independent from terraform state to bundle conversionJun 27, 2025

denik mentioned this pull request Jun 27, 2025
Expand coverage for "bundle summary" #3130
Merged

Base automatically changed from denik/TEST_DEFAULT_WAREHOUSE_ID to mainJune 27, 2025 09:34

denik force-pushed the denik/improve-terraform-load branch from 543d521 to 6ad23b8Compare June 27, 2025 09:49

denik temporarily deployed to test-trigger-is June 27, 2025 09:49 — with GitHub Actions Inactive

denik force-pushed the denik/improve-terraform-load branch from 6ad23b8 to a2650a4Compare June 27, 2025 13:08

denik temporarily deployed to test-trigger-is June 27, 2025 13:08 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is June 30, 2025 07:45 — with GitHub Actions Inactive

denik mentioned this pull request Jun 30, 2025
Add test for modified_status in bundle summary #3142
Merged

denik marked this pull request as draft June 30, 2025 10:27

denik force-pushed the denik/improve-terraform-load branch from 9360615 to 7e52eb4Compare June 30, 2025 14:26

denik temporarily deployed to test-trigger-is June 30, 2025 14:26 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is June 30, 2025 14:36 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is June 30, 2025 15:09 — with GitHub Actions Inactive

denik marked this pull request as ready for review June 30, 2025 15:09

denik force-pushed the denik/improve-terraform-load branch from 45b54a5 to 7139d58Compare July 1, 2025 08:06

denik temporarily deployed to test-trigger-is July 1, 2025 08:06 — with GitHub Actions Inactive

denik force-pushed the denik/improve-terraform-load branch from 7139d58 to 1808a26Compare July 1, 2025 09:12

denik temporarily deployed to test-trigger-is July 1, 2025 09:12 — with GitHub Actions Inactive

denik force-pushed the denik/improve-terraform-load branch from 1808a26 to 29edbecCompare July 1, 2025 09:33

denik added 8 commits July 1, 2025 17:43

wip
5496fe5

break dependency of statemgmt on deploy/terraform
eec31a2

fixes
58cb183

add state_export.go
9515ab4

update secret scopes test
485ac75

move ResourceState to its own package
6dea1a6

create resources.X if it does not exist
3fa312e

expand modified_status to test absence of resources: as well
6d94ba1

denik force-pushed the denik/improve-terraform-load branch from 878520f to fd3c594Compare July 1, 2025 15:43

denik temporarily deployed to test-trigger-is July 1, 2025 15:43 — with GitHub Actions Inactive

denik mentioned this pull request Jul 1, 2025
[direct] Implement bundle summary/run/open and resource deletion #3162
Merged

denik added 2 commits July 2, 2025 09:08

restore empty resources{}
068c940

create resources & resources.section if missing
cbe7fc9

denik force-pushed the denik/improve-terraform-load branch from fd3c594 to cbe7fc9Compare July 2, 2025 07:08

denik temporarily deployed to test-trigger-is July 2, 2025 07:09 — with GitHub Actions Inactive

pietern approved these changes Jul 2, 2025
View reviewed changes

pietern reviewed Jul 2, 2025
View reviewed changes

add comments and remove fallthrough
ca58ff3

denik temporarily deployed to test-trigger-is July 2, 2025 09:06 — with GitHub Actions Inactive

denik changed the title ~~Generic and independent from terraform state to bundle conversion~~Generalize terraform to bundle conversion, prepare for direct deploymentJul 2, 2025

andrewnester approved these changes Jul 2, 2025
View reviewed changes

Comment threadbundle/deploy/terraform/util.go Outdated

switch groupName {
case "apps":
fallthrough
andrewnesterJul 2, 2025
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to duplication

denik merged commit 8442924 into mainJul 2, 2025

denik deleted the denik/improve-terraform-load branch July 2, 2025 09:21

Uh oh!

Conversation

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

Uh oh!

Changes

Why

Tests

Uh oh!

eng-dev-ecosystem-bot commented Jun 27, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

pietern left a comment

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!

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!

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!

pietern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

denik commented Jun 27, 2025•
edited
Loading

eng-dev-ecosystem-bot commented Jun 27, 2025•
edited
Loading

denik commented Jun 30, 2025•
edited
Loading