Added support for lifecycle prevent_destroy option #3448

andrewnester · 2025-08-20T13:14:47Z

Changes

Added support for lifecycle prevent_destroy option

In TF, if lifecycle.prevent_destroy is set to true, terraform plan will fail with a corresponding error. That's why we can't really reuse the same code logic for both direct and TF.

Direct implementation mimics the TF one.

A. When lifecycle.prevent_destroy is set to true:

bundle destroy will fail
if there are any configuration changes that recreate the resource, bundle deploy will fail

B. When lifecycle.prevent_destroy is switched to true destroy and deploy will succeed

C. When the whole resource is removed from bundle confiuguration, destroy and deploy will succeed

Why

Allows specifying resources as non-destructible, which is already supported by TF

Tests

Added accceptance tests

bundle/config/resources/schema.go

acceptance/bundle/lifecycle/prevent_deploy/databricks.yml

eng-dev-ecosystem-bot · 2025-08-20T13:34:22Z

Run: 17584931660

	Env	✅pass	🔄flaky	🙈skip
✅	aws linux	311		525
✅	aws windows	312		524
🔄	aws-ucws linux	421	2	423
✅	aws-ucws windows	424		422
✅	azure linux	311		524
✅	azure windows	312		523
✅	azure-ucws linux	423		422
✅	azure-ucws windows	424		421
✅	gcp linux	310		526
✅	gcp windows	311		525

Test Name	aws-ucws linux
TestAccept	🔄flaky
TestAccept/bundle/deployment/bind/job/job-spark-python-task	🔄flaky

pietern · 2025-09-08T11:54:02Z

acceptance/bundle/lifecycle/prevent-destroy/databricks.yml

+        schema: test-schema
+        catalog: main
+
+<<: *pipeline_base


Curious: why use the YAML anchors if they aren't used anywhere?

I use them in the test to remove the whole resource from the configuration

Alternative, could be easier to manage and follow than anchors:

have each resource in their own yaml

main databricks includes "resources/*.yml"

removing resources: rm resources/filename.yml
duplicating resource: cp resource/a.yml resource/b.yml, followed by update_file.py

I don't have a preference here. If you think this makes it easier to understand, I'll change it to that

Done here 65bc5dd

acceptance/bundle/lifecycle/prevent-destroy/script

bundle/config/resources/database_catalog.go

bundle/internal/schema/annotations.yml

bundle/internal/schema/annotations_openapi_overrides.yml

bundle/phases/plan.go

…uct (#3571) ## Changes Extract common resource fields into a separate embedded struct `BaseResource` ## Why Based on feedback from #3448 (comment) ## Tests Existing tests pass

denik · 2025-09-09T12:27:24Z

bundle/config/resources/clusters.go


 	Permissions []ClusterPermission `json:"permissions,omitempty"`
-
-	compute.ClusterSpec


Curious about this move -- was it needed for this PR to work?

No, just grouping them together, it's less readable when these structs are separated by custom fields

bundle/phases/plan.go

denik · 2025-09-09T12:35:03Z

libs/structwalk/walktype_test.go

 	testStruct(t,
 		reflect.TypeOf(config.Root{}),
-		3800, 4200, // 4001 at the time of the update
+		4000, 4300, // 4003 at the time of the update


FYI, it's okay to set higher bound to 10k or something. IT's a sanity check but I did not intend it to be updated frequently.

denik · 2025-09-09T12:36:32Z

acceptance/bundle/lifecycle/prevent-destroy/databricks.yml

+  lifecycle:
+    prevent_destroy: true
+
+pipeline: &pipeline_base


Would it work if we had large databricks.yml with all the resources? So we don't loop but test all at once. This requires all error messages to be printed rather than the first one, which may be a good idea anyway.

acceptance/bundle/lifecycle/prevent-destroy/out.terraform.txt

denik · 2025-09-09T13:09:07Z

acceptance/bundle/lifecycle/prevent-destroy/databricks.yml

+  lifecycle:
+    prevent_destroy: true
+
+pipeline: &pipeline_base


It could work, but if you add a new type of resource, you need to remember to add it to this databricks.yml file.

It depends. If existing resource provide enough variety and implementation is general (it is) we can say we've covered our ground.

The unit test I added will fail if the new resource does not handle prevent destroy correctly.

It only covers TF, right?

Overall I agree we can proceed without full coverage, but it would be interesting to cover >1 resource to see what kind of error messages we get. Do we get them all? One at random?

denik · 2025-09-09T13:11:29Z

acceptance/bundle/lifecycle/prevent-destroy/databricks.yml

+        schema: test-schema
+        catalog: main
+
+<<: *pipeline_base


Alternative, could be easier to manage and follow than anchors:

have each resource in their own yaml

main databricks includes "resources/*.yml"

removing resources: rm resources/filename.yml
duplicating resource: cp resource/a.yml resource/b.yml, followed by update_file.py

denik · 2025-09-16T07:56:26Z

bundle/phases/plan.go

+		// If there is no prevent_destroy, skip
+		preventDestroyV, err := dyn.GetByPath(root, path)
+		if err != nil {
+			return nil


Should this be a return or continue? Looks like we going to miss actions that come afterwards?

Added support for lifecycle prevent_deploy option

2ddb691

andrewnester temporarily deployed to test-trigger-is August 20, 2025 13:14 — with GitHub Actions Inactive

denik reviewed Aug 20, 2025

View reviewed changes

bundle/config/resources/schema.go Outdated Show resolved Hide resolved

acceptance/bundle/lifecycle/prevent_deploy/databricks.yml Outdated Show resolved Hide resolved

added support for prevent_destroy in direct

637a057

andrewnester requested a review from denik September 2, 2025 10:44

andrewnester marked this pull request as ready for review September 2, 2025 10:44

andrewnester requested review from anton-107, pietern and shreyas-goenka as code owners September 2, 2025 10:44

Merge branch 'main' into feature/prevent-destroy

251284c

andrewnester temporarily deployed to test-trigger-is September 2, 2025 10:46 — with GitHub Actions Inactive

fixes

fd67d51

andrewnester temporarily deployed to test-trigger-is September 2, 2025 11:27 — with GitHub Actions Inactive

Merge branch 'main' into feature/prevent-destroy

d8c5ecb

andrewnester temporarily deployed to test-trigger-is September 2, 2025 12:40 — with GitHub Actions Inactive

fixes

cc79894

andrewnester temporarily deployed to test-trigger-is September 2, 2025 12:56 — with GitHub Actions Inactive

removed unnecessary test

9dc81ce

andrewnester temporarily deployed to test-trigger-is September 2, 2025 13:05 — with GitHub Actions Inactive

Merge branch 'main' into feature/prevent-destroy

877deb3

andrewnester temporarily deployed to test-trigger-is September 2, 2025 14:47 — with GitHub Actions Inactive

update schema

04b059d

andrewnester temporarily deployed to test-trigger-is September 2, 2025 14:48 — with GitHub Actions Inactive

omitempty

7e5b1fb

andrewnester temporarily deployed to test-trigger-is September 2, 2025 14:54 — with GitHub Actions Inactive

fixed schema

db9bafa

andrewnester temporarily deployed to test-trigger-is September 2, 2025 15:01 — with GitHub Actions Inactive

make python

71d8981

andrewnester temporarily deployed to test-trigger-is September 2, 2025 15:06 — with GitHub Actions Inactive

Merge branch 'main' into feature/prevent-destroy

c066038

andrewnester temporarily deployed to test-trigger-is September 8, 2025 11:13 — with GitHub Actions Inactive

codegen python

a32e69a

andrewnester temporarily deployed to test-trigger-is September 8, 2025 11:14 — with GitHub Actions Inactive

pietern reviewed Sep 8, 2025

View reviewed changes

fixes

109d453

andrewnester temporarily deployed to test-trigger-is September 8, 2025 12:43 — with GitHub Actions Inactive

andrewnester mentioned this pull request Sep 8, 2025

refactor: Extract common resource fields into a separate embedded struct #3571

Merged

andrewnester requested a review from pietern September 8, 2025 13:07

Merge branch 'main' into feature/prevent-destroy

1165def

andrewnester temporarily deployed to test-trigger-is September 9, 2025 11:30 — with GitHub Actions Inactive

fixes

db34f10

andrewnester temporarily deployed to test-trigger-is September 9, 2025 11:43 — with GitHub Actions Inactive

regenerate python

9118ceb

andrewnester temporarily deployed to test-trigger-is September 9, 2025 11:46 — with GitHub Actions Inactive

denik reviewed Sep 9, 2025

View reviewed changes

do not use isDestroy flag and read prevent_destroy directly

fe565f5

andrewnester temporarily deployed to test-trigger-is September 9, 2025 12:58 — with GitHub Actions Inactive

andrewnester requested a review from denik September 9, 2025 12:59

denik approved these changes Sep 9, 2025

View reviewed changes

andrewnester added 2 commits September 9, 2025 15:50

add schemas to test

65bc5dd

show all failed resources

42faa7b

andrewnester temporarily deployed to test-trigger-is September 9, 2025 13:53 — with GitHub Actions Inactive

andrewnester enabled auto-merge September 9, 2025 13:54

andrewnester added this pull request to the merge queue Sep 9, 2025

Merged via the queue into main with commit e5a4da5 Sep 9, 2025
19 checks passed

andrewnester deleted the feature/prevent-destroy branch September 9, 2025 14:34

denik reviewed Sep 16, 2025

View reviewed changes


		Permissions []ClusterPermission `json:"permissions,omitempty"`

		compute.ClusterSpec

Added support for lifecycle prevent_destroy option #3448

Added support for lifecycle prevent_destroy option #3448

Uh oh!

Conversation

andrewnester commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Why

Tests

Uh oh!

Uh oh!

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

denik Sep 9, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

andrewnester commented Aug 20, 2025 •

edited

Loading

eng-dev-ecosystem-bot commented Aug 20, 2025 •

edited

Loading

denik Sep 9, 2025 •

edited

Loading