Apply ownership for UC objects during creation #1338

nkvuong · 2022-05-26T13:40:16Z

This PR makes the ownership behaviour for catalogs, schemas and tables the same as metastores

The current behaviour of Unity Catalog API is as follows:

databricks_catalog, databricks_metastore, databricks_external_location, databricks_schema, databricks_storage_credentials, databricks_table have an owner field
on Create, the new object’s owner field is set to the user performing the operation
to ensure this field is set correctly in Terraform - we need to patch the resource after creating it

Changes applied:

set owner during creation for UC securables
update docs
refactor into update function factory
add support for locations & credentials
remove update APIs
add unit test when cannot delete default schema
add MarkNewResource to testing fixture

nfx

Makes sense. Please introduce "update function factory" to have less repetition.

Also are you sure about tables?

catalog/resource_catalog.go

catalog/resource_catalog_test.go

catalog/resource_table.go

nkvuong · 2022-06-01T18:34:07Z

@nfx will need your help with this one:

I have now refactored the update function as a factory, so it can be reused across all the resources
The tests are failing though, as the d.HasChange(field) call does not work when calling it immediately after create (it still shows the old state, so all attributes are sent across)
There is also the problem with the API allowing name changes for most resources (unlike the id for metastore)
Also only certain fields can be updated at the same time (e.g. owner, and name), but we may just let the API throw the error

nfx

resources start to look neat! still some comments.

catalog/resource_catalog.go

catalog/resource_catalog_test.go

catalog/resource_external_location.go

catalog/resource_schema_test.go

catalog/resource_storage_credentials_test.go

catalog/resource_table_test.go

nfx · 2022-06-02T09:34:20Z

catalog/update.go

+		switch securable {
+		case "metastore":
+			return NewMetastoresAPI(ctx, c).updateMetastore(d.Id(), patch)
+		case "catalog":
+			return NewCatalogsAPI(ctx, c).updateCatalog(d.Id(), patch)
+		case "schema":
+			return NewSchemasAPI(ctx, c).updateSchema(d.Id(), patch)
+		case "table":
+			return NewTablesAPI(ctx, c).updateTable(d.Id(), patch)
+		case "external-location":
+			return NewExternalLocationsAPI(ctx, c).update(d.Id(), patch)
+		case "storage-credential":
+			return NewStorageCredentialsAPI(ctx, c).update(d.Id(), patch)


i don't like this hardcoding and tight coupling. at the same time this makes it more readable.

there are also two other options:

very explicit:

return updateFactory(func (ctx context.Context) func(string, map[string]interface{}) error { return NewCatalogsAPI(ctx, c).updateCatalog }, resourceData, client, "owner", "name", "comment", "properties")

layer leak:

update := updateFactory("/unity-catalog/catalogs", "owner", "name", "comment", "properties")

which would entail implementation like

func updateFunctionFactory(pathPrefix string, updatable string...) func(context.Context, *schema.ResourceData, *common.DatabricksClient) error { return func(... { ... return c.Patch(ctx, path.Join(pathPrefix, d.Id()), patch) } }

with this layer leak we technically don't need update methods for all securables, right?

moved to layer leak - looking relatively clean

nfx · 2022-06-02T09:51:56Z

catalog/update.go

+		case "storage-credential":
+			return NewStorageCredentialsAPI(ctx, c).update(d.Id(), patch)
+		default:
+			return nil


must be an error here ;) like "securable is not implemented for updates" or something. with "layer leak" this is not really a problem

nfx · 2022-06-02T16:06:32Z

@nkvuong please reintegrate this branch because of #1334

codecov · 2022-06-02T19:05:30Z

Codecov Report

Merging #1338 (2705c0d) into master (ddd9fe6) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1338      +/-   ##
==========================================
- Coverage   90.11%   90.10%   -0.01%     
==========================================
  Files         121      122       +1     
  Lines       10135    10111      -24     
==========================================
- Hits         9133     9111      -22     
+ Misses        638      637       -1     
+ Partials      364      363       -1

Impacted Files	Coverage Δ
catalog/resource_catalog.go	`100.00% <100.00%> (+5.71%)`	⬆️
catalog/resource_external_location.go	`100.00% <100.00%> (ø)`
catalog/resource_metastore.go	`100.00% <100.00%> (ø)`
catalog/resource_schema.go	`100.00% <100.00%> (ø)`
catalog/resource_storage_credentials.go	`100.00% <100.00%> (ø)`
catalog/resource_table.go	`100.00% <100.00%> (ø)`
catalog/update.go	`100.00% <100.00%> (ø)`
qa/testing.go	`83.72% <100.00%> (+0.12%)`	⬆️

nfx

beef up the logic of update factory

catalog/resource_catalog.go

catalog/resource_external_location.go

catalog/resource_metastore.go

catalog/update.go

Co-authored-by: Serge Smertin <259697+nfx@users.noreply.github.com>

lgtm-com · 2022-06-03T06:32:25Z

This pull request introduces 2 alerts when merging e81a9a3 into ddd9fe6 - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

nfx

Please build locally and try to create some resources with owners with d.MarkNewResource() removed from creates, as it's supposed to be done already implicitly by terraform SDK on creates.

I've pointed where to "fix" the ResourceFixture, so that it's unit-testable.

nfx · 2022-06-03T12:07:20Z

catalog/resource_catalog.go

@@ -69,7 +66,8 @@ func ResourceCatalog() *schema.Resource {
 				return fmt.Errorf("cannot remove new catalog default schema: %w", err)
 			}
 			d.SetId(ci.Name)
-			return nil
+			d.MarkNewResource()


@nkvuong are you 100% sure you need to call d.MarknewResource()? All creates should have it https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/resource.go#L833-L838

ResourceFixture has New flag, that can help. Actually, you can even modify here https://github.com/databrickslabs/terraform-provider-databricks/blob/master/qa/testing.go#L111-L129 to mark new resources for creates.

yup, adding this to the testing fixture is what's needed

nfx · 2022-06-03T15:41:04Z

catalog/resource_catalog.go

@@ -57,6 +53,7 @@ func ResourceCatalog() *schema.Resource {
 		func(m map[string]*schema.Schema) map[string]*schema.Schema {
 			return m
 		})
+	update := updateFunctionFactory("/unity-catalog/catalogs", []string{"owner", "name", "comment", "properties"})


so, as you told over slack - does it still make sense to have name updatable?

nfx · 2022-06-03T15:42:24Z

catalog/resource_metastore.go

-				patch["delta_sharing_recipient_token_lifetime_in_seconds"] =
-					d.Get("delta_sharing_recipient_token_lifetime_in_seconds")
-			}
+	update := updateFunctionFactory("/unity-catalog/metastores", []string{"owner", "name", "delta_sharing_enabled",


so, as you told over slack - does it still make sense to have name updatable?

for metastore it is fine - since there is a metastore_id 😄

nfx · 2022-06-03T15:42:41Z

catalog/resource_schema.go

@@ -17,7 +17,7 @@ func NewSchemasAPI(ctx context.Context, m interface{}) SchemasAPI {
 }

 type SchemaInfo struct {
-	Name        string            `json:"name" tf:"force_new"`


so, as you told over slack - does it still make sense to have name updatable?

catalog/resource_schema_test.go

nfx · 2022-06-03T15:45:17Z

catalog/resource_table.go

@@ -32,7 +32,7 @@ type ColumnInfo struct {
 }

 type TableInfo struct {
-	Name                  string            `json:"name" tf:"force_new"`
+	Name                  string            `json:"name"`


okay, for table names it makes total sense to rename.

docs/resources/catalog.md

nfx

👍

* Added `delta_sharing_*` support to `databricks_metastore` ([#1334](#1334)). * Added `databricks_git_credentials` pat discovery from common environment variables ([#1353](#1353)). * Added `databricks_permissions` for `databricks_pipeline` ([#1361](#1361)). * Added `network_id` to `network` block in `databricks_mws_workspaces` for GCP ([#1360](#1360)). * Added `azure_managed_identity` block to `databricks_storage_credential` and `databricks_metastore_data_access` resources ([#1354](#1354)). * Update docs regarding importing of `databricks_sql_*` resources ([#1349](#1349)). * Apply ownership for UC objects during creation ([#1338](#1338)). * Re-create purged cluster for `databricks_mount` for Google Storage ([#1333](#1333)). * Various documentation fixes ([#1350](#1350)). Updated dependency versions: * Bump google.golang.org/api from 0.80.0 to 0.81.0 * Bump gopkg.in/ini.v1 from 1.66.4 to 1.66.6 * Bump google.golang.org/api from 0.81.0 to 0.82.0 * Bump github.com/stretchr/testify from 1.7.1 to 1.7.2 * Bump github.com/hashicorp/terraform-plugin-sdk/v2 from 2.16.0 to 2.17.0

The current behaviour of Unity Catalog API is as follows: - `databricks_catalog`, `databricks_metastore`, `databricks_external_location`, `databricks_schema`, `databricks_storage_credentials`, `databricks_table` have an owner field - on Create, the new object’s owner field is set to the user performing the operation - to ensure this field is set correctly in Terraform - we need to patch the resource after creating it Changes applied: * set owner during creation for UC securables * updated docs * refactored updates into a function factory * added support for locations & credentials * removed update APIs * add unit test when cannot delete default schema * add MarkNewResource to testing fixture

* Added `delta_sharing_*` support to `databricks_metastore` ([databricks#1334](databricks#1334)). * Added `databricks_git_credentials` pat discovery from common environment variables ([databricks#1353](databricks#1353)). * Added `databricks_permissions` for `databricks_pipeline` ([databricks#1361](databricks#1361)). * Added `network_id` to `network` block in `databricks_mws_workspaces` for GCP ([databricks#1360](databricks#1360)). * Added `azure_managed_identity` block to `databricks_storage_credential` and `databricks_metastore_data_access` resources ([databricks#1354](databricks#1354)). * Update docs regarding importing of `databricks_sql_*` resources ([databricks#1349](databricks#1349)). * Apply ownership for UC objects during creation ([databricks#1338](databricks#1338)). * Re-create purged cluster for `databricks_mount` for Google Storage ([databricks#1333](databricks#1333)). * Various documentation fixes ([databricks#1350](databricks#1350)). Updated dependency versions: * Bump google.golang.org/api from 0.80.0 to 0.81.0 * Bump gopkg.in/ini.v1 from 1.66.4 to 1.66.6 * Bump google.golang.org/api from 0.81.0 to 0.82.0 * Bump github.com/stretchr/testify from 1.7.1 to 1.7.2 * Bump github.com/hashicorp/terraform-plugin-sdk/v2 from 2.16.0 to 2.17.0

nkvuong added 2 commits May 26, 2022 14:33

set owner during creation for UC securables

40daa71

update docs

46650db

nfx requested changes May 26, 2022

View reviewed changes

catalog/resource_catalog.go Outdated Show resolved Hide resolved

catalog/resource_catalog_test.go Outdated Show resolved Hide resolved

catalog/resource_table.go Outdated Show resolved Hide resolved

nkvuong marked this pull request as draft May 26, 2022 14:06

nkvuong added 2 commits May 26, 2022 15:44

fix tests

ff149dd

refactor into update function factory

f848ee8

nfx linked an issue May 30, 2022 that may be closed by this pull request

[FEATURE REQUEST] resource to manage UC tables/views owner #1267

Open

nkvuong added 2 commits June 1, 2022 19:24

add support for locations & credentials

9a415a4

Merge 'master'

cac25fd

nfx requested changes Jun 2, 2022

View reviewed changes

nfx linked an issue Jun 2, 2022 that may be closed by this pull request

[ISSUE] Unity catalog update owner sending full payload #1351

Closed

nfx mentioned this pull request Jun 2, 2022

[ISSUE] Unity catalog update owner sending full payload #1351

Closed

nkvuong added 6 commits June 2, 2022 19:38

refactor tests

64ffda0

refactor updates

7ebf935

add new tests instead

b299a26

merge master

ee84b60

fix test

8ca579c

remove update APIs

b931300

nfx requested changes Jun 2, 2022

View reviewed changes

nkvuong and others added 3 commits June 3, 2022 07:24

remove trailing slash

aeda74b

Co-authored-by: Serge Smertin <259697+nfx@users.noreply.github.com>

remove single-use variables

5905be7

Co-authored-by: Serge Smertin <259697+nfx@users.noreply.github.com>

remove single-use variables

e81a9a3

Co-authored-by: Serge Smertin <259697+nfx@users.noreply.github.com>

nkvuong added 2 commits June 3, 2022 08:26

handle create/update inside factory

6f6558e

add unit test when cannot delete default schema

c085747

nfx requested changes Jun 3, 2022

View reviewed changes

add MarkNewResource to testing fixture

d0e75f0

nkvuong added 2 commits June 3, 2022 16:07

add update tests for databricks#1351

97bc2e5

update docs

5b76734

nfx reviewed Jun 3, 2022

View reviewed changes

nkvuong added 3 commits June 3, 2022 17:04

update docs

1530720

fix tests

62f78d4

force_new for name

2705c0d

nfx approved these changes Jun 3, 2022

View reviewed changes

nfx marked this pull request as ready for review June 3, 2022 16:26

nfx merged commit cdcb3e5 into databricks:master Jun 3, 2022

nfx mentioned this pull request Jun 7, 2022

Release v0.6.0 #1362

Merged

nkvuong deleted the feature/apply_uc_owner_at_creation branch June 10, 2022 16:19

nfx mentioned this pull request Oct 13, 2022

[FEATURE REQUEST] resource to manage UC tables/views owner #1267

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply ownership for UC objects during creation #1338

Apply ownership for UC objects during creation #1338

nkvuong commented May 26, 2022 •

edited by nfx

Loading

nfx left a comment

nkvuong commented Jun 1, 2022 •

edited

Loading

nfx left a comment

nfx Jun 2, 2022

nkvuong Jun 2, 2022

nfx Jun 2, 2022

nfx commented Jun 2, 2022

codecov bot commented Jun 2, 2022 •

edited

Loading

nfx left a comment

lgtm-com bot commented Jun 3, 2022

nfx left a comment

nfx Jun 3, 2022

nkvuong Jun 3, 2022

nfx Jun 3, 2022

nfx Jun 3, 2022

nkvuong Jun 3, 2022

nfx Jun 3, 2022

nfx Jun 3, 2022

nfx left a comment

Apply ownership for UC objects during creation #1338

Apply ownership for UC objects during creation #1338

Conversation

nkvuong commented May 26, 2022 • edited by nfx Loading

nfx left a comment

Choose a reason for hiding this comment

nkvuong commented Jun 1, 2022 • edited Loading

nfx 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

nfx commented Jun 2, 2022

codecov bot commented Jun 2, 2022 • edited Loading

Codecov Report

nfx left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 3, 2022

nfx 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

nfx left a comment

Choose a reason for hiding this comment

nkvuong commented May 26, 2022 •

edited by nfx

Loading

nkvuong commented Jun 1, 2022 •

edited

Loading

codecov bot commented Jun 2, 2022 •

edited

Loading