[Fix] Fix `databricks_sql_table` treatment of properties by mgyucht · Pull Request #3925 · databricks/terraform-provider-databricks

mgyucht · 2024-08-21T15:01:49Z

Changes

databricks_sql_table has been difficult to maintain due to the fact that Databricks adds table properties and options that users haven't directly configured. Historically, to handle this, we have tried to construct properties carefully by tracking which properties are added by Databricks automatically and removing them. This has two challenges:

We don't know ahead of time which properties are set by Databricks, so we don't know which properties we need to remove to compute an accurate diff.
Users are not able to specify different values for those default properties: they are hidden from the serializeProperties() and serializeOptions() functions in the resource implementation.

This PR takes a different approach for handling these fields by storing the properties/options returned by the API in effective_properties and effective_options fields, respectively. These fields are computed and non-user-settable, so users cannot override them. When creating/updating the table, we only include the properties specified by the user. However, when determining if there is a diff, we compare the effective properties fetched by the terraform plan triggering an update of the table state with the user-specified properties. If the user specified any properties that are not part of the effective properties/options, or if the user's value clashes with the value provided, this will trigger a diff. However, if a non-user-specified property has a difference, then that does not trigger a diff.

As a side-effect, this PR also fixes the treatment of TBLPROPERTIES and OPTIONS as reported in #3894. It also makes the properties field not computed, fixing #3652. It also allows users to override properties that are otherwise set by default, fixing #2800.

Fixes #3894.
Fixes #3652.
Fixes #3378.
Fixes #2800.

Tests

Unit tests to verify the diff behavior with this change.
Unit test to ensure that TBLPROPERTIES is correctly set.

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

alexott

lgtm

alexott · 2024-08-22T13:49:13Z

+	for _, field := range []string{"effective_properties", "effective_options"} {
+		s.SchemaPath(field).SetReadOnly()
+	}


if we set them to tf:computed they should be set as readonly automatically

nkvuong · 2024-08-23T09:19:23Z

+					StorageLocation:       "s3://ext-main/foo/bar1",
+					StorageCredentialName: "somecred",
+					Comment:               "terraform managed",
+					Properties: map[string]string{


nit: delta.enableDeletionVectors should appear in the response properties as well?

nkvuong

this looks good to me, just a nit on the new unit test

apamildner · 2024-08-27T07:52:25Z

Thank you very much for this! Can we do a new release so this commit is available to us? It has been long awaited 👍

### New Features and Improvements * Added `no_wait` option for clusters to skip waiting to start on cluster creation ([#3953](#3953)). * Introduced Plugin Framework ([#3920](#3920)). ### Bug Fixes * Add suppress diff for `azure_attributes.spot_bid_max_price` in `databricks_instance_pool` ([#3970](#3970)). * Correctly send workload_type fields in `databricks_cluster` to allow users to disable usage in certain contexts ([#3972](#3972)). * Fix `TestAccClusterResource_WorkloadType` ([#3989](#3989)). * Fix `databricks_sql_table` treatment of properties ([#3925](#3925)). * Force send fields for settings resources ([#3978](#3978)). * Handle cluster deletion in `databricks_library` read ([#3909](#3909)). * Make subscriptions optional for SqlAlertTask ([#3983](#3983)). ### Documentation * Add troubleshooting guide for Provider "registry.terraform.io/databricks/databricks" planned an invalid value ([#3961](#3961)). * Adopt official naming of Mosaic AI Vector Search ([#3971](#3971)). * Document Terraform 1.0 as minimum version ([#3952](#3952)). * Mention Salesforce as supported type in `databricks_connection` ([#3949](#3949)). * Reimplement Azure Databricks deployment guide to use VNet injection & NPIP ([#3986](#3986)). * Resolves [#3127](#3127): Remove deprecated account_id field from mws_credentials resource ([#3974](#3974)). * Small Grammar Corrections in Docs ([#4006](#4006)). * Update `databricks_vector_search_index` docs to match latest SDK ([#4008](#4008)). * Update aws_unity_catalog_assume_role_policy.md ([#3968](#3968)). * Update documentation regarding authentication with Azure-managed Service Principal using GITHUB OIDC ([#3932](#3932)). * Update metastore_assignment.md to properly reflect possible usage ([#3967](#3967)). * Update minimum supported terraform version to 1.1.5 ([#3965](#3965)). * Update resources diagram to include newer resources ([#3962](#3962)). * Update workspace_binding import command ([#3944](#3944)). * fix possible values for `securable_type` in `databricks_workspace_binding` ([#3942](#3942)). ### Internal Changes * Add `AddPlanModifer` method for AttributeBuilder ([#4009](#4009)). * Add integration tests for volumes and quality monitor plugin framework ([#3975](#3975)). * Add support for `computed` tag in TfSDK Structs ([#4005](#4005)). * Added `databricks_quality_monitor` resource and `databricks_volumes` data source to plugin framework ([#3958](#3958)). * Allow vector search tests to fail ([#3959](#3959)). * Make test utils public and move integration test for quality monitor ([#3993](#3993)). * Migrate Share resource to Go SDK ([#3916](#3916)). * Migrate imports for terraform plugin framework + update init test provider factory ([#3943](#3943)). * Move volumes test next to plugin framework data source ([#3995](#3995)). * Refactor provider and related packages ([#3940](#3940)). * Support import in acceptance test + adding import state for quality monitor ([#3994](#3994)). ### Dependency Updates * Bump github.com/hashicorp/hcl/v2 from 2.21.0 to 2.22.0 ([#3948](#3948)). * Update Go SDK to 0.46.0 ([#4007](#4007)). ### Exporter * Don't generate instance pools if the pool name is empty ([#3960](#3960)).

### New Features and Improvements * Add support for filters in `databricks_clusters` data source ([#4014](#4014)). * Added `no_wait` option for clusters to skip waiting to start on cluster creation ([#3953](#3953)). * Introduced Plugin Framework ([#3920](#3920)). * Library plugin framework migration ([#3979](#3979)). ### Bug Fixes * Add suppress diff for `azure_attributes.spot_bid_max_price` in `databricks_instance_pool` ([#3970](#3970)). * Correctly send workload_type fields in `databricks_cluster` to allow users to disable usage in certain contexts ([#3972](#3972)). * Fix `TestAccClusterResource_WorkloadType` ([#3989](#3989)). * Fix `databricks_sql_table` treatment of properties ([#3925](#3925)). * Force send fields for settings resources ([#3978](#3978)). * Handle cluster deletion in `databricks_library` read ([#3909](#3909)). * Make subscriptions optional for SqlAlertTask ([#3983](#3983)). * Permanently delete `ERROR` and `TERMINATED` state clusters if their creation fails ([#4021](#4021)). ### Documentation * Add troubleshooting guide for Provider "registry.terraform.io/databricks/databricks" planned an invalid value ([#3961](#3961)). * Adopt official naming of Mosaic AI Vector Search ([#3971](#3971)). * Document Terraform 1.0 as minimum version ([#3952](#3952)). * Mention Salesforce as supported type in `databricks_connection` ([#3949](#3949)). * Reimplement Azure Databricks deployment guide to use VNet injection & NPIP ([#3986](#3986)). * Resolves [#3127](#3127): Remove deprecated account_id field from mws_credentials resource ([#3974](#3974)). * Small Grammar Corrections in Docs ([#4006](#4006)). * Update `databricks_vector_search_index` docs to match latest SDK ([#4008](#4008)). * Update aws_unity_catalog_assume_role_policy.md ([#3968](#3968)). * Update documentation regarding authentication with Azure-managed Service Principal using GITHUB OIDC ([#3932](#3932)). * Update metastore_assignment.md to properly reflect possible usage ([#3967](#3967)). * Update minimum supported terraform version to 1.1.5 ([#3965](#3965)). * Update resources diagram to include newer resources ([#3962](#3962)). * Update workspace_binding import command ([#3944](#3944)). * fix possible values for `securable_type` in `databricks_workspace_binding` ([#3942](#3942)). ### Internal Changes * Add `AddPlanModifer` method for AttributeBuilder ([#4009](#4009)). * Add integration tests for volumes and quality monitor plugin framework ([#3975](#3975)). * Add support for `computed` tag in TfSDK Structs ([#4005](#4005)). * Added `databricks_quality_monitor` resource and `databricks_volumes` data source to plugin framework ([#3958](#3958)). * Allow vector search tests to fail ([#3959](#3959)). * Clean up comments in library resource ([#4015](#4015)). * Fix irregularities in plugin framework converter function errors ([#4010](#4010)). * Make test utils public and move integration test for quality monitor ([#3993](#3993)). * Migrate Share resource to Go SDK ([#3916](#3916)). * Migrate `databricks_cluster` data source to plugin framework ([#3988](#3988)). * Migrate imports for terraform plugin framework + update init test provider factory ([#3943](#3943)). * Move volumes test next to plugin framework data source ([#3995](#3995)). * Refactor provider and related packages ([#3940](#3940)). * Support import in acceptance test + adding import state for quality monitor ([#3994](#3994)). ### Dependency Updates * Bump github.com/hashicorp/hcl/v2 from 2.21.0 to 2.22.0 ([#3948](#3948)). * Update Go SDK to 0.46.0 ([#4007](#4007)). ### Exporter * Don't generate instance pools if the pool name is empty ([#3960](#3960)).

### New Features and Improvements * Add support for filters in `databricks_clusters` data source ([#4014](#4014)). * Added `no_wait` option for clusters to skip waiting to start on cluster creation ([#3953](#3953)). * Introduced Plugin Framework ([#3920](#3920)). ### Bug Fixes * Add suppress diff for `azure_attributes.spot_bid_max_price` in `databricks_instance_pool` ([#3970](#3970)). * Correctly send workload_type fields in `databricks_cluster` to allow users to disable usage in certain contexts ([#3972](#3972)). * Fix `databricks_sql_table` treatment of properties ([#3925](#3925)). * Force send fields for settings resources ([#3978](#3978)). * Handle cluster deletion in `databricks_library` read ([#3909](#3909)). * Make subscriptions optional for SqlAlertTask ([#3983](#3983)). * Permanently delete `ERROR` and `TERMINATED` state clusters if their creation fails ([#4021](#4021)). ### Documentation * Add troubleshooting guide for Provider "registry.terraform.io/databricks/databricks" planned an invalid value ([#3961](#3961)). * Adopt official naming of Mosaic AI Vector Search ([#3971](#3971)). * Document Terraform 1.0 as minimum version ([#3952](#3952)). * Mention Salesforce as supported type in `databricks_connection` ([#3949](#3949)). * Reimplement Azure Databricks deployment guide to use VNet injection & NPIP ([#3986](#3986)). * Resolves [#3127](#3127): Remove deprecated account_id field from mws_credentials resource ([#3974](#3974)). * Small Grammar Corrections in Docs ([#4006](#4006)). * Update `databricks_vector_search_index` docs to match latest SDK ([#4008](#4008)). * Update aws_unity_catalog_assume_role_policy.md ([#3968](#3968)). * Update documentation regarding authentication with Azure-managed Service Principal using GITHUB OIDC ([#3932](#3932)). * Update metastore_assignment.md to properly reflect possible usage ([#3967](#3967)). * Update minimum supported terraform version to 1.1.5 ([#3965](#3965)). * Update resources diagram to include newer resources ([#3962](#3962)). * Update workspace_binding import command ([#3944](#3944)). * fix possible values for `securable_type` in `databricks_workspace_binding` ([#3942](#3942)). ### Internal Changes * Add `AddPlanModifer` method for AttributeBuilder ([#4009](#4009)). * Add integration tests for volumes and quality monitor plugin framework ([#3975](#3975)). * Add support for `computed` tag in TfSDK Structs ([#4005](#4005)). * Added `databricks_quality_monitor` resource and `databricks_volumes` data source to plugin framework ([#3958](#3958)). * Allow vector search tests to fail ([#3959](#3959)). * Clean up comments in library resource ([#4015](#4015)). * Fix irregularities in plugin framework converter function errors ([#4010](#4010)). * Make test utils public and move integration test for quality monitor ([#3993](#3993)). * Migrate Share resource to Go SDK ([#3916](#3916)). * Migrate `databricks_cluster` data source to plugin framework ([#3988](#3988)). * Migrate imports for terraform plugin framework + update init test provider factory ([#3943](#3943)). * Move volumes test next to plugin framework data source ([#3995](#3995)). * Refactor provider and related packages ([#3940](#3940)). * Support import in acceptance test + adding import state for quality monitor ([#3994](#3994)). * Library plugin framework migration ([#3979](#3979)). * Fix `TestAccClusterResource_WorkloadType` ([#3989](#3989)). ### Dependency Updates * Bump github.com/hashicorp/hcl/v2 from 2.21.0 to 2.22.0 ([#3948](#3948)). * Update Go SDK to 0.46.0 ([#4007](#4007)). ### Exporter * Don't generate instance pools if the pool name is empty ([#3960](#3960)). * Expand list of non-interactive clusters ([#4023](#4023)). * Ignore databricks_artifact_allowlist with zero artifact_matcher blocks ([#4019](#4019)). ## [Release] Release v1.51.0 ### Breaking Changes With this release, only protocol version 6 will be supported which is compatible with terraform CLI version 1.1.5 and later. If you are using an older version of the terraform CLI, please upgrade it to use this and further releases of Databricks terraform provider. ### New Features and Improvements * Automatically create `parent_path` folder when creating `databricks_dashboard resource` if it doesn't exist ([#3778](#3778)). ### Bug Fixes * Fixed logging for underlying Go SDK ([#3917](#3917)). * Remove not necessary field in `databricks_job` schema ([#3907](#3907)). ### Internal Changes * Add AttributeBuilder for Plugin Framework schema ([#3922](#3922)). * Add CustomizableSchema for Plugin Framework ([#3927](#3927)). * Add StructToSchema for Plugin Framework ([#3928](#3928)). * Add codegen template and generated files for tfsdk structs ([#3911](#3911)). * Add converter functions and tests for plugin framework ([#3914](#3914)). * Added support to use protocol version 6 provider server for SDK plugin ([#3862](#3862)). * Bump Go SDK to v0.45.0 ([#3933](#3933)). * Change name with the aliases in codegen template ([#3936](#3936)). * Update jd version from latest to 1.8.1 ([#3915](#3915)). * Upgrade `staticcheck` to v0.5.1 to get Go 1.23 support ([#3931](#3931)). * OPENAPI_SHA check ([#3935](#3935)). * Use generic error for missing clusters ([#3938](#3938)) ### Exporter * Better support for notebooks with /Workspace path ([#3901](#3901)). * Improve exporting of DLT and test coverage ([#3898](#3898)).

mgyucht added 3 commits August 21, 2024 15:36

Use ResourceProvider interface for schema definition

2e9d5a3

some progress

67347b3

Better test cases, and make properties no longer computed

f4d2f8f

mgyucht requested review from alexott and nkvuong August 21, 2024 15:01

effective_* fields should be not optional

acf2c85

mgyucht changed the title ~~[Fix] Fix databricks_sql_table~~ [Fix] Fix databricks_sql_table treatment of properties and options Aug 21, 2024

mgyucht marked this pull request as ready for review August 21, 2024 15:44

mgyucht requested review from a team as code owners August 21, 2024 15:44

mgyucht requested review from hectorcast-db and removed request for a team August 21, 2024 15:44

alexott approved these changes Aug 22, 2024

View reviewed changes

mgyucht added 3 commits August 22, 2024 16:59

run

85a591e

parse options out of response

0997e96

fix

d5a5ea5

nkvuong reviewed Aug 23, 2024

View reviewed changes

nkvuong approved these changes Aug 23, 2024

View reviewed changes

address comment

5bde37f

mgyucht changed the title ~~[Fix] Fix databricks_sql_table treatment of properties and options~~ [Fix] Fix databricks_sql_table treatment of properties Aug 23, 2024

mgyucht enabled auto-merge August 23, 2024 12:56

mgyucht added this pull request to the merge queue Aug 23, 2024

Merged via the queue into main with commit 5603eb6 Aug 23, 2024

mgyucht deleted the issue-3894 branch August 23, 2024 13:01

hectorcast-db mentioned this pull request Sep 12, 2024

[Release] Release v1.52.0 #4013

Closed

tanmay-db mentioned this pull request Sep 16, 2024

[Release] Release v1.52.0 #4022

Merged

apamildner mentioned this pull request Oct 11, 2024

[ISSUE] Issue with databricks_sql_table resource perpetual diff on options #4098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix] Fix `databricks_sql_table` treatment of properties#3925

[Fix] Fix `databricks_sql_table` treatment of properties#3925
mgyucht merged 8 commits intomainfrom
issue-3894

mgyucht commented Aug 21, 2024 •

edited

Loading

Uh oh!

alexott left a comment

Uh oh!

alexott Aug 22, 2024

Uh oh!

nkvuong Aug 23, 2024 •

edited

Loading

Uh oh!

nkvuong left a comment

Uh oh!

apamildner commented Aug 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

mgyucht commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

alexott left a comment

Choose a reason for hiding this comment

Uh oh!

alexott Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

nkvuong Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkvuong left a comment

Choose a reason for hiding this comment

Uh oh!

apamildner commented Aug 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mgyucht commented Aug 21, 2024 •

edited

Loading

nkvuong Aug 23, 2024 •

edited

Loading