diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 3a51ffcab4..11d882a7d5 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -10,6 +10,7 @@ ### Bug Fixes +* Fixed syncing of effective fields in plugin framework implementation of share resource ([#4969](https://github.com/databricks/terraform-provider-databricks/pull/4969)) * Mark `storage_location` as read-only in `databricks_catalog` ([#5075](https://github.com/databricks/terraform-provider-databricks/pull/5075)) ### Documentation diff --git a/internal/providers/pluginfw/products/sharing/resource_share.go b/internal/providers/pluginfw/products/sharing/resource_share.go index 179c6dd937..4f09e62f48 100644 --- a/internal/providers/pluginfw/products/sharing/resource_share.go +++ b/internal/providers/pluginfw/products/sharing/resource_share.go @@ -445,16 +445,33 @@ func (effectiveFieldsActionRead) objectLevel(ctx context.Context, state *sharing state.SyncFieldsDuringRead(ctx, plan) } -func (r *ShareResource) syncEffectiveFields(ctx context.Context, plan, state ShareInfoExtended, mode effectiveFieldsAction) (ShareInfoExtended, diag.Diagnostics) { +// syncEffectiveFields syncs the effective fields between existingState and newState +// and returns the newState +// +// existingState: infrastructure values that are recorded in the existing terraform state. +// newState: latest infrastructure values that are returned by the CRUD API calls. +// +// HCL config is compared with this newState to determine what changes are to be made +// to the infrastructure and then the newState values are recorded in the terraform state. +// Hence we ignore the values in existingState which are not present in newState. +func (r *ShareResource) syncEffectiveFields(ctx context.Context, existingState, newState ShareInfoExtended, mode effectiveFieldsAction) (ShareInfoExtended, diag.Diagnostics) { var d diag.Diagnostics - mode.resourceLevel(ctx, &state, plan.ShareInfo_SdkV2) - planObjects, _ := plan.GetObjects(ctx) - stateObjects, _ := state.GetObjects(ctx) + mode.resourceLevel(ctx, &newState, existingState.ShareInfo_SdkV2) + existingStateObjects, _ := existingState.GetObjects(ctx) + newStateObjects, _ := newState.GetObjects(ctx) finalObjects := []sharing_tf.SharedDataObject_SdkV2{} - for i := range stateObjects { - mode.objectLevel(ctx, &stateObjects[i], planObjects[i]) - finalObjects = append(finalObjects, stateObjects[i]) + for i := range newStateObjects { + // For each object in the new state, we check if it exists in the existing state + // and if it does, we sync the effective fields. + // If it does not exist, we keep the new state object as is. + for j := range existingStateObjects { + if newStateObjects[i].Name == existingStateObjects[j].Name { + mode.objectLevel(ctx, &newStateObjects[i], existingStateObjects[j]) + break + } + } + finalObjects = append(finalObjects, newStateObjects[i]) } - state.SetObjects(ctx, finalObjects) - return state, d + newState.SetObjects(ctx, finalObjects) + return newState, d } diff --git a/internal/providers/pluginfw/products/sharing/resource_acc_test.go b/internal/providers/pluginfw/products/sharing/resource_share_acc_test.go similarity index 69% rename from internal/providers/pluginfw/products/sharing/resource_acc_test.go rename to internal/providers/pluginfw/products/sharing/resource_share_acc_test.go index 82f5aae302..75d713c88f 100644 --- a/internal/providers/pluginfw/products/sharing/resource_acc_test.go +++ b/internal/providers/pluginfw/products/sharing/resource_share_acc_test.go @@ -1,11 +1,17 @@ package sharing_test import ( + "context" "fmt" + "maps" "testing" + "github.com/databricks/databricks-sdk-go" + "github.com/databricks/databricks-sdk-go/service/sharing" "github.com/databricks/terraform-provider-databricks/internal/acceptance" "github.com/hashicorp/terraform-plugin-testing/terraform" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) const preTestTemplate = ` @@ -269,3 +275,105 @@ func TestUcAccUpdateShareName(t *testing.T) { Check: shareCheckStateforID(), }) } + +const preTestTemplateSchema = ` + resource "databricks_catalog" "sandbox" { + name = "sandbox{var.STICKY_RANDOM}" + comment = "this catalog is managed by terraform" + properties = { + purpose = "testing" + } + } + resource "databricks_schema" "schema1" { + catalog_name = databricks_catalog.sandbox.id + name = "schema1{var.STICKY_RANDOM}" + comment = "this database is managed by terraform" + properties = { + kind = "various" + } + } + resource "databricks_schema" "schema2" { + catalog_name = databricks_catalog.sandbox.id + name = "schema2{var.STICKY_RANDOM}" + comment = "this database is managed by terraform" + properties = { + kind = "various" + } + } + resource "databricks_schema" "schema3" { + catalog_name = databricks_catalog.sandbox.id + name = "schema3{var.STICKY_RANDOM}" + comment = "this database is managed by terraform" + properties = { + kind = "various" + } + } +` + +func TestUcAccUpdateShareOutsideTerraform(t *testing.T) { + shareName := "" + sharedObjectNameToAdd := "" + acceptance.UnityWorkspaceLevel(t, acceptance.Step{ + Template: preTestTemplateSchema + ` + resource "databricks_share_pluginframework" "myshare" { + name = "{var.STICKY_RANDOM}-terraform-delta-share-outside-terraform" + object { + name = databricks_schema.schema1.id + data_object_type = "SCHEMA" + } + object { + name = databricks_schema.schema3.id + data_object_type = "SCHEMA" + } + }`, + Check: func(s *terraform.State) error { + resources := s.RootModule().Resources + share := resources["databricks_share_pluginframework.myshare"] + if share == nil { + return fmt.Errorf("expected to find databricks_share_pluginframework.myshare in resources keys: %v", maps.Keys(resources)) + } + shareName = share.Primary.Attributes["name"] + assert.NotEmpty(t, shareName) + + schema := resources["databricks_schema.schema2"] + if schema == nil { + return fmt.Errorf("expected to find databricks_schema.schema2 in resources keys: %v", maps.Keys(resources)) + } + sharedObjectNameToAdd = schema.Primary.Attributes["id"] + assert.NotEmpty(t, sharedObjectNameToAdd) + return nil + }, + }, acceptance.Step{ + PreConfig: func() { + w, err := databricks.NewWorkspaceClient(&databricks.Config{}) + require.NoError(t, err) + + // Add object to share outside terraform + _, err = w.Shares.Update(context.Background(), sharing.UpdateShare{ + Name: shareName, + Updates: []sharing.SharedDataObjectUpdate{ + { + Action: sharing.SharedDataObjectUpdateActionAdd, + DataObject: &sharing.SharedDataObject{ + Name: sharedObjectNameToAdd, + DataObjectType: "SCHEMA", + }, + }, + }, + }) + require.NoError(t, err) + }, + Template: preTestTemplateSchema + ` + resource "databricks_share_pluginframework" "myshare" { + name = "{var.STICKY_RANDOM}-terraform-delta-share-outside-terraform" + object { + name = databricks_schema.schema1.id + data_object_type = "SCHEMA" + } + object { + name = databricks_schema.schema3.id + data_object_type = "SCHEMA" + } + }`, + }) +} diff --git a/internal/providers/pluginfw/products/sharing/resource_share_test.go b/internal/providers/pluginfw/products/sharing/resource_share_test.go new file mode 100644 index 0000000000..5370be6716 --- /dev/null +++ b/internal/providers/pluginfw/products/sharing/resource_share_test.go @@ -0,0 +1,142 @@ +package sharing + +import ( + "context" + "testing" + + "github.com/databricks/databricks-sdk-go/service/sharing" + "github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/converters" + "github.com/stretchr/testify/assert" +) + +func TestShareSyncEffectiveFields(t *testing.T) { + shareName := "test-share-name" + ctx := context.Background() + shares := ShareResource{} + + tests := []struct { + name string + planGoSDK sharing.ShareInfo + stateGoSDK sharing.ShareInfo + }{ + { + name: "plan with less objects", + planGoSDK: sharing.ShareInfo{ + Name: shareName, + Objects: []sharing.SharedDataObject{ + { + Name: "obj-1", + Partitions: []sharing.Partition{ + {Values: []sharing.PartitionValue{{Value: "part-1"}}}, + }, + }, + { + Name: "obj-3", + Partitions: []sharing.Partition{ + {Values: []sharing.PartitionValue{{Value: "part-3"}}}, + }, + }, + }, + }, + stateGoSDK: sharing.ShareInfo{ + Name: shareName, + Objects: []sharing.SharedDataObject{ + { + Name: "obj-1", + Partitions: []sharing.Partition{ + {Values: []sharing.PartitionValue{{Value: "part-1"}}}, + }, + }, + { + Name: "obj-2", + Partitions: []sharing.Partition{ + {Values: []sharing.PartitionValue{{Value: "part-2"}}}, + }, + }, + { + Name: "obj-3", + Partitions: []sharing.Partition{ + {Values: []sharing.PartitionValue{{Value: "part-3"}}}, + }, + }, + }, + }, + }, + { + name: "plan with more objects", + planGoSDK: sharing.ShareInfo{ + Name: shareName, + Objects: []sharing.SharedDataObject{ + { + Name: "obj-1", + Partitions: []sharing.Partition{ + {Values: []sharing.PartitionValue{{Value: "part-1"}}}, + }, + }, + { + Name: "obj-2", + Partitions: []sharing.Partition{ + {Values: []sharing.PartitionValue{{Value: "part-2"}}}, + }, + }, + { + Name: "obj-3", + Partitions: []sharing.Partition{ + {Values: []sharing.PartitionValue{{Value: "part-3"}}}, + }, + }, + }, + }, + stateGoSDK: sharing.ShareInfo{ + Name: shareName, + Objects: []sharing.SharedDataObject{ + { + Name: "obj-1", + Partitions: []sharing.Partition{ + {Values: []sharing.PartitionValue{{Value: "part-1"}}}, + }, + }, + { + Name: "obj-3", + Partitions: []sharing.Partition{ + {Values: []sharing.PartitionValue{{Value: "part-3"}}}, + }, + }, + }, + }, + }, + { + name: "empty plan", + planGoSDK: sharing.ShareInfo{ + Name: shareName, + Objects: []sharing.SharedDataObject{}, + }, + stateGoSDK: sharing.ShareInfo{ + Name: shareName, + Objects: []sharing.SharedDataObject{ + { + Name: "obj-1", + Partitions: []sharing.Partition{ + {Values: []sharing.PartitionValue{{Value: "part-1"}}}, + }, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var planTFSDK ShareInfoExtended + diagnostics := converters.GoSdkToTfSdkStruct(ctx, tt.planGoSDK, &planTFSDK) + assert.False(t, diagnostics.HasError()) + + var stateTFSDK ShareInfoExtended + diagnostics = converters.GoSdkToTfSdkStruct(ctx, tt.stateGoSDK, &stateTFSDK) + assert.False(t, diagnostics.HasError()) + + _, diagnostics = shares.syncEffectiveFields(ctx, planTFSDK, stateTFSDK, effectiveFieldsActionRead{}) + assert.False(t, diagnostics.HasError()) + }) + } +}