Migrate SQL Warehouse to Go SDK #3044

mgyucht · 2023-12-19T14:34:31Z

Changes

This PR migrates the SQL Warehouse resource to the Go SDK. It also modifies all tests that used the original API client, which means appending ? to resource URIs in fixtures.

To do this, some changes to supporting code are also made:

Introduced several helper methods in common to simplify customizing schemas computed by StructToSchema for several common cases:

SetSuppressDiff: sets a diff suppressor on the field so that fields not populated by the user in their configuration are not treated as a diff, even if the API returned some value for that field.
SetDefault: marks a schema as optional and sets a default value for it.
SetReadOnly: marks a schema as not configurable by users but that can be referenced in Terraform.

Improved suppress diff logic to work with lists/sets. If two lists have a different number of elements, the size of the list is compared, rather than individual list items. The corresponding check should be that new is "0" (not in config) and old is not "0" (state captured something).
Improved all reflection-based methods to work with embedded structs. It is now possible to use StructToSchema on structs like

type S struct {
  sdk.SdkType
  AnotherField string `json:"another_field,omitempty"`
}

The fields of the embedded field are expanded into the schema. This way, maintainers can reuse the Go type as-is without having to copy all of its fields into a new structure.

Bugfix in collectionToMaps that assumed that a data structure is a list iff MaxItems is 1. MaxItems can't be set on read-only fields. This now checks the underlying type is a slice or array.
Introduced WorkspaceDataWithCustomAttrs and AccountDataWithCustomAttrs. These functions are similar to WorkspaceData and AccountData except that they permit two type parameters. The first corresponds to the type used for the resource, ensuring that the schema of the data source and that of the resource are identical. The second corresponds to any additional data that a user might specify in the data source.

Side note: we should probably look into changing suppress diff from being a schema-level configuration to actually adjusting the returned value from the Read method. Otherwise, if the only change made by a user is e.g. to remove all tags, that won't show up as a diff.

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

# Conflicts: # Makefile # scripts/diff-schema.sh

codecov-commenter · 2023-12-21T09:45:47Z

Codecov Report

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

Comparison is base (44eefaa) 84.55% compared to head (a3f415d) 84.21%.
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3044      +/-   ##
==========================================
- Coverage   84.55%   84.21%   -0.35%     
==========================================
  Files         161      161              
  Lines       14187    14260      +73     
==========================================
+ Hits        11996    12009      +13     
- Misses       1508     1559      +51     
- Partials      683      692       +9

Files	Coverage Δ
exporter/util.go	`78.76% <100.00%> (-0.05%)`	⬇️
qa/testing.go	`80.06% <100.00%> (ø)`
sql/data_sql_warehouses.go	`100.00% <100.00%> (ø)`
exporter/importables.go	`79.77% <33.33%> (-0.12%)`	⬇️
sql/data_sql_warehouse.go	`70.37% <66.66%> (-13.31%)`	⬇️
sql/resource_sql_endpoint.go	`86.51% <85.71%> (-11.08%)`	⬇️
common/reflect_resource.go	`80.87% <52.08%> (-3.25%)`	⬇️
common/resource.go	`65.24% <0.00%> (-8.70%)`	⬇️

... and 1 file with indirect coverage changes

mgyucht · 2023-12-21T14:50:18Z

common/reflect_resource.go

@@ -175,26 +191,52 @@ func diffSuppressor(zero string) func(k, old, new string, d *schema.ResourceData
 			log.Printf("[DEBUG] Suppressing diff for %v: platform=%#v config=%#v", k, old, new)
 			return true
 		}
+		if strings.HasSuffix(".#", k) && new == "0" && old != "0" {


Lists and sets are compared first for size, which end with .# and the values provided are the sizes of the list/set as a string. If the API returned a non-nil value like {}, the resulting state will have an entry (e.g. for SQL Warehouses, it will be a [{"custom_tags": []}]. The "new" config will be completely empty because it is not defined in the resource itself.

mgyucht · 2023-12-21T22:39:50Z

sql/data_sql_warehouses.go

 		if err != nil {
 			return err
 		}
 		name_contains := (*data).WarehouseNameContains
-		for _, e := range list.Endpoints {
+		for _, e := range list {
 			match_name := strings.Contains(strings.ToLower(e.Name), name_contains)


Oh, I was just preserving the existing behavior. Can we make any changes as a follow-up?

mgyucht · 2023-12-21T22:40:12Z

common/resource.go

+	}, false)
+}
+
+func WorkspaceData2[SdkType, OtherType any](read func(context.Context, OtherType, *SdkType, *databricks.WorkspaceClient) error) *schema.Resource {


Yeah this name is not very good. I was planning on revisiting this & adding the account-level version in this PR as well.

mgyucht · 2023-12-22T08:15:24Z

sql/resource_sql_endpoint.go

@@ -214,33 +85,58 @@ func ResourceSqlEndpoint() *schema.Resource {
 			Create: schema.DefaultTimeout(30 * time.Minute),
 		},
 		Create: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
-			var se SQLEndpoint
-			common.DataToStructPointer(d, s, &se)
-			if err := NewSQLEndpointsAPI(ctx, c).Create(&se, d.Timeout(schema.TimeoutCreate)); err != nil {


I think the context itself has a timeout. https://developer.hashicorp.com/terraform/plugin/sdkv2/resources/retries-and-customizable-timeouts#default-timeouts-and-deadline-exceeded-errors

edwardfeng-db

Halfway through reviewing, need a bit more time understanding context on the WorkspaceData and AccountData

edwardfeng-db · 2024-01-08T10:05:21Z

common/reflect_resource.go

@@ -260,7 +317,7 @@ func typeToSchema(v reflect.Value, t reflect.Type, path []string) map[string]*sc
 			scm[fieldName].Type = schema.TypeList
 			elem := typeField.Type.Elem()
 			sv := reflect.New(elem).Elem()
-			nestedSchema := typeToSchema(sv, elem, append(path, fieldName, "0"))
+			nestedSchema := typeToSchema(sv, append(path, fieldName, "0"))
 			if strings.Contains(tfTag, "suppress_diff") {
 				blockCount := strings.Join(append(path, fieldName, "#"), ".")
 				scm[fieldName].DiffSuppressFunc = makeEmptyBlockSuppressFunc(blockCount)


With the new suffix checking in diffSuppressor, can we replace makeEmptyBlockSuppressFunc calls?

Maybe we can do this as a separate PR?

edwardfeng-db · 2024-01-08T13:07:36Z

sql/resource_sql_endpoint.go

 		m map[string]*schema.Schema) map[string]*schema.Schema {
+		m["id"].Computed = true


Can we use common.SetReadOnly for this one?

alexott

in general, looks good for me from the first pass...

alexott · 2024-01-08T18:25:23Z

common/reflect_resource.go

+
+func listAllFields(v reflect.Value) []field {
+	t := v.Type()
+	fields := make([]field, 0)


what about doing

fields := make([]field, 0, v.NumField())

to avoid reallocations?

alexott · 2024-01-08T18:41:29Z

common/resource.go

+//	 }
+//
+//	 WorkspaceDataWithParams(
+//	     func(ctx context.Context, data SqlWarehouseDataParams, w *databricks.WorkspaceClient) (*Warehouse, error) {


*Warehouse should be *SqlWarehouse ?

alexott · 2024-01-08T18:49:23Z

exporter/exporter_test.go

@@ -313,7 +314,7 @@ var emptyWorkspace = qa.HTTPFixture{

 var emptySqlEndpoints = qa.HTTPFixture{
 	Method:       "GET",
-	Resource:     "/api/2.0/sql/warehouses",
+	Resource:     "/api/2.0/sql/warehouses?",


Can we fix SDK to not to add ? if there are no parameters?

We could. Let's talk online about testing for TF, we just added some testing interfaces which should hide the network layer from you (if you want). Take a look at databricks/databricks-sdk-go@b3cdc97 for the change. That would allow you to mock the responses from Databricks more easily.

alexott · 2024-01-08T18:59:27Z

sql/resource_sql_endpoint.go

 		m["cluster_size"].ValidateDiagFunc = validation.ToDiagFunc(
 			validation.StringInSlice(ClusterSizes, false))
+		common.SetDefault(m["enable_photon"], true)
+		common.SetSuppressDiff(m["enable_serverless_compute"])


Should we try to handle force send fields here for serverless & photon?

I was going to do that in a follow-up PR, in case that change causes issues and we need to revert.

alexott · 2024-01-08T19:18:32Z

common/reflect_resource_test.go

@mgyucht I think that we need to add at least two new test cases:

For inherited structure - so we can see that it's flattened correctly into schema

For a structure with a field from Go SDK

Migrate SQL Warehouse to Go SDK

8ca7989

mgyucht requested review from a team as code owners December 19, 2023 14:34

mgyucht requested review from tanmay-db and removed request for a team December 19, 2023 14:34

mgyucht added 21 commits December 20, 2023 09:28

fix compile

3ab53a2

check in diff-schema scrip

97f0f43

Merge branch 'master' into migrate-sql-warehouse-to-go-sdk

a69d5f7

fix

69b892b

fix

426598b

test fix

97b0643

fix

a6e5fb5

fix

05184aa

Merge branch 'master' into migrate-sql-warehouse-to-go-sdk

4f62c47

# Conflicts: # Makefile # scripts/diff-schema.sh

undo extra change

62e5c44

work

dcc0d59

work

ff1234d

work

faeb466

fix

d80936c

fixes

6f1218e

fix

8cb7951

remove

045a26f

handle slices as well

5079eca

Merge branch 'master' into migrate-sql-warehouse-to-go-sdk

e8ecc58

fix stubs in exporter test

8c113a5

test

3f3aca5

mgyucht requested a review from alexott December 21, 2023 12:19

mgyucht added 2 commits December 21, 2023 14:20

Fix tags diff suppression

0c7d9c6

fix

a45c93b

mgyucht added 8 commits December 22, 2023 09:56

fix workflow

387ed42

fix

8ecc720

Merge branch 'master' into migrate-sql-warehouse-to-go-sdk

f1dae30

only diff against merge-base of master

adb8ed1

update docs

63ae324

remove deprecated resources

e08fbe8

undo extra docs changes

20c3d14

remove extra space

61ed26e

mgyucht commented Dec 22, 2023

View reviewed changes

mgyucht requested a review from nkvuong December 22, 2023 09:21

Merge branch 'master' into migrate-sql-warehouse-to-go-sdk

a33b53d

edwardfeng-db reviewed Jan 8, 2024

View reviewed changes

mgyucht enabled auto-merge January 8, 2024 10:35

small tweak

a3f415d

edwardfeng-db reviewed Jan 8, 2024

View reviewed changes

small tweaks

0a069b7

alexott reviewed Jan 8, 2024

View reviewed changes

edwardfeng-db mentioned this pull request Jan 8, 2024

Migrate cluster schema to use the go-sdk struct #3076

Merged

5 tasks

alexott mentioned this pull request Jan 9, 2024

Added filter block to databricks_instance_profiles data source #2988

Open

5 tasks

edwardfeng-db approved these changes Jan 9, 2024

View reviewed changes

mgyucht added this pull request to the merge queue Jan 9, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2024

mgyucht added 2 commits January 9, 2024 16:37

Merge branch 'master' into migrate-sql-warehouse-to-go-sdk

56e3b46

tests and address comments

ce5cec7

mgyucht added this pull request to the merge queue Jan 10, 2024

Merged via the queue into master with commit d3427f5 Jan 10, 2024
5 checks passed

mgyucht deleted the migrate-sql-warehouse-to-go-sdk branch January 10, 2024 09:12

tanmay-db mentioned this pull request Jan 11, 2024

Release v1.34.0 #3105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate SQL Warehouse to Go SDK #3044

Migrate SQL Warehouse to Go SDK #3044

mgyucht commented Dec 19, 2023 •

edited

Loading

codecov-commenter commented Dec 21, 2023 •

edited

Loading

mgyucht Dec 21, 2023

mgyucht Dec 21, 2023

mgyucht Dec 21, 2023

mgyucht Dec 22, 2023

edwardfeng-db left a comment

edwardfeng-db Jan 8, 2024

mgyucht Jan 8, 2024

edwardfeng-db Jan 8, 2024

alexott left a comment

alexott Jan 8, 2024

alexott Jan 8, 2024

alexott Jan 8, 2024

mgyucht Jan 9, 2024

alexott Jan 8, 2024

mgyucht Jan 9, 2024

alexott Jan 8, 2024

		m map[string]schema.Schema) map[string]schema.Schema {
		m["id"].Computed = true

Migrate SQL Warehouse to Go SDK #3044

Migrate SQL Warehouse to Go SDK #3044

Conversation

mgyucht commented Dec 19, 2023 • edited Loading

Changes

Tests

codecov-commenter commented Dec 21, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardfeng-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgyucht commented Dec 19, 2023 •

edited

Loading

codecov-commenter commented Dec 21, 2023 •

edited

Loading