Migrate cluster schema to use the go-sdk struct

[edwardfeng-db]

Changes

• Before we are able to migrate the cluster resources to use the go-sdk schema, we need to figure out a way to represent the current tf tags on the manually maintained structs
• This PR introduces a new interface ResourceProviderStruct which contains a method for aliases and all of the customizations.
  • Aliases() map[string]string
  • CustomizeSchema(map[string]*schema.Schema) map[string]*schema.Schema
• Also added a series of customization functions such as SetOptional, etc so that we can easily write customization logics into the CustomizeSchema function to replace the current tf tags
• Migrated the cluster schema into the go-sdk schema with this new interface

Tests

• make test run locally
• relevant change in docs/ folder
• covered with integration tests in internal/acceptance
• relevant acceptance tests are passing
• using Go SDK

[codecov-commenter]

Codecov Report

Attention: 73 lines in your changes are missing coverage. Please review.

Comparison is base (16c61e5) 84.31% compared to head (78fc653) 83.46%.
Report is 20 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3076      +/-   ##
==========================================
- Coverage   84.31%   83.46%   -0.86%     
==========================================
  Files         162      166       +4     
  Lines       14412    14710     +298     
==========================================
+ Hits        12152    12278     +126     
- Misses       1563     1719     +156     
- Partials      697      713      +16     

Files	Coverage Δ
clusters/clusters_api.go	84.48% <ø> (ø)
clusters/resource_cluster.go	87.86% <100.00%> (+2.09%)	⬆️
common/reflect_resource.go	82.93% <59.25%> (-1.45%)	⬇️
common/customizable_schema.go	0.00% <0.00%> (ø)

... and 20 files with indirect coverage changes

[edwardfeng-db]

This change is introduced in #3044

[edwardfeng-db]

Will need to figure out a way to merge these with the code introduced in #3044. Shouldn't be too hard

[edwardfeng-db]

This will introduce a "breaking" diff, but it shouldn't be a problem since users are not expected to set this any way. And this will address #2966

[tanmay-db]

Thanks for the PR, left some comments. Just to confirm, the nightly did pass on the PR right?

[tanmay-db]

This looks a bit difficult to maintain and I think something similar would be required for jobs resource as well? should we split this up so it's easier to read, for example, we could split on top level field, for example init_scripts, workload_type? A question - how do we check if customizations are complete?

[edwardfeng-db]

Yeah for the jobs resource, we would just set whatever path that leads to the cluster reference to be the schema derived from ClusterResourceProvider so we don't have to duplicate it.

For making sure the costomizations are complete, we rely on the diff schema PR check and I verified that there's no breaking change (only added field, nothing removed)

[mgyucht]

Ultimately, these things need to be derived one way or another from the OpenAPI specification. From my read, the main classes of issues are:

1. Computed fields (not in the OpenAPI spec, but potentially could be derived from comparing get to create/update).
2. Optional fields (this should be in the OpenAPI spec).
3. Required fields (this should be in the OpenAPI spec).
4. Fields that conflict with other fields (not yet in the OpenAPI spec, but can be with oneOf support).
5. OpenAPI spec missing information (in the short term, we can file a ticket to the upstream team, @edwardfeng-db would be great if you can do this; in the long term, our OpenAPI generator from @hectorcast-db will address this).
6. MaxItems validation (this could also be in the OpenAPI spec).
7. Deprecated fields (this is in the OpenAPI spec but is lost in the translation to the Go SDK).
8. Enum values (these are in the OpenAPI spec, but there is no convenient way to get all enum values in the Go SDK).
9. Sensitive data (needs an extension, probably, but could be in the OpenAPI spec).
10. Suppress diff

Here's how I think we can handle this:
2: Fields are optional by default, so I'm curious whether/why we need to call SetOptional() for so many of them. @edwardfeng-db why is this?
3, 5: file a ticket with the Clusters team to update their OpenAPI spec so that it is complete, including missing fields and the correct annotations for required/optional.
8: We can add a method to enums to list all values; then, we could automatically compute a validator for enum fields in the resource. Let's file a ticket for this @edwardfeng-db.
1, 4, 6, 7, 9, 10: I think most of these will be most easily addressed as we try to autogenerate the provider implementation from the OpenAPI spec itself; otherwise, they require hacks to embed this metadata somehow in Go SDK struct field tags.

[edwardfeng-db]

Thanks. This sounds good. I'll create the follow up tickets.

For 2, there were many calls to SetOptional() because I was following the previous override function but I realized that a lot of these can be taken out because omit_empty exists on the openapi spec. I just took the unnecessary ones out, it should look cleaner now.

[edwardfeng-db]

Ticket for enums: https://databricks.atlassian.net/browse/DECO-8056

Ticket for the cluster team to fix openapi spec https://databricks.atlassian.net/browse/ES-1004564

[mgyucht]

Looking great. I don't think we need to block on the OpenAPI changes for this PR, but I do think we should take that as an immediate follow-up. Just a few comments, and we need to add a unit test for the alias behavior changes.

@tanmay-db let's also take note of these elements as things that need to be addressed as part of the autogeneration of TF resources from OpenAPI spec.

[mgyucht]

Ultimately, these things need to be derived one way or another from the OpenAPI specification. From my read, the main classes of issues are:

1. Computed fields (not in the OpenAPI spec, but potentially could be derived from comparing get to create/update).
2. Optional fields (this should be in the OpenAPI spec).
3. Required fields (this should be in the OpenAPI spec).
4. Fields that conflict with other fields (not yet in the OpenAPI spec, but can be with oneOf support).
5. OpenAPI spec missing information (in the short term, we can file a ticket to the upstream team, @edwardfeng-db would be great if you can do this; in the long term, our OpenAPI generator from @hectorcast-db will address this).
6. MaxItems validation (this could also be in the OpenAPI spec).
7. Deprecated fields (this is in the OpenAPI spec but is lost in the translation to the Go SDK).
8. Enum values (these are in the OpenAPI spec, but there is no convenient way to get all enum values in the Go SDK).
9. Sensitive data (needs an extension, probably, but could be in the OpenAPI spec).
10. Suppress diff

Here's how I think we can handle this:
2: Fields are optional by default, so I'm curious whether/why we need to call SetOptional() for so many of them. @edwardfeng-db why is this?
3, 5: file a ticket with the Clusters team to update their OpenAPI spec so that it is complete, including missing fields and the correct annotations for required/optional.
8: We can add a method to enums to list all values; then, we could automatically compute a validator for enum fields in the resource. Let's file a ticket for this @edwardfeng-db.
1, 4, 6, 7, 9, 10: I think most of these will be most easily addressed as we try to autogenerate the provider implementation from the OpenAPI spec itself; otherwise, they require hacks to embed this metadata somehow in Go SDK struct field tags.

[mgyucht]

LGTM, thanks man!

[alexott]

There are still changes required in the cluster resource.

[alexott]

There is a permanent drift at least on Azure:

  # databricks_cluster.shared_autoscaling will be updated in-place
  ~ resource "databricks_cluster" "shared_autoscaling" {
        id                           = "0118-192555-7kqkpwml"
        # (14 unchanged attributes hidden)

      ~ azure_attributes {
          - availability       = "ON_DEMAND_AZURE" -> null
          - first_on_demand    = 1 -> null
          - spot_bid_max_price = -1 -> null
        }

        # (1 unchanged block hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Code to reproduce:

terraform {
  required_providers {
    databricks = {
      source = "databricks/databricks"
      version = "1.34.0"
    }
  }
}

provider "databricks" {
  # Configuration options
}


data "databricks_node_type" "smallest" {
  local_disk = true
}

data "databricks_spark_version" "latest_lts" {
  long_term_support = true
}

resource "databricks_cluster" "shared_autoscaling" {
  cluster_name            = "Shared Autoscaling"
  spark_version           = data.databricks_spark_version.latest_lts.id
  node_type_id            = data.databricks_node_type.smallest.id
  autotermination_minutes = 20
  idempotency_token = "123"
  autoscale {
    min_workers = 1
    max_workers = 50
  }
}

[edwardfeng-db]

@alexott Thanks! I fixed the problem with the permanent drift on azure_attributes. It was due to suppress_diff not set on those fields. Now we set those properly.

[mgyucht]

Todo: check aws & gcp attributes don't drift. Can add an integration test that create cluster works with no custom attributes set.

[mgyucht]

Checked other nightlies: they did fail previously before suppress diff was added to the cloud-specific resources.