Skip to content

direct: Handle data_security_mode aliases for clusters #3839

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+34 −39
Open

direct: Handle data_security_mode aliases for clusters #3839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6683d6b
direct: Handle data_security_mode aliases for clusters
andrewnester Oct 29, 2025
9e08d30
added comment
andrewnester Oct 29, 2025
f869a26
on;y for remote diff
andrewnester Nov 3, 2025
1e01f6f
Merge branch 'main' into direct/fix-cluster-aliases
andrewnester Nov 4, 2025
bd987e0
remove old files
andrewnester Nov 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions acceptance/bundle/resources/clusters/deploy/data_security_mode/out.direct.txt
View file
Open in desktop

This file was deleted.

5 changes: 0 additions & 5 deletions acceptance/bundle/resources/clusters/deploy/data_security_mode/out.plan.direct.txt
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions acceptance/bundle/resources/clusters/deploy/data_security_mode/out.plan.terraform.txt
View file
Open in desktop

This file was deleted.

12 changes: 0 additions & 12 deletions acceptance/bundle/resources/clusters/deploy/data_security_mode/out.terraform.txt
View file
Open in desktop

This file was deleted.

15 changes: 15 additions & 0 deletions acceptance/bundle/resources/clusters/deploy/data_security_mode/output.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,19 @@

>>> [CLI] bundle deploy
Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/[UNIQUE_NAME]/files...
Deploying resources...
Updating deployment state...
Deployment complete!

>>> [CLI] bundle plan
Plan: 0 to add, 0 to change, 0 to delete, 1 unchanged

>>> [CLI] bundle deploy
Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/[UNIQUE_NAME]/files...
Deploying resources...
Updating deployment state...
Deployment complete!

>>> [CLI] bundle destroy --auto-approve
The following resources will be deleted:
delete cluster test_cluster
Expand Down
8 changes: 3 additions & 5 deletions acceptance/bundle/resources/clusters/deploy/data_security_mode/script
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ cleanup() {
}
trap cleanup EXIT

trace $CLI bundle deploy > out.$DATABRICKS_BUNDLE_ENGINE.txt 2>&1

trace $CLI bundle plan >> out.plan.$DATABRICKS_BUNDLE_ENGINE.txt 2>&1

trace errcode $CLI bundle deploy >> out.$DATABRICKS_BUNDLE_ENGINE.txt 2>&1
trace $CLI bundle deploy
trace $CLI bundle plan
trace $CLI bundle deploy
18 changes: 16 additions & 2 deletions bundle/direct/dresources/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,13 +119,27 @@ func (r *ResourceCluster) DoDelete(ctx context.Context, id string) error {
return r.client.Clusters.PermanentDeleteByClusterId(ctx, id)
}

func (r *ResourceCluster) ClassifyChange(change structdiff.Change, remoteState *compute.ClusterDetails, _ bool) (deployplan.ActionType, error) {
func (r *ResourceCluster) ClassifyChange(change structdiff.Change, remoteState *compute.ClusterDetails, isLocal bool) (deployplan.ActionType, error) {
changedPath := change.Path.String()
if changedPath == "data_security_mode" && !isLocal {
// We do change skip here in the same way TF provider does suppress diff if the alias is used.
// https://github.com/databricks/terraform-provider-databricks/blob/main/clusters/resource_cluster.go#L109-L117
if change.Old == compute.DataSecurityModeDataSecurityModeStandard && change.New == compute.DataSecurityModeUserIsolation {
return deployplan.ActionTypeSkip, nil
}
if change.Old == compute.DataSecurityModeDataSecurityModeDedicated && change.New == compute.DataSecurityModeSingleUser {
return deployplan.ActionTypeSkip, nil
}
if change.Old == compute.DataSecurityModeDataSecurityModeAuto && (change.New == compute.DataSecurityModeSingleUser || change.New == compute.DataSecurityModeUserIsolation) {
Copy link
Contributor

@denik denik Oct 29, 2025

Choose a reason for hiding this comment

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

I suppose it's better to skip it than to remap these in RemapState. Separate question (follow up PR) - should we also return a reason in ClassifyChanges? So we can show reason="alias" next to action="skip" in the plan.

Copy link
Contributor Author

@andrewnester andrewnester Oct 29, 2025

Choose a reason for hiding this comment

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

Tbh I don't have a preference on adding the reason

Copy link
Contributor

@pietern pietern Nov 5, 2025

Choose a reason for hiding this comment

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

Separate reason in the plan would be nice. Can be separate PR.

return deployplan.ActionTypeSkip, nil
}
}

// Always update if the cluster is not running.
if remoteState.State != compute.StateRunning {
return deployplan.ActionTypeUpdate, nil
}

changedPath := change.Path.String()
if changedPath == "num_workers" || strings.HasPrefix(changedPath, "autoscale") {
return deployplan.ActionTypeResize, nil
}
Expand Down