-
Notifications
You must be signed in to change notification settings - Fork 112
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.
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
base: main
Are you sure you want to change the base?
Conversation
| 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
51 failing tests:
|
a1a6c48 to
9e08d30
Compare
pietern
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are they mapped exclusively into the new values? (no possibility we need to flip?)
@denik Other concerns with this?
| 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) { |
There was a problem hiding this comment.
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.
Changes
Handle data_security_mode aliases for clusters
Why
To be in line with TF provider behaviour, we skip diff if it's just a change of the alias
https://github.com/databricks/terraform-provider-databricks/blob/main/clusters/resource_cluster.go#L109-L117
Tests
Added an acceptance test