Fixed updates for databricks_share resource (#2307)

Author: nkvuong

catalog/resource_share.go

@@ -5,6 +5,7 @@ import (
 	"reflect"
 	"sort"
 
+	"github.com/databricks/databricks-sdk-go/service/sharing"
 	"github.com/databricks/terraform-provider-databricks/common"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 )
@@ -84,11 +85,6 @@ func (si *ShareInfo) suppressCDFEnabledDiff() {
 	}
 }
 
-func (a SharesAPI) create(si *ShareInfo) error {
-	si.sortSharesByName()
-	return a.client.Post(a.context, "/unity-catalog/shares", si, si)
-}
-
 func (a SharesAPI) get(name string) (si ShareInfo, err error) {
 	err = a.client.Get(a.context, "/unity-catalog/shares/"+name+"?include_shared_data=true", nil, &si)
 	si.sortSharesByName()
@@ -103,10 +99,6 @@ func (a SharesAPI) update(name string, su ShareUpdates) error {
 	return a.client.Patch(a.context, "/unity-catalog/shares/"+name, su)
 }
 
-func (a SharesAPI) delete(name string) error {
-	return a.client.Delete(a.context, "/unity-catalog/shares/"+name, nil)
-}
-
 func (si ShareInfo) shareChanges(action string) ShareUpdates {
 	var changes []ShareDataChange
 	for _, obj := range si.Objects {
@@ -128,19 +120,23 @@ func (si ShareInfo) resourceShareMap() map[string]SharedDataObject {
 	return m
 }
 
-func Equal(a, b SharedDataObject) bool {
-	if b.SharedAs == "" {
-		b.SharedAs = a.SharedAs
+func (sdo SharedDataObject) Equal(other SharedDataObject) bool {
+	if other.SharedAs == "" {
+		other.SharedAs = sdo.SharedAs
 	}
-	return reflect.DeepEqual(a, b)
+	//don't compare computed fields
+	other.AddedAt = sdo.AddedAt
+	other.AddedBy = sdo.AddedBy
+	other.Status = sdo.Status
+	return reflect.DeepEqual(sdo, other)
 }
 
-func (si ShareInfo) Diff(other ShareInfo) []ShareDataChange {
-	beforeMap := si.resourceShareMap()
-	afterMap := other.resourceShareMap()
+func (beforeSi ShareInfo) Diff(afterSi ShareInfo) []ShareDataChange {
+	beforeMap := beforeSi.resourceShareMap()
+	afterMap := afterSi.resourceShareMap()
 	changes := []ShareDataChange{}
 	// not in after so remove
-	for _, beforeSdo := range si.Objects {
+	for _, beforeSdo := range beforeSi.Objects {
 		_, exists := afterMap[beforeSdo.Name]
 		if exists {
 			continue
@@ -153,10 +149,12 @@ func (si ShareInfo) Diff(other ShareInfo) []ShareDataChange {
 
 	// not in before so add
 	// if in before but diff then update
-	for _, afterSdo := range other.Objects {
+	for _, afterSdo := range afterSi.Objects {
 		beforeSdo, exists := beforeMap[afterSdo.Name]
 		if exists {
-			if !Equal(beforeSdo, afterSdo) {
+			if !beforeSdo.Equal(afterSdo) {
+				// do not send SharedAs
+				afterSdo.SharedAs = ""
 				changes = append(changes, ShareDataChange{
 					Action:     ShareUpdate,
 					DataObject: afterSdo,
@@ -179,16 +177,24 @@ func ResourceShare() *schema.Resource {
 	return common.Resource{
 		Schema: shareSchema,
 		Create: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
-			var si ShareInfo
-			common.DataToStructPointer(d, shareSchema, &si)
-			if err := NewSharesAPI(ctx, c).create(&si); err != nil {
+			w, err := c.WorkspaceClient()
+			if err != nil {
+				return err
+			}
+
+			var createRequest sharing.CreateShare
+			common.DataToStructPointer(d, shareSchema, &createRequest)
+			if _, err := w.Shares.Create(ctx, createRequest); err != nil {
 				return err
 			}
+
 			//can only create empty share, objects have to be added using update API
+			var si ShareInfo
+			common.DataToStructPointer(d, shareSchema, &si)
 			shareChanges := si.shareChanges(ShareAdd)
 			if err := NewSharesAPI(ctx, c).update(si.Name, shareChanges); err != nil {
 				//delete orphaned share if update fails
-				if d_err := NewSharesAPI(ctx, c).delete(si.Name); d_err != nil {
+				if d_err := w.Shares.DeleteByName(ctx, si.Name); d_err != nil {
 					return d_err
 				}
 				return err
@@ -204,19 +210,23 @@ func ResourceShare() *schema.Resource {
 			return common.StructToData(si, shareSchema, d)
 		},
 		Update: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
-			si, err := NewSharesAPI(ctx, c).get(d.Id())
+			beforeSi, err := NewSharesAPI(ctx, c).get(d.Id())
 			if err != nil {
 				return err
 			}
-			var shareInfo ShareInfo
-			common.DataToStructPointer(d, shareSchema, &shareInfo)
-			changes := si.Diff(shareInfo)
+			var afterSi ShareInfo
+			common.DataToStructPointer(d, shareSchema, &afterSi)
+			changes := beforeSi.Diff(afterSi)
 			return NewSharesAPI(ctx, c).update(d.Id(), ShareUpdates{
 				Updates: changes,
 			})
 		},
 		Delete: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
-			return NewSharesAPI(ctx, c).delete(d.Id())
+			w, err := c.WorkspaceClient()
+			if err != nil {
+				return err
+			}
+			return w.Shares.DeleteByName(ctx, d.Id())
 		},
 	}.ToResource()
 }

catalog/resource_share_test.go

@@ -184,18 +184,6 @@ func TestCreateShare(t *testing.T) {
 				Resource: "/api/2.1/unity-catalog/shares",
 				ExpectedRequest: ShareInfo{
 					Name: "a",
-					Objects: []SharedDataObject{
-						{
-							Name:           "main.a",
-							DataObjectType: "TABLE",
-							Comment:        "c",
-						},
-						{
-							Name:           "main.b",
-							DataObjectType: "TABLE",
-							Comment:        "c",
-						},
-					},
 				},
 				Response: RecipientInfo{
 					Name: "a",
@@ -431,18 +419,6 @@ func TestCreateShare_ThrowError(t *testing.T) {
 				Resource: "/api/2.1/unity-catalog/shares",
 				ExpectedRequest: ShareInfo{
 					Name: "a",
-					Objects: []SharedDataObject{
-						{
-							Name:           "main.a",
-							DataObjectType: "TABLE",
-							Comment:        "c",
-						},
-						{
-							Name:           "main.b",
-							DataObjectType: "TABLE",
-							Comment:        "c",
-						},
-					},
 				},
 				Response: apierr.APIErrorBody{
 					ErrorCode: "INVALID_REQUEST",
@@ -479,18 +455,6 @@ func TestCreateShareButPatchFails(t *testing.T) {
 				Resource: "/api/2.1/unity-catalog/shares",
 				ExpectedRequest: ShareInfo{
 					Name: "a",
-					Objects: []SharedDataObject{
-						{
-							Name:           "main.a",
-							DataObjectType: "TABLE",
-							Comment:        "c",
-						},
-						{
-							Name:           "main.b",
-							DataObjectType: "TABLE",
-							Comment:        "c",
-						},
-					},
 				},
 				Response: RecipientInfo{
 					Name: "a",
@@ -527,7 +491,7 @@ func TestCreateShareButPatchFails(t *testing.T) {
 			},
 			{
 				Method:   "DELETE",
-				Resource: "/api/2.1/unity-catalog/shares/a",
+				Resource: "/api/2.1/unity-catalog/shares/a?",
 			},
 		},
 		Resource: ResourceShare(),

docs/resources/share.md

@@ -9,6 +9,8 @@ A `databricks_share` is contained within [databricks_metastore](metastore.md) an
 
 ## Example Usage
 
+-> **Note** In Terraform configuration, it is recommended to define objects in alphabetical order of their `name` arguments, so that you get consistent and readable diff. Whenever objects are added or removed, or `name` is renamed, you'll observe a change in the majority of tasks. It's related to the fact that the current version of the provider treats `object` blocks as an ordered list. Alternatively, `object` block could have been an unordered set, though end-users would see the entire block replaced upon a change in single property of the task.
+
 Creating a Delta Sharing share and add some existing tables to it
 
 ```hcl
@@ -72,7 +74,7 @@ The following arguments are required:
 * `name` (Required) - Full name of the object, e.g. `catalog.schema.name` for a table.
 * `data_object_type` (Required) - Type of the object, currently only `TABLE` is allowed.
 * `comment` (Optional) -  Description about the object.
-* `shared_as` (Optional) - A user-provided new name for the data object within the share. If this new name is not provided, the object's original name will be used as the `shared_as` name. The `shared_as` name must be unique within a Share.
+* `shared_as` (Optional) - A user-provided new name for the data object within the share. If this new name is not provided, the object's original name will be used as the `shared_as` name. The `shared_as` name must be unique within a Share. Change forces creation of a new resource.
 * `cdf_enabled` (Optional) - Whether to enable Change Data Feed (cdf) on the shared object. When this field is set, field `history_data_sharing_status` can not be set.
 * `start_version` (Optional) -  The start version associated with the object for cdf. This allows data providers to control the lowest object version that is accessible by clients.
 * `history_data_sharing_status` (Optional) - Whether to enable history sharing, one of: `ENABLED`, `DISABLED`. When a table has history sharing enabled, recipients can query table data by version, starting from the current table version. If not specified, clients can only query starting from the version of the object at the time it was added to the share. *NOTE*: The start_version should be less than or equal the current version of the object. When this field is set, field `cdf_enabled` can not be set.