[Fix] Fix drift in `databricks_cluster` when `is_single_node` is set to `true` #4416

ashenm · 2025-01-17T16:20:43Z

Changes

Tests

make test run locally
relevant change in docs/ folder
covered with integration tests in internal/acceptance
using Go SDK
using TF Plugin Framework

In addition to the unit tests, deployed provider locally with following resource definition and verified diff

resource "databricks_cluster" "single_node" {
  spark_version      = "15.4.x-scala2.12"
  single_user_name   = "[email protected]"
  runtime_engine     = "STANDARD"
  node_type_id       = "r5.large"
  kind               = "CLASSIC_PREVIEW"
  is_single_node     = true
  data_security_mode = "SINGLE_USER"
  cluster_name       = upper("demo-single-node")

  custom_tags = {
    "owner" = "ashenm"
  }
  autotermination_minutes = 10
}

ashenm · 2025-01-17T16:22:24Z

@alexott please take a stab at it when u get a slot

alexott · 2025-01-17T16:29:40Z

Please add tests for it - we have example of test for CustomizeDiff in resource_sql_table

alexott · 2025-01-20T09:59:35Z

@mgyucht given the work on porting clusters to the plugin framework - do we need this PR?

mgyucht · 2025-01-20T14:36:14Z

That work may take a bit longer than we thought to finish, so I'm OK merging incremental improvements like this.

mgyucht

Implementation seems OK. Several comments can be addressed.

Please include any testing you did in the PR description.

Can you add a unit test to verify that the diff is as expected? Create a new test, and use a qa.ResourceFixture where you supply the HCL, InstanceState, and ExpectedDiff fields, in addition to the Resource field. Take a look at catalog/resource_sql_table_test.go TestResourceSqlTable_Diff_ExistingResource for an example of how to do this. Add your test to clusters/resource_cluster_test.go.

mgyucht · 2025-01-20T14:37:26Z

clusters/resource_cluster.go

+		new, okNew := n.(map[string]interface{})
+
+		if !okNew || !okOld {
+			return fmt.Errorf("internal type casting error")


Can you include the actual type of o/n here with %T?

mgyucht · 2025-01-20T14:54:13Z

clusters/resource_cluster.go

@@ -48,6 +54,43 @@ func ResourceCluster() common.Resource {
 	}
 }

+func singleNodeClusterChangesCustomizeDiff(d *schema.ResourceDiff) error {


In general, let's include documentation comments for any methods or complex logic. Here's a suggestion:

Suggested change

func singleNodeClusterChangesCustomizeDiff(d *schema.ResourceDiff) error {

// singleNodeClusterChangesCustomizeDiff adjusts the plan when `is_single_node` is `true`. When this happens,

// the API automatically sets the `ResourceClass` key in `custom_tags` and two other keys in the `spark_conf`.

// If the user hasn't set these explicitly in their config, the plan marks these keys for removal.

// This method copies the values for these keys from state to the plan.

// This needs to be done in addition to setting these attributes as computed; otherwise, this customization

// won't take effect for users who have set additional `spark_conf` or `custom_tags`.

func singleNodeClusterChangesCustomizeDiff(d *schema.ResourceDiff) error {

mgyucht · 2025-01-20T14:58:19Z

clusters/resource_cluster.go

+		Delete: resourceClusterDelete,
+		Schema: clusterSchema,
+		CustomizeDiff: func(ctx context.Context, d *schema.ResourceDiff) error {
+			if isSingleNode, _ := d.GetOk("is_single_node"); isSingleNode.(bool) {


Potentially avoid interface conversion panic.

Suggested change

if isSingleNode, _ := d.GetOk("is_single_node"); isSingleNode.(bool) {

if isSingleNode, ok := d.GetOk("is_single_node"); ok && isSingleNode.(bool) {

Nice catch! Fixed 🙌

ashenm · 2025-01-21T08:06:38Z

clusters/resource_cluster_test.go

+			expectedDiff["default_tags.%"] = &terraform.ResourceAttrDiff{Old: "", New: "", NewComputed: true, NewRemoved: false, RequiresNew: false, Sensitive: false}
+			expectedDiff["driver_instance_pool_id"] = &terraform.ResourceAttrDiff{Old: "", New: "", NewComputed: true, NewRemoved: false, RequiresNew: false, Sensitive: false}
+			expectedDiff["driver_node_type_id"] = &terraform.ResourceAttrDiff{Old: "", New: "", NewComputed: true, NewRemoved: false, RequiresNew: false, Sensitive: false}
+			expectedDiff["enable_elastic_disk"] = &terraform.ResourceAttrDiff{Old: "", New: "", NewComputed: true, NewRemoved: false, RequiresNew: false, Sensitive: false}
+			expectedDiff["enable_local_disk_encryption"] = &terraform.ResourceAttrDiff{Old: "", New: "", NewComputed: true, NewRemoved: false, RequiresNew: false, Sensitive: false}
+			expectedDiff["state"] = &terraform.ResourceAttrDiff{Old: "", New: "", NewComputed: true, NewRemoved: false, RequiresNew: false, Sensitive: false}
+			expectedDiff["url"] = &terraform.ResourceAttrDiff{Old: "", New: "", NewComputed: true, NewRemoved: false, RequiresNew: false, Sensitive: false}
+
+			instanceState := testCase.instanceState
+			instanceState["cluster_id"] = "abc"
+			instanceState["autotermination_minutes"] = "60"
+			instanceState["cluster_name"] = "Single Node Cluster"
+			instanceState["data_security_mode"] = "SINGLE_USER"
+			instanceState["spark_version"] = "15.4.x-scala2.12"
+			instanceState["single_user_name"] = "testuser"
+			instanceState["runtime_engine"] = "STANDARD"
+			instanceState["num_workers"] = "0"
+			instanceState["node_type_id"] = "Standard_F4s"
+			instanceState["kind"] = "CLASSIC_PREVIEW"
+			instanceState["is_single_node"] = "true"


Not a fan of this; if there's a way to simplify this please lemme know

…to `true`

github-actions · 2025-01-21T18:00:44Z

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/terraform

Inputs:

PR number: 4416
Commit SHA: a063bc5d54621c6f1039cfa60fcec033af3820ee

Checks will be approved automatically on success.

ashenm requested review from a team as code owners January 17, 2025 16:20

ashenm requested review from mgyucht and removed request for a team January 17, 2025 16:20

ashenm temporarily deployed to test-trigger-is January 17, 2025 16:30 — with GitHub Actions Inactive

mgyucht reviewed Jan 20, 2025

View reviewed changes

ashenm force-pushed the issue-4360 branch from 506d8ee to d639747 Compare January 21, 2025 07:59

ashenm commented Jan 21, 2025

View reviewed changes

[Fix] Fix drift in databricks_cluster when is_single_node is set …

a063bc5

…to `true`

ashenm force-pushed the issue-4360 branch from d639747 to a063bc5 Compare January 21, 2025 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] Fix drift in `databricks_cluster` when `is_single_node` is set to `true` #4416

[Fix] Fix drift in `databricks_cluster` when `is_single_node` is set to `true` #4416

ashenm commented Jan 17, 2025 •

edited

Loading

ashenm commented Jan 17, 2025

alexott commented Jan 17, 2025

alexott commented Jan 20, 2025

mgyucht commented Jan 20, 2025

mgyucht left a comment

mgyucht Jan 20, 2025

mgyucht Jan 20, 2025

ashenm Jan 21, 2025

mgyucht Jan 20, 2025

ashenm Jan 21, 2025

ashenm Jan 21, 2025

github-actions bot commented Jan 21, 2025

-func singleNodeClusterChangesCustomizeDiff(d *schema.ResourceDiff) error {
+// singleNodeClusterChangesCustomizeDiff adjusts the plan when `is_single_node` is `true`. When this happens,
+// the API automatically sets the `ResourceClass` key in `custom_tags` and two other keys in the `spark_conf`.
+// If the user hasn't set these explicitly in their config, the plan marks these keys for removal.
+// This method copies the values for these keys from state to the plan.
+// This needs to be done in addition to setting these attributes as computed; otherwise, this customization
+// won't take effect for users who have set additional `spark_conf` or `custom_tags`.
+func singleNodeClusterChangesCustomizeDiff(d *schema.ResourceDiff) error {

	if isSingleNode, _ := d.GetOk("is_single_node"); isSingleNode.(bool) {
	if isSingleNode, ok := d.GetOk("is_single_node"); ok && isSingleNode.(bool) {

[Fix] Fix drift in databricks_cluster when is_single_node is set to true #4416

Are you sure you want to change the base?

[Fix] Fix drift in databricks_cluster when is_single_node is set to true #4416

Conversation

ashenm commented Jan 17, 2025 • edited Loading

Changes

Tests

ashenm commented Jan 17, 2025

alexott commented Jan 17, 2025

alexott commented Jan 20, 2025

mgyucht commented Jan 20, 2025

mgyucht left a comment

Choose a reason for hiding this comment

mgyucht Jan 20, 2025

Choose a reason for hiding this comment

mgyucht Jan 20, 2025

Choose a reason for hiding this comment

ashenm Jan 21, 2025

Choose a reason for hiding this comment

mgyucht Jan 20, 2025

Choose a reason for hiding this comment

ashenm Jan 21, 2025

Choose a reason for hiding this comment

ashenm Jan 21, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 21, 2025

[Fix] Fix drift in `databricks_cluster` when `is_single_node` is set to `true` #4416

[Fix] Fix drift in `databricks_cluster` when `is_single_node` is set to `true` #4416

ashenm commented Jan 17, 2025 •

edited

Loading