From 21735c45d27132410ad127d6b290d14a40804ede Mon Sep 17 00:00:00 2001 From: Tanmay Rustagi <88379306+tanmay-db@users.noreply.github.com> Date: Thu, 14 Nov 2024 17:18:22 +0530 Subject: [PATCH 1/2] [Internal] Rename resources directory to products in pluginframework (#4139) ## Changes We organise both resources and data sources together for each type of products, renaming resources directory to products to make it less confusing. ## Tests Existing 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 --- CONTRIBUTING.md | 6 +++--- .../providers/pluginfw/pluginfw_rollout_utils.go | 16 ++++++++-------- .../catalog/data_functions.go | 0 .../cluster/data_cluster.go | 0 .../cluster/data_cluster_acc_test.go | 0 .../cluster/data_cluster_test.go | 0 .../library/resource_library.go | 0 .../library/resource_library_acc_test.go | 0 .../data_notification_destinations.go | 0 .../data_notification_destinations_acc_test.go | 0 .../data_notification_destinations_test.go | 0 .../qualitymonitor/resource_quality_monitor.go | 0 .../resource_quality_monitor_acc_test.go | 0 .../registered_model/data_registered_model.go | 0 .../sharing/data_share.go | 0 .../sharing/data_shares.go | 0 .../sharing/data_shares_acc_test.go | 0 .../sharing/resource_acc_test.go | 0 .../sharing/resource_share.go | 0 .../volume/data_volumes.go | 0 .../volume/data_volumes_acc_test.go | 0 21 files changed, 11 insertions(+), 11 deletions(-) rename internal/providers/pluginfw/{resources => products}/catalog/data_functions.go (100%) rename internal/providers/pluginfw/{resources => products}/cluster/data_cluster.go (100%) rename internal/providers/pluginfw/{resources => products}/cluster/data_cluster_acc_test.go (100%) rename internal/providers/pluginfw/{resources => products}/cluster/data_cluster_test.go (100%) rename internal/providers/pluginfw/{resources => products}/library/resource_library.go (100%) rename internal/providers/pluginfw/{resources => products}/library/resource_library_acc_test.go (100%) rename internal/providers/pluginfw/{resources => products}/notificationdestinations/data_notification_destinations.go (100%) rename internal/providers/pluginfw/{resources => products}/notificationdestinations/data_notification_destinations_acc_test.go (100%) rename internal/providers/pluginfw/{resources => products}/notificationdestinations/data_notification_destinations_test.go (100%) rename internal/providers/pluginfw/{resources => products}/qualitymonitor/resource_quality_monitor.go (100%) rename internal/providers/pluginfw/{resources => products}/qualitymonitor/resource_quality_monitor_acc_test.go (100%) rename internal/providers/pluginfw/{resources => products}/registered_model/data_registered_model.go (100%) rename internal/providers/pluginfw/{resources => products}/sharing/data_share.go (100%) rename internal/providers/pluginfw/{resources => products}/sharing/data_shares.go (100%) rename internal/providers/pluginfw/{resources => products}/sharing/data_shares_acc_test.go (100%) rename internal/providers/pluginfw/{resources => products}/sharing/resource_acc_test.go (100%) rename internal/providers/pluginfw/{resources => products}/sharing/resource_share.go (100%) rename internal/providers/pluginfw/{resources => products}/volume/data_volumes.go (100%) rename internal/providers/pluginfw/{resources => products}/volume/data_volumes_acc_test.go (100%) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ab38782660..33e24db676 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -119,7 +119,7 @@ We are migrating the resource from SDKv2 to Plugin Framework provider and hence - `sdkv2`: Contains the changes specific to SDKv2. This package shouldn't depend on pluginfw or common. ### Adding a new resource -1. Check if the directory for this particular resource exists under `internal/providers/pluginfw/resources`, if not create the directory eg: `cluster`, `volume` etc... Please note: Resources and Data sources are organized under the same package for that service. +1. Check if the directory for this particular resource exists under `internal/providers/plugnifw/products`, if not create the directory eg: `cluster`, `volume` etc... Please note: Resources and Data sources are organized under the same package for that service. 2. Create a file with resource_resource-name.go and write the CRUD methods, schema for that resource. For reference, please take a look at existing resources eg: `resource_quality_monitor.go` 3. Create a file with `resource_resource-name_acc_test.go` and add integration tests here. 4. Create a file with `resource_resource-name_test.go` and add unit tests here. Note: Please make sure to abstract specific method of the resource so they are unit test friendly and not testing internal part of terraform plugin framework library. You can compare the diagnostics, for example: please take a look at: `data_cluster_test.go` @@ -127,7 +127,7 @@ We are migrating the resource from SDKv2 to Plugin Framework provider and hence 6. Create a PR and send it for review. ### Adding a new data source -1. Check if the directory for this particular datasource exists under `internal/providers/pluginfw/resources`, if not create the directory eg: `cluster`, `volume` etc... Please note: Resources and Data sources are organized under the same package for that service. +1. Check if the directory for this particular datasource exists under `internal/providers/plugnifw/products`, if not create the directory eg: `cluster`, `volume` etc... Please note: Resources and Data sources are organized under the same package for that service. 2. Create a file with `data_resource-name.go` and write the CRUD methods, schema for that data source. For reference, please take a look at existing data sources eg: `data_cluster.go` 3. Create a file with `data_resource-name_acc_test.go` and add integration tests here. 4. Create a file with `data_resource-name_test.go` and add unit tests here. Note: Please make sure to abstract specific method of the resource so they are unit test friendly and not testing internal part of terraform plugin framework library. You can compare the diagnostics, for example: please take a look at: `data_cluster_test.go` @@ -141,7 +141,7 @@ Ideally there shouldn't be any behaviour change when migrating a resource or dat ### Code Organization -Each resource and data source should be defined in package `internal/providers/pluginfw/resources/`, e.g.: `internal/providers/pluginfw/resources/volume` package will contain both resource, data sources and other utils specific to volumes. Tests (both unit and integration tests) will also remain in this package. +Each resource and data source should be defined in package `internal/providers/plugnifw/products/`, e.g.: `internal/providers/plugnifw/products/volume` package will contain both resource, data sources and other utils specific to volumes. Tests (both unit and integration tests) will also remain in this package. Note: Only Docs will stay under root docs/ directory. diff --git a/internal/providers/pluginfw/pluginfw_rollout_utils.go b/internal/providers/pluginfw/pluginfw_rollout_utils.go index 90b782a511..87e75d01d8 100644 --- a/internal/providers/pluginfw/pluginfw_rollout_utils.go +++ b/internal/providers/pluginfw/pluginfw_rollout_utils.go @@ -12,14 +12,14 @@ import ( "slices" "strings" - "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/resources/catalog" - "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/resources/cluster" - "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/resources/library" - "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/resources/notificationdestinations" - "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/resources/qualitymonitor" - "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/resources/registered_model" - "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/resources/sharing" - "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/resources/volume" + "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/products/catalog" + "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/products/cluster" + "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/products/library" + "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/products/notificationdestinations" + "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/products/qualitymonitor" + "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/products/registered_model" + "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/products/sharing" + "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/products/volume" "github.com/hashicorp/terraform-plugin-framework/datasource" "github.com/hashicorp/terraform-plugin-framework/resource" ) diff --git a/internal/providers/pluginfw/resources/catalog/data_functions.go b/internal/providers/pluginfw/products/catalog/data_functions.go similarity index 100% rename from internal/providers/pluginfw/resources/catalog/data_functions.go rename to internal/providers/pluginfw/products/catalog/data_functions.go diff --git a/internal/providers/pluginfw/resources/cluster/data_cluster.go b/internal/providers/pluginfw/products/cluster/data_cluster.go similarity index 100% rename from internal/providers/pluginfw/resources/cluster/data_cluster.go rename to internal/providers/pluginfw/products/cluster/data_cluster.go diff --git a/internal/providers/pluginfw/resources/cluster/data_cluster_acc_test.go b/internal/providers/pluginfw/products/cluster/data_cluster_acc_test.go similarity index 100% rename from internal/providers/pluginfw/resources/cluster/data_cluster_acc_test.go rename to internal/providers/pluginfw/products/cluster/data_cluster_acc_test.go diff --git a/internal/providers/pluginfw/resources/cluster/data_cluster_test.go b/internal/providers/pluginfw/products/cluster/data_cluster_test.go similarity index 100% rename from internal/providers/pluginfw/resources/cluster/data_cluster_test.go rename to internal/providers/pluginfw/products/cluster/data_cluster_test.go diff --git a/internal/providers/pluginfw/resources/library/resource_library.go b/internal/providers/pluginfw/products/library/resource_library.go similarity index 100% rename from internal/providers/pluginfw/resources/library/resource_library.go rename to internal/providers/pluginfw/products/library/resource_library.go diff --git a/internal/providers/pluginfw/resources/library/resource_library_acc_test.go b/internal/providers/pluginfw/products/library/resource_library_acc_test.go similarity index 100% rename from internal/providers/pluginfw/resources/library/resource_library_acc_test.go rename to internal/providers/pluginfw/products/library/resource_library_acc_test.go diff --git a/internal/providers/pluginfw/resources/notificationdestinations/data_notification_destinations.go b/internal/providers/pluginfw/products/notificationdestinations/data_notification_destinations.go similarity index 100% rename from internal/providers/pluginfw/resources/notificationdestinations/data_notification_destinations.go rename to internal/providers/pluginfw/products/notificationdestinations/data_notification_destinations.go diff --git a/internal/providers/pluginfw/resources/notificationdestinations/data_notification_destinations_acc_test.go b/internal/providers/pluginfw/products/notificationdestinations/data_notification_destinations_acc_test.go similarity index 100% rename from internal/providers/pluginfw/resources/notificationdestinations/data_notification_destinations_acc_test.go rename to internal/providers/pluginfw/products/notificationdestinations/data_notification_destinations_acc_test.go diff --git a/internal/providers/pluginfw/resources/notificationdestinations/data_notification_destinations_test.go b/internal/providers/pluginfw/products/notificationdestinations/data_notification_destinations_test.go similarity index 100% rename from internal/providers/pluginfw/resources/notificationdestinations/data_notification_destinations_test.go rename to internal/providers/pluginfw/products/notificationdestinations/data_notification_destinations_test.go diff --git a/internal/providers/pluginfw/resources/qualitymonitor/resource_quality_monitor.go b/internal/providers/pluginfw/products/qualitymonitor/resource_quality_monitor.go similarity index 100% rename from internal/providers/pluginfw/resources/qualitymonitor/resource_quality_monitor.go rename to internal/providers/pluginfw/products/qualitymonitor/resource_quality_monitor.go diff --git a/internal/providers/pluginfw/resources/qualitymonitor/resource_quality_monitor_acc_test.go b/internal/providers/pluginfw/products/qualitymonitor/resource_quality_monitor_acc_test.go similarity index 100% rename from internal/providers/pluginfw/resources/qualitymonitor/resource_quality_monitor_acc_test.go rename to internal/providers/pluginfw/products/qualitymonitor/resource_quality_monitor_acc_test.go diff --git a/internal/providers/pluginfw/resources/registered_model/data_registered_model.go b/internal/providers/pluginfw/products/registered_model/data_registered_model.go similarity index 100% rename from internal/providers/pluginfw/resources/registered_model/data_registered_model.go rename to internal/providers/pluginfw/products/registered_model/data_registered_model.go diff --git a/internal/providers/pluginfw/resources/sharing/data_share.go b/internal/providers/pluginfw/products/sharing/data_share.go similarity index 100% rename from internal/providers/pluginfw/resources/sharing/data_share.go rename to internal/providers/pluginfw/products/sharing/data_share.go diff --git a/internal/providers/pluginfw/resources/sharing/data_shares.go b/internal/providers/pluginfw/products/sharing/data_shares.go similarity index 100% rename from internal/providers/pluginfw/resources/sharing/data_shares.go rename to internal/providers/pluginfw/products/sharing/data_shares.go diff --git a/internal/providers/pluginfw/resources/sharing/data_shares_acc_test.go b/internal/providers/pluginfw/products/sharing/data_shares_acc_test.go similarity index 100% rename from internal/providers/pluginfw/resources/sharing/data_shares_acc_test.go rename to internal/providers/pluginfw/products/sharing/data_shares_acc_test.go diff --git a/internal/providers/pluginfw/resources/sharing/resource_acc_test.go b/internal/providers/pluginfw/products/sharing/resource_acc_test.go similarity index 100% rename from internal/providers/pluginfw/resources/sharing/resource_acc_test.go rename to internal/providers/pluginfw/products/sharing/resource_acc_test.go diff --git a/internal/providers/pluginfw/resources/sharing/resource_share.go b/internal/providers/pluginfw/products/sharing/resource_share.go similarity index 100% rename from internal/providers/pluginfw/resources/sharing/resource_share.go rename to internal/providers/pluginfw/products/sharing/resource_share.go diff --git a/internal/providers/pluginfw/resources/volume/data_volumes.go b/internal/providers/pluginfw/products/volume/data_volumes.go similarity index 100% rename from internal/providers/pluginfw/resources/volume/data_volumes.go rename to internal/providers/pluginfw/products/volume/data_volumes.go diff --git a/internal/providers/pluginfw/resources/volume/data_volumes_acc_test.go b/internal/providers/pluginfw/products/volume/data_volumes_acc_test.go similarity index 100% rename from internal/providers/pluginfw/resources/volume/data_volumes_acc_test.go rename to internal/providers/pluginfw/products/volume/data_volumes_acc_test.go From 6e7ca4c95353acc63313973e229fc36f0909713f Mon Sep 17 00:00:00 2001 From: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com> Date: Thu, 14 Nov 2024 17:28:26 +0530 Subject: [PATCH 2/2] [Fix] Remove single-node validation from interactive clusters (#4222) ## Changes There were reports on https://github.com/databricks/cli/issues/1546 about customers being unable to use cluster policies for single-node job clusters because Terraform was performing this validation. https://github.com/databricks/terraform-provider-databricks/pull/4216 removed this validation for job clusters. This PR removes this validation for interactive clusters as well to keep them consistent and unblock the use of policy from interactive clusters. Also, the clusters team is improving the API interface to simplify creating single-node clusters, significantly reducing the chances of users misconfiguring them. ## Tests Unit test and manually. Clusters are now successfully created and updated with `num_workers=0` --- clusters/resource_cluster.go | 59 ---------------------- clusters/resource_cluster_test.go | 82 ++++++++++++++++++------------- 2 files changed, 49 insertions(+), 92 deletions(-) diff --git a/clusters/resource_cluster.go b/clusters/resource_cluster.go index 28672e2962..93d8d2fb59 100644 --- a/clusters/resource_cluster.go +++ b/clusters/resource_cluster.go @@ -26,26 +26,6 @@ var clusterSchema = resourceClusterSchema() var clusterSchemaVersion = 4 const ( - numWorkerErr = `num_workers may be 0 only for single-node clusters. To create a single node -cluster please include the following configuration in your cluster configuration: - - spark_conf = { - "spark.databricks.cluster.profile" : "singleNode" - "spark.master" : "local[*]" - } - - custom_tags = { - "ResourceClass" = "SingleNode" - } - -Please note that the Databricks Terraform provider cannot detect if the above configuration -is defined in a policy used by the cluster. Please define this in the cluster configuration -itself to create a single node cluster. - -For more details please see: - 1. https://registry.terraform.io/providers/databricks/databricks/latest/docs/resources/cluster#fixed-size-or-autoscaling-cluster - 2. https://docs.databricks.com/clusters/single-node.html` - unsupportedExceptCreateEditClusterSpecErr = "unsupported type %T, must be one of %scompute.CreateCluster, %scompute.ClusterSpec or %scompute.EditCluster. Please report this issue to the GitHub repo" ) @@ -130,39 +110,6 @@ func ZoneDiffSuppress(k, old, new string, d *schema.ResourceData) bool { return false } -func Validate(cluster any) error { - var profile, master, resourceClass string - switch c := cluster.(type) { - case compute.CreateCluster: - if c.NumWorkers > 0 || c.Autoscale != nil { - return nil - } - profile = c.SparkConf["spark.databricks.cluster.profile"] - master = c.SparkConf["spark.master"] - resourceClass = c.CustomTags["ResourceClass"] - case compute.EditCluster: - if c.NumWorkers > 0 || c.Autoscale != nil { - return nil - } - profile = c.SparkConf["spark.databricks.cluster.profile"] - master = c.SparkConf["spark.master"] - resourceClass = c.CustomTags["ResourceClass"] - case compute.ClusterSpec: - if c.NumWorkers > 0 || c.Autoscale != nil { - return nil - } - profile = c.SparkConf["spark.databricks.cluster.profile"] - master = c.SparkConf["spark.master"] - resourceClass = c.CustomTags["ResourceClass"] - default: - return fmt.Errorf(unsupportedExceptCreateEditClusterSpecErr, cluster, "", "", "") - } - if profile == "singleNode" && strings.HasPrefix(master, "local") && resourceClass == "SingleNode" { - return nil - } - return errors.New(numWorkerErr) -} - // This method is a duplicate of ModifyRequestOnInstancePool() in clusters/clusters_api.go that uses Go SDK. // Long term, ModifyRequestOnInstancePool() in clusters_api.go will be removed once all the resources using clusters are migrated to Go SDK. func ModifyRequestOnInstancePool(cluster any) error { @@ -443,9 +390,6 @@ func resourceClusterCreate(ctx context.Context, d *schema.ResourceData, c *commo clusters := w.Clusters var createClusterRequest compute.CreateCluster common.DataToStructPointer(d, clusterSchema, &createClusterRequest) - if err := Validate(createClusterRequest); err != nil { - return err - } if err = ModifyRequestOnInstancePool(&createClusterRequest); err != nil { return err } @@ -596,9 +540,6 @@ func resourceClusterUpdate(ctx context.Context, d *schema.ResourceData, c *commo if hasClusterConfigChanged(d) { log.Printf("[DEBUG] Cluster state has changed!") - if err := Validate(cluster); err != nil { - return err - } if err = ModifyRequestOnInstancePool(&cluster); err != nil { return err } diff --git a/clusters/resource_cluster_test.go b/clusters/resource_cluster_test.go index 1ef37126d7..00e5e1dfd4 100644 --- a/clusters/resource_cluster_test.go +++ b/clusters/resource_cluster_test.go @@ -1630,22 +1630,6 @@ func TestResourceClusterCreate_SingleNode(t *testing.T) { assert.NoError(t, err) assert.Equal(t, 0, d.Get("num_workers")) } - -func TestResourceClusterCreate_SingleNodeFail(t *testing.T) { - _, err := qa.ResourceFixture{ - Create: true, - Resource: ResourceCluster(), - State: map[string]any{ - "autotermination_minutes": 120, - "cluster_name": "Single Node Cluster", - "spark_version": "7.3.x-scala12", - "node_type_id": "Standard_F4s", - "is_pinned": false, - }, - }.Apply(t) - assert.EqualError(t, err, numWorkerErr) -} - func TestResourceClusterCreate_NegativeNumWorkers(t *testing.T) { _, err := qa.ResourceFixture{ Create: true, @@ -1662,27 +1646,59 @@ func TestResourceClusterCreate_NegativeNumWorkers(t *testing.T) { require.Equal(t, true, strings.Contains(err.Error(), "expected num_workers to be at least (0)")) } -func TestResourceClusterUpdate_FailNumWorkersZero(t *testing.T) { - _, err := qa.ResourceFixture{ - ID: "abc", - Update: true, - Resource: ResourceCluster(), - InstanceState: map[string]string{ - "autotermination_minutes": "15", - "cluster_name": "Shared Autoscaling", - "spark_version": "7.1-scala12", - "node_type_id": "i3.xlarge", - "num_workers": "100", +func TestResourceClusterCreate_NumWorkersIsZero(t *testing.T) { + d, err := qa.ResourceFixture{ + Fixtures: []qa.HTTPFixture{ + nothingPinned, + { + Method: "POST", + Resource: "/api/2.1/clusters/create", + ExpectedRequest: compute.CreateCluster{ + NumWorkers: 0, + ClusterName: "Zero workers cluster", + SparkVersion: "7.3.x-scala12", + NodeTypeId: "Standard_F4s", + AutoterminationMinutes: 120, + ForceSendFields: []string{"NumWorkers"}, + }, + Response: compute.ClusterDetails{ + ClusterId: "abc", + State: compute.StateRunning, + }, + }, + { + Method: "GET", + ReuseRequest: true, + Resource: "/api/2.1/clusters/get?cluster_id=abc", + Response: compute.ClusterDetails{ + ClusterId: "abc", + ClusterName: "Zero workers cluster", + SparkVersion: "7.3.x-scala12", + NodeTypeId: "Standard_F4s", + AutoterminationMinutes: 120, + State: compute.StateRunning, + }, + }, + { + Method: "GET", + Resource: "/api/2.0/libraries/cluster-status?cluster_id=abc", + Response: compute.ClusterLibraryStatuses{ + LibraryStatuses: []compute.LibraryFullStatus{}, + }, + }, }, + Create: true, + Resource: ResourceCluster(), State: map[string]any{ - "autotermination_minutes": 15, - "cluster_name": "Shared Autoscaling", - "spark_version": "7.1-scala12", - "node_type_id": "i3.xlarge", - "num_workers": 0, + "autotermination_minutes": 120, + "cluster_name": "Zero workers cluster", + "spark_version": "7.3.x-scala12", + "node_type_id": "Standard_F4s", + "is_pinned": false, }, }.Apply(t) - assert.EqualError(t, err, numWorkerErr) + assert.NoError(t, err) + assert.Equal(t, 0, d.Get("num_workers")) } func TestModifyClusterRequestAws(t *testing.T) {