-
Notifications
You must be signed in to change notification settings - Fork 386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate SQL Warehouse to Go SDK #3044
Changes from 24 commits
8ca7989
3ab53a2
97f0f43
a69d5f7
69b892b
426598b
97b0643
a6e5fb5
05184aa
4f62c47
62e5c44
dcc0d59
ff1234d
faeb466
d80936c
6f1218e
8cb7951
045a26f
5079eca
e8ecc58
8c113a5
3f3aca5
0c7d9c6
a45c93b
1687e2f
fc902b3
387ed42
8ecc720
f1dae30
adb8ed1
63ae324
e08fbe8
20c3d14
61ed26e
a33b53d
a3f415d
0a069b7
56e3b46
ce5cec7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,7 +77,7 @@ func MustSchemaPath(s map[string]*schema.Schema, path ...string) *schema.Schema | |
// StructToSchema makes schema from a struct type & applies customizations from callback given | ||
func StructToSchema(v any, customize func(map[string]*schema.Schema) map[string]*schema.Schema) map[string]*schema.Schema { | ||
rv := reflect.ValueOf(v) | ||
scm := typeToSchema(rv, rv.Type(), []string{}) | ||
scm := typeToSchema(rv, []string{}) | ||
if customize != nil { | ||
scm = customize(scm) | ||
} | ||
|
@@ -145,6 +145,22 @@ func handleSuppressDiff(typeField reflect.StructField, v *schema.Schema) { | |
} | ||
} | ||
|
||
func SetSuppressDiff(v *schema.Schema) { | ||
v.DiffSuppressFunc = diffSuppressor(fmt.Sprintf("%v", v.Type.Zero())) | ||
} | ||
|
||
func SetDefault(v *schema.Schema, value any) { | ||
v.Default = value | ||
v.Optional = true | ||
} | ||
|
||
func SetReadOnly(v *schema.Schema) { | ||
v.Optional = false | ||
v.Required = false | ||
v.MaxItems = 0 | ||
v.Computed = true | ||
} | ||
|
||
func getAlias(typeField reflect.StructField) string { | ||
tfTags := strings.Split(typeField.Tag.Get("tf"), ",") | ||
for _, tag := range tfTags { | ||
|
@@ -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" { | ||
field := strings.TrimSuffix(k, ".#") | ||
log.Printf("[DEBUG] Suppressing diff for list or set %v: no value configured but platform returned some value (likely {})", field) | ||
return true | ||
} | ||
return false | ||
} | ||
} | ||
|
||
type field struct { | ||
sf reflect.StructField | ||
v reflect.Value | ||
} | ||
|
||
func listAllFields(v reflect.Value) []field { | ||
t := v.Type() | ||
fields := make([]field, 0) | ||
for i := 0; i < v.NumField(); i++ { | ||
f := t.Field(i) | ||
if f.Anonymous { | ||
fields = append(fields, listAllFields(v.Field(i))...) | ||
} else { | ||
fields = append(fields, field{ | ||
sf: f, | ||
v: v.Field(i), | ||
}) | ||
} | ||
} | ||
return fields | ||
} | ||
|
||
// typeToSchema converts struct into terraform schema. `path` is used for block suppressions | ||
// special path element `"0"` is used to denote either arrays or sets of elements | ||
func typeToSchema(v reflect.Value, t reflect.Type, path []string) map[string]*schema.Schema { | ||
func typeToSchema(v reflect.Value, path []string) map[string]*schema.Schema { | ||
scm := map[string]*schema.Schema{} | ||
rk := v.Kind() | ||
if rk == reflect.Ptr { | ||
v = v.Elem() | ||
t = v.Type() | ||
rk = v.Kind() | ||
} | ||
if rk != reflect.Struct { | ||
panic(fmt.Errorf("Schema value of Struct is expected, but got %s: %#v", reflectKind(rk), v)) | ||
} | ||
for i := 0; i < v.NumField(); i++ { | ||
typeField := t.Field(i) | ||
|
||
fields := listAllFields(v) | ||
for _, field := range fields { | ||
typeField := field.sf | ||
tfTag := typeField.Tag.Get("tf") | ||
|
||
fieldName := chooseFieldName(typeField) | ||
|
@@ -260,7 +302,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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the new suffix checking in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can do this as a separate PR? |
||
|
@@ -279,7 +321,7 @@ func typeToSchema(v reflect.Value, t reflect.Type, path []string) map[string]*sc | |
elem := typeField.Type // changed from ptr | ||
sv := reflect.New(elem) // changed from ptr | ||
|
||
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) | ||
|
@@ -310,7 +352,7 @@ func typeToSchema(v reflect.Value, t reflect.Type, path []string) map[string]*sc | |
case reflect.Struct: | ||
sv := reflect.New(elem).Elem() | ||
scm[fieldName].Elem = &schema.Resource{ | ||
Schema: typeToSchema(sv, elem, append(path, fieldName, "0")), | ||
Schema: typeToSchema(sv, append(path, fieldName, "0")), | ||
} | ||
} | ||
default: | ||
|
@@ -341,6 +383,20 @@ func IsRequestEmpty(v any) (bool, error) { | |
return !isNotEmpty, err | ||
} | ||
|
||
// isGoSdk returns true if the struct is from databricks-sdk-go or embeds a struct from databricks-sdk-go. | ||
func isGoSdk(v reflect.Value) bool { | ||
if strings.Contains(v.Type().PkgPath(), "databricks-sdk-go") { | ||
return true | ||
} | ||
for i := 0; i < v.NumField(); i++ { | ||
f := v.Type().Field(i) | ||
if f.Anonymous && isGoSdk(v.Field(i)) { | ||
return true | ||
} | ||
} | ||
return false | ||
} | ||
|
||
func iterFields(rv reflect.Value, path []string, s map[string]*schema.Schema, | ||
cb func(fieldSchema *schema.Schema, path []string, valueField *reflect.Value) error) error { | ||
rk := rv.Kind() | ||
|
@@ -350,9 +406,10 @@ func iterFields(rv reflect.Value, path []string, s map[string]*schema.Schema, | |
if !rv.IsValid() { | ||
return fmt.Errorf("%s: got invalid reflect value %#v", path, rv) | ||
} | ||
isGoSDK := strings.Contains(rv.Type().PkgPath(), "databricks-sdk-go") | ||
for i := 0; i < rv.NumField(); i++ { | ||
typeField := rv.Type().Field(i) | ||
isGoSDK := isGoSdk(rv) | ||
fields := listAllFields(rv) | ||
for _, field := range fields { | ||
typeField := field.sf | ||
fieldName := chooseFieldName(typeField) | ||
if fieldName == "-" { | ||
continue | ||
|
@@ -370,7 +427,7 @@ func iterFields(rv reflect.Value, path []string, s map[string]*schema.Schema, | |
if fieldSchema.Optional && defaultEmpty && !omitEmpty { | ||
return fmt.Errorf("inconsistency: %s is optional, default is empty, but has no omitempty", fieldName) | ||
} | ||
valueField := rv.Field(i) | ||
valueField := field.v | ||
err := cb(fieldSchema, append(path, fieldName), &valueField) | ||
if err != nil { | ||
return fmt.Errorf("%s: %s", fieldName, err) | ||
|
@@ -393,13 +450,15 @@ func collectionToMaps(v any, s *schema.Schema) ([]any, error) { | |
return nil, fmt.Errorf("not resource") | ||
} | ||
var allItems []reflect.Value | ||
if s.MaxItems == 1 { | ||
allItems = append(allItems, reflect.ValueOf(v)) | ||
} else { | ||
vs := reflect.ValueOf(v) | ||
for i := 0; i < vs.Len(); i++ { | ||
allItems = append(allItems, vs.Index(i)) | ||
rv := reflect.ValueOf(v) | ||
rvType := rv.Type().Kind() | ||
isList := rvType == reflect.Array || rvType == reflect.Slice | ||
if isList { | ||
for i := 0; i < rv.Len(); i++ { | ||
allItems = append(allItems, rv.Index(i)) | ||
} | ||
} else { | ||
allItems = append(allItems, rv) | ||
} | ||
for _, v := range allItems { | ||
data := map[string]any{} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mgyucht I think that we need to add at least two new test cases:
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -256,9 +256,17 @@ func DataResource(sc any, read func(context.Context, any, *DatabricksClient) err | |
// ... | ||
// }) | ||
func WorkspaceData[T any](read func(context.Context, *T, *databricks.WorkspaceClient) error) *schema.Resource { | ||
return genericDatabricksData(func(c *DatabricksClient) (*databricks.WorkspaceClient, error) { | ||
return genericDatabricksData[T, struct{}, *databricks.WorkspaceClient](func(c *DatabricksClient) (*databricks.WorkspaceClient, error) { | ||
return c.WorkspaceClient() | ||
}, read) | ||
}, func(ctx context.Context, s struct{}, t *T, wc *databricks.WorkspaceClient) error { | ||
return read(ctx, t, wc) | ||
}, false) | ||
} | ||
|
||
func WorkspaceData2[SdkType, OtherType any](read func(context.Context, OtherType, *SdkType, *databricks.WorkspaceClient) error) *schema.Resource { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we name this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this name is not very good. I was planning on revisiting this & adding the account-level version in this PR as well. |
||
return genericDatabricksData[SdkType, OtherType, *databricks.WorkspaceClient](func(c *DatabricksClient) (*databricks.WorkspaceClient, error) { | ||
return c.WorkspaceClient() | ||
}, read, true) | ||
} | ||
|
||
// AccountData is a generic way to define account data resources in Terraform provider. | ||
|
@@ -273,15 +281,36 @@ func WorkspaceData[T any](read func(context.Context, *T, *databricks.WorkspaceCl | |
// ... | ||
// }) | ||
func AccountData[T any](read func(context.Context, *T, *databricks.AccountClient) error) *schema.Resource { | ||
return genericDatabricksData(func(c *DatabricksClient) (*databricks.AccountClient, error) { | ||
return genericDatabricksData[T, struct{}, *databricks.AccountClient](func(c *DatabricksClient) (*databricks.AccountClient, error) { | ||
return c.AccountClient() | ||
}, read) | ||
}, func(ctx context.Context, s struct{}, t *T, ac *databricks.AccountClient) error { | ||
return read(ctx, t, ac) | ||
}, false) | ||
} | ||
|
||
// genericDatabricksData is generic and common way to define both account and workspace data and calls their respective clients | ||
func genericDatabricksData[T any, C any](getClient func(*DatabricksClient) (C, error), read func(context.Context, *T, C) error) *schema.Resource { | ||
var dummy T | ||
func genericDatabricksData[SdkType, OtherFields any, C any]( | ||
getClient func(*DatabricksClient) (C, error), | ||
read func(context.Context, OtherFields, *SdkType, C) error, | ||
hasOther bool) *schema.Resource { | ||
var dummy SdkType | ||
var other OtherFields | ||
otherFields := StructToSchema(other, NoCustomize) | ||
s := StructToSchema(dummy, func(m map[string]*schema.Schema) map[string]*schema.Schema { | ||
// For WorkspaceData and AccountData, a single data type is used to represent all of the fields of | ||
// the resource, so its configuration is correct. For WorkspaceData2, the SdkType parameter is copied | ||
// directly from the resource definition, which means that all fields from that type are computed and | ||
// optional, and the fields from OtherFields are overlaid on top of the schema generated by SdkType. | ||
if hasOther { | ||
for k := range m { | ||
m[k].Computed = true | ||
m[k].Required = false | ||
m[k].Optional = true | ||
} | ||
for k, v := range otherFields { | ||
m[k] = v | ||
} | ||
} | ||
// `id` attribute must be marked as computed, otherwise it's not set! | ||
if v, ok := m["id"]; ok { | ||
v.Computed = true | ||
|
@@ -298,20 +327,22 @@ func genericDatabricksData[T any, C any](getClient func(*DatabricksClient) (C, e | |
diags = diag.Errorf("panic: %v", panic) | ||
} | ||
}() | ||
ptr := reflect.New(reflect.ValueOf(dummy).Type()) | ||
DataToReflectValue(d, &schema.Resource{Schema: s}, ptr.Elem()) | ||
var dummy SdkType | ||
var other OtherFields | ||
DataToStructPointer(d, s, &other) | ||
DataToStructPointer(d, s, &dummy) | ||
client := m.(*DatabricksClient) | ||
c, err := getClient(client) | ||
if err != nil { | ||
err = nicerError(ctx, err, "read data") | ||
err = nicerError(ctx, err, "get client") | ||
return diag.FromErr(err) | ||
} | ||
err = read(ctx, ptr.Interface().(*T), c) | ||
err = read(ctx, other, &dummy, c) | ||
if err != nil { | ||
err = nicerError(ctx, err, "read data") | ||
diags = diag.FromErr(err) | ||
} | ||
StructToData(ptr.Elem().Interface(), s, d) | ||
StructToData(&dummy, s, d) | ||
// check if the resource schema has the `id` attribute (marked with `json:"id"` in the provided structure). | ||
// and if yes, then use it as resource ID. If not, then use default value for resource ID (`_`) | ||
if _, ok := s["id"]; ok { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ import ( | |
"github.com/databricks/databricks-sdk-go/service/ml" | ||
"github.com/databricks/databricks-sdk-go/service/serving" | ||
"github.com/databricks/databricks-sdk-go/service/settings" | ||
"github.com/databricks/databricks-sdk-go/service/sql" | ||
workspaceApi "github.com/databricks/databricks-sdk-go/service/workspace" | ||
"github.com/databricks/terraform-provider-databricks/aws" | ||
"github.com/databricks/terraform-provider-databricks/clusters" | ||
|
@@ -29,7 +30,7 @@ import ( | |
"github.com/databricks/terraform-provider-databricks/repos" | ||
"github.com/databricks/terraform-provider-databricks/scim" | ||
"github.com/databricks/terraform-provider-databricks/secrets" | ||
"github.com/databricks/terraform-provider-databricks/sql" | ||
tfsql "github.com/databricks/terraform-provider-databricks/sql" | ||
"github.com/databricks/terraform-provider-databricks/workspace" | ||
"github.com/hashicorp/hcl/v2/hclwrite" | ||
|
||
|
@@ -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?", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we fix SDK to not to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
Response: map[string]any{}, | ||
ReuseRequest: true, | ||
} | ||
|
@@ -342,7 +343,7 @@ var emptySqlQueries = qa.HTTPFixture{ | |
var emptySqlAlerts = qa.HTTPFixture{ | ||
Method: "GET", | ||
Resource: "/api/2.0/preview/sql/alerts", | ||
Response: []sql.AlertEntity{}, | ||
Response: []tfsql.AlertEntity{}, | ||
ReuseRequest: true, | ||
} | ||
|
||
|
@@ -369,7 +370,7 @@ var allKnownWorkspaceConfs = qa.HTTPFixture{ | |
var emptyGlobalSQLConfig = qa.HTTPFixture{ | ||
Method: "GET", | ||
Resource: "/api/2.0/sql/config/warehouses", | ||
Response: sql.GlobalConfigForRead{}, | ||
Response: tfsql.GlobalConfigForRead{}, | ||
ReuseRequest: true, | ||
} | ||
|
||
|
@@ -1714,21 +1715,21 @@ func TestImportingSqlObjects(t *testing.T) { | |
}, | ||
{ | ||
Method: "GET", | ||
Resource: "/api/2.0/sql/warehouses", | ||
Resource: "/api/2.0/sql/warehouses?", | ||
Response: getJSONObject("test-data/get-sql-endpoints.json"), | ||
}, | ||
{ | ||
Method: "GET", | ||
Resource: "/api/2.0/sql/warehouses/f562046bc1272886", | ||
Resource: "/api/2.0/sql/warehouses/f562046bc1272886?", | ||
Response: getJSONObject("test-data/get-sql-endpoint.json"), | ||
}, | ||
{ | ||
Method: "GET", | ||
Resource: "/api/2.0/preview/sql/data_sources", | ||
Response: []sql.DataSource{ | ||
{ | ||
ID: "147164a6-8316-4a9d-beff-f57261801374", | ||
EndpointID: "f562046bc1272886", | ||
Id: "147164a6-8316-4a9d-beff-f57261801374", | ||
WarehouseId: "f562046bc1272886", | ||
}, | ||
}, | ||
ReuseRequest: true, | ||
|
@@ -2044,15 +2045,24 @@ func TestImportingGlobalSqlConfig(t *testing.T) { | |
meAdminFixture, | ||
{ | ||
Method: "GET", | ||
Resource: "/api/2.0/sql/warehouses", | ||
Response: sql.EndpointList{}, | ||
Resource: "/api/2.0/sql/warehouses?", | ||
Response: sql.ListWarehousesResponse{}, | ||
}, | ||
{ | ||
Method: "GET", | ||
Resource: "/api/2.0/sql/config/warehouses", | ||
Response: sql.GlobalConfigForRead{ | ||
EnableServerlessCompute: true, | ||
InstanceProfileARN: "arn:...", | ||
Response: sql.GetWorkspaceWarehouseConfigResponse{ | ||
EnabledWarehouseTypes: []sql.WarehouseTypePair{ | ||
{ | ||
WarehouseType: sql.WarehouseTypePairWarehouseTypeClassic, | ||
Enabled: true, | ||
}, | ||
{ | ||
WarehouseType: sql.WarehouseTypePairWarehouseTypePro, | ||
Enabled: true, | ||
}, | ||
}, | ||
InstanceProfileArn: "arn:...", | ||
}, | ||
}, | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about doing
to avoid reallocations?