Skip to content

Commit 0c0272a

Browse files
authored
[Fix] Fixed syncing of effective fields in plugin framework implementation of share resource (#4969)
## Changes <!-- Summary of your changes that are easy to understand --> Planned objects can be less than the state objects. This can happen when someone removes an object in share resource outside the terraform (for example through UI). The changes fixes the syncing of effective fields by making sure we iterate over the planned and state objects properly. Ref: #4913 ## Tests <!-- How is this tested? Please see the checklist below and also describe any other relevant tests --> Integration tests - Test panics over main (before these changes): #5084 - Test passes on the changes in this PR. Unit tests - Tests panics before the changes ([ref](#4970)): <img width="1227" height="590" alt="image" src="https://github.com/user-attachments/assets/1268effa-41ae-44be-9f2a-c0df08cc12e2" /> - Tests passes over the changes
1 parent 5a582a6 commit 0c0272a

File tree

4 files changed

+243
-9
lines changed

4 files changed

+243
-9
lines changed

NEXT_CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
### Bug Fixes
1414

15+
* Fixed syncing of effective fields in plugin framework implementation of share resource ([#4969](https://github.com/databricks/terraform-provider-databricks/pull/4969))
1516
* Mark `storage_location` as read-only in `databricks_catalog` ([#5075](https://github.com/databricks/terraform-provider-databricks/pull/5075))
1617

1718
### Documentation

internal/providers/pluginfw/products/sharing/resource_share.go

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -445,16 +445,33 @@ func (effectiveFieldsActionRead) objectLevel(ctx context.Context, state *sharing
445445
state.SyncFieldsDuringRead(ctx, plan)
446446
}
447447

448-
func (r *ShareResource) syncEffectiveFields(ctx context.Context, plan, state ShareInfoExtended, mode effectiveFieldsAction) (ShareInfoExtended, diag.Diagnostics) {
448+
// syncEffectiveFields syncs the effective fields between existingState and newState
449+
// and returns the newState
450+
//
451+
// existingState: infrastructure values that are recorded in the existing terraform state.
452+
// newState: latest infrastructure values that are returned by the CRUD API calls.
453+
//
454+
// HCL config is compared with this newState to determine what changes are to be made
455+
// to the infrastructure and then the newState values are recorded in the terraform state.
456+
// Hence we ignore the values in existingState which are not present in newState.
457+
func (r *ShareResource) syncEffectiveFields(ctx context.Context, existingState, newState ShareInfoExtended, mode effectiveFieldsAction) (ShareInfoExtended, diag.Diagnostics) {
449458
var d diag.Diagnostics
450-
mode.resourceLevel(ctx, &state, plan.ShareInfo_SdkV2)
451-
planObjects, _ := plan.GetObjects(ctx)
452-
stateObjects, _ := state.GetObjects(ctx)
459+
mode.resourceLevel(ctx, &newState, existingState.ShareInfo_SdkV2)
460+
existingStateObjects, _ := existingState.GetObjects(ctx)
461+
newStateObjects, _ := newState.GetObjects(ctx)
453462
finalObjects := []sharing_tf.SharedDataObject_SdkV2{}
454-
for i := range stateObjects {
455-
mode.objectLevel(ctx, &stateObjects[i], planObjects[i])
456-
finalObjects = append(finalObjects, stateObjects[i])
463+
for i := range newStateObjects {
464+
// For each object in the new state, we check if it exists in the existing state
465+
// and if it does, we sync the effective fields.
466+
// If it does not exist, we keep the new state object as is.
467+
for j := range existingStateObjects {
468+
if newStateObjects[i].Name == existingStateObjects[j].Name {
469+
mode.objectLevel(ctx, &newStateObjects[i], existingStateObjects[j])
470+
break
471+
}
472+
}
473+
finalObjects = append(finalObjects, newStateObjects[i])
457474
}
458-
state.SetObjects(ctx, finalObjects)
459-
return state, d
475+
newState.SetObjects(ctx, finalObjects)
476+
return newState, d
460477
}

internal/providers/pluginfw/products/sharing/resource_acc_test.go renamed to internal/providers/pluginfw/products/sharing/resource_share_acc_test.go

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,17 @@
11
package sharing_test
22

33
import (
4+
"context"
45
"fmt"
6+
"maps"
57
"testing"
68

9+
"github.com/databricks/databricks-sdk-go"
10+
"github.com/databricks/databricks-sdk-go/service/sharing"
711
"github.com/databricks/terraform-provider-databricks/internal/acceptance"
812
"github.com/hashicorp/terraform-plugin-testing/terraform"
13+
"github.com/stretchr/testify/assert"
14+
"github.com/stretchr/testify/require"
915
)
1016

1117
const preTestTemplate = `
@@ -351,3 +357,71 @@ func TestUcAccShareReorderObject(t *testing.T) {
351357
ExpectNonEmptyPlan: true,
352358
})
353359
}
360+
361+
func TestUcAccUpdateShareOutsideTerraform(t *testing.T) {
362+
shareName := ""
363+
sharedObjectNameToAdd := ""
364+
acceptance.UnityWorkspaceLevel(t, acceptance.Step{
365+
Template: preTestTemplateSchema + `
366+
resource "databricks_share_pluginframework" "myshare" {
367+
name = "{var.STICKY_RANDOM}-terraform-delta-share-outside-terraform"
368+
object {
369+
name = databricks_schema.schema1.id
370+
data_object_type = "SCHEMA"
371+
}
372+
object {
373+
name = databricks_schema.schema3.id
374+
data_object_type = "SCHEMA"
375+
}
376+
}`,
377+
Check: func(s *terraform.State) error {
378+
resources := s.RootModule().Resources
379+
share := resources["databricks_share_pluginframework.myshare"]
380+
if share == nil {
381+
return fmt.Errorf("expected to find databricks_share_pluginframework.myshare in resources keys: %v", maps.Keys(resources))
382+
}
383+
shareName = share.Primary.Attributes["name"]
384+
assert.NotEmpty(t, shareName)
385+
386+
schema := resources["databricks_schema.schema2"]
387+
if schema == nil {
388+
return fmt.Errorf("expected to find databricks_schema.schema2 in resources keys: %v", maps.Keys(resources))
389+
}
390+
sharedObjectNameToAdd = schema.Primary.Attributes["id"]
391+
assert.NotEmpty(t, sharedObjectNameToAdd)
392+
return nil
393+
},
394+
}, acceptance.Step{
395+
PreConfig: func() {
396+
w, err := databricks.NewWorkspaceClient(&databricks.Config{})
397+
require.NoError(t, err)
398+
399+
// Add object to share outside terraform
400+
_, err = w.Shares.Update(context.Background(), sharing.UpdateShare{
401+
Name: shareName,
402+
Updates: []sharing.SharedDataObjectUpdate{
403+
{
404+
Action: sharing.SharedDataObjectUpdateActionAdd,
405+
DataObject: &sharing.SharedDataObject{
406+
Name: sharedObjectNameToAdd,
407+
DataObjectType: "SCHEMA",
408+
},
409+
},
410+
},
411+
})
412+
require.NoError(t, err)
413+
},
414+
Template: preTestTemplateSchema + `
415+
resource "databricks_share_pluginframework" "myshare" {
416+
name = "{var.STICKY_RANDOM}-terraform-delta-share-outside-terraform"
417+
object {
418+
name = databricks_schema.schema1.id
419+
data_object_type = "SCHEMA"
420+
}
421+
object {
422+
name = databricks_schema.schema3.id
423+
data_object_type = "SCHEMA"
424+
}
425+
}`,
426+
})
427+
}
Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
package sharing
2+
3+
import (
4+
"context"
5+
"testing"
6+
7+
"github.com/databricks/databricks-sdk-go/service/sharing"
8+
"github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw/converters"
9+
"github.com/stretchr/testify/assert"
10+
)
11+
12+
func TestShareSyncEffectiveFields(t *testing.T) {
13+
shareName := "test-share-name"
14+
ctx := context.Background()
15+
shares := ShareResource{}
16+
17+
tests := []struct {
18+
name string
19+
planGoSDK sharing.ShareInfo
20+
stateGoSDK sharing.ShareInfo
21+
}{
22+
{
23+
name: "plan with less objects",
24+
planGoSDK: sharing.ShareInfo{
25+
Name: shareName,
26+
Objects: []sharing.SharedDataObject{
27+
{
28+
Name: "obj-1",
29+
Partitions: []sharing.Partition{
30+
{Values: []sharing.PartitionValue{{Value: "part-1"}}},
31+
},
32+
},
33+
{
34+
Name: "obj-3",
35+
Partitions: []sharing.Partition{
36+
{Values: []sharing.PartitionValue{{Value: "part-3"}}},
37+
},
38+
},
39+
},
40+
},
41+
stateGoSDK: sharing.ShareInfo{
42+
Name: shareName,
43+
Objects: []sharing.SharedDataObject{
44+
{
45+
Name: "obj-1",
46+
Partitions: []sharing.Partition{
47+
{Values: []sharing.PartitionValue{{Value: "part-1"}}},
48+
},
49+
},
50+
{
51+
Name: "obj-2",
52+
Partitions: []sharing.Partition{
53+
{Values: []sharing.PartitionValue{{Value: "part-2"}}},
54+
},
55+
},
56+
{
57+
Name: "obj-3",
58+
Partitions: []sharing.Partition{
59+
{Values: []sharing.PartitionValue{{Value: "part-3"}}},
60+
},
61+
},
62+
},
63+
},
64+
},
65+
{
66+
name: "plan with more objects",
67+
planGoSDK: sharing.ShareInfo{
68+
Name: shareName,
69+
Objects: []sharing.SharedDataObject{
70+
{
71+
Name: "obj-1",
72+
Partitions: []sharing.Partition{
73+
{Values: []sharing.PartitionValue{{Value: "part-1"}}},
74+
},
75+
},
76+
{
77+
Name: "obj-2",
78+
Partitions: []sharing.Partition{
79+
{Values: []sharing.PartitionValue{{Value: "part-2"}}},
80+
},
81+
},
82+
{
83+
Name: "obj-3",
84+
Partitions: []sharing.Partition{
85+
{Values: []sharing.PartitionValue{{Value: "part-3"}}},
86+
},
87+
},
88+
},
89+
},
90+
stateGoSDK: sharing.ShareInfo{
91+
Name: shareName,
92+
Objects: []sharing.SharedDataObject{
93+
{
94+
Name: "obj-1",
95+
Partitions: []sharing.Partition{
96+
{Values: []sharing.PartitionValue{{Value: "part-1"}}},
97+
},
98+
},
99+
{
100+
Name: "obj-3",
101+
Partitions: []sharing.Partition{
102+
{Values: []sharing.PartitionValue{{Value: "part-3"}}},
103+
},
104+
},
105+
},
106+
},
107+
},
108+
{
109+
name: "empty plan",
110+
planGoSDK: sharing.ShareInfo{
111+
Name: shareName,
112+
Objects: []sharing.SharedDataObject{},
113+
},
114+
stateGoSDK: sharing.ShareInfo{
115+
Name: shareName,
116+
Objects: []sharing.SharedDataObject{
117+
{
118+
Name: "obj-1",
119+
Partitions: []sharing.Partition{
120+
{Values: []sharing.PartitionValue{{Value: "part-1"}}},
121+
},
122+
},
123+
},
124+
},
125+
},
126+
}
127+
128+
for _, tt := range tests {
129+
t.Run(tt.name, func(t *testing.T) {
130+
var planTFSDK ShareInfoExtended
131+
diagnostics := converters.GoSdkToTfSdkStruct(ctx, tt.planGoSDK, &planTFSDK)
132+
assert.False(t, diagnostics.HasError())
133+
134+
var stateTFSDK ShareInfoExtended
135+
diagnostics = converters.GoSdkToTfSdkStruct(ctx, tt.stateGoSDK, &stateTFSDK)
136+
assert.False(t, diagnostics.HasError())
137+
138+
_, diagnostics = shares.syncEffectiveFields(ctx, planTFSDK, stateTFSDK, effectiveFieldsActionRead{})
139+
assert.False(t, diagnostics.HasError())
140+
})
141+
}
142+
}

0 commit comments

Comments
 (0)