Skip to content

Commit d84bc6f

Browse files
alexottalexott
authored
Fix regression with databricks_group data source introduced by a recent change (#4995)
## Changes <!-- Summary of your changes that are easy to understand --> It looks like some users are using the `databricks_group` data source as non-admin users (in some cases, incorrectly). Recent change in SCIM API required to change from List to List for `id` + Get to retrieve group members, but it had an unexpected effect that Get for non-admin users throws an error as we request for `externalId` attribute. I got clarification from the SCIM team that changes in the `members` retrieval are affecting only the account-level API, and not the workspace-level API. So this PR reimplements the required changes as follows: - on account level we do `List` for group ID by name, followed by `Get` to retrieve `members`, `externalId`, etc. - on workspace level we use the old implementation of `List` + default attributes returned by filter operation Resolves #4994 Resolves #4996 ## Tests <!-- How is this tested? Please see the checklist below and also describe any other relevant tests --> - [x] `make test` run locally - [ ] relevant change in `docs/` folder - [ ] covered with integration tests in `internal/acceptance` - [ ] using Go SDK - [ ] using TF Plugin Framework - [x] has entry in `NEXT_CHANGELOG.md` file
1 parent 7399207 commit d84bc6f

File tree

3 files changed

+118
-51
lines changed

3 files changed

+118
-51
lines changed

‎NEXT_CHANGELOG.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,11 @@
1010

1111
### Bug Fixes
1212

13+
* Fix regression with `databricks_group` data source introduced by a recent change ([#4995](https://github.com/databricks/terraform-provider-databricks/pull/4995))
14+
1315
### Documentation
14-
* Document `continuous.task_retry_mode` in `databricks_job` ([#4993](https://github.com/databricks/terraform-provider-databricks/pull/4993))
1516

17+
* Document `continuous.task_retry_mode` in `databricks_job` ([#4993](https://github.com/databricks/terraform-provider-databricks/pull/4993))
1618

1719
### Exporter
1820

‎scim/data_group.go‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -40,14 +40,22 @@ func DataSourceGroup() common.Resource {
4040
Schema: s,
4141
Read: func(ctx context.Context, d *schema.ResourceData, m *common.DatabricksClient) error {
4242
var this entity
43+
var group Group
44+
var err error
4345
common.DataToStructPointer(d, s, &this)
4446
groupsAPI := NewGroupsAPI(ctx, m)
45-
groupAttributes := "members,roles,entitlements,externalId"
46-
group, err := groupsAPI.ReadByDisplayName(this.DisplayName, "id")
47-
if err != nil {
48-
return err
47+
groupAttributes := "displayName,members,roles,entitlements,externalId,groups"
48+
if m.DatabricksClient.Config.IsAccountClient() {
49+
group, err = groupsAPI.ReadByDisplayName(this.DisplayName, "id")
50+
if err != nil {
51+
return err
52+
}
53+
group, err = groupsAPI.Read(group.ID, groupAttributes)
54+
} else {
55+
// For workspace level we rely on default attributes as explicit attributes, likes `externalId`
56+
// may not work for non-admin users
57+
group, err = groupsAPI.ReadByDisplayName(this.DisplayName, "")
4958
}
50-
group, err = groupsAPI.Read(group.ID, groupAttributes)
5159
if err != nil {
5260
return err
5361
}
@@ -84,7 +92,7 @@ func DataSourceGroup() common.Resource {
8492
}
8593
}
8694
this.ExternalID = group.ExternalID
87-
this.AclPrincipalID = fmt.Sprintf("groups/%s", group.DisplayName)
95+
this.AclPrincipalID = fmt.Sprintf("groups/%s", this.DisplayName)
8896
sort.Strings(this.Groups)
8997
sort.Strings(this.Members)
9098
sort.Strings(this.Users)

‎scim/data_group_test.go‎

Lines changed: 101 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -4,42 +4,118 @@ import (
44
"testing"
55

66
"github.com/databricks/terraform-provider-databricks/qa"
7-
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
8-
"github.com/stretchr/testify/assert"
9-
"github.com/stretchr/testify/require"
107
)
118

12-
func assertContains(t *testing.T, s any, e string) bool {
13-
return assert.True(t, s.(*schema.Set).Contains(e), "%#v doesn't contain %s", s, e)
14-
}
15-
169
func TestDataSourceGroup(t *testing.T) {
17-
d, err := qa.ResourceFixture{
10+
qa.ResourceFixture{
1811
Fixtures: []qa.HTTPFixture{
1912
{
2013
Method: "GET",
21-
Resource: `/api/2.0/preview/scim/v2/Groups?attributes=id&filter=displayName%20eq%20%22ds%22`,
14+
Resource: `/api/2.0/preview/scim/v2/Groups?filter=displayName%20eq%20%22ds%22`,
2215
Response: GroupList{
2316
Resources: []Group{
2417
{
2518
DisplayName: "ds",
2619
ID: "eerste",
20+
Entitlements: []ComplexValue{
21+
{
22+
Value: "allow-cluster-create",
23+
},
24+
},
25+
Members: []ComplexValue{
26+
{
27+
Ref: "Users/1112",
28+
Value: "1112",
29+
},
30+
{
31+
Ref: "ServicePrincipals/1113",
32+
Value: "1113",
33+
},
34+
{
35+
Ref: "Groups/1114",
36+
Value: "1114",
37+
},
38+
},
39+
Groups: []ComplexValue{
40+
{
41+
Value: "abc",
42+
},
43+
},
44+
Roles: []ComplexValue{
45+
{
46+
Value: "a",
47+
},
48+
},
2749
},
2850
},
2951
},
3052
},
3153
{
3254
Method: "GET",
33-
Resource: "/api/2.0/preview/scim/v2/Groups/eerste?attributes=members,roles,entitlements,externalId",
34-
55+
Resource: "/api/2.0/preview/scim/v2/Groups/abc?attributes=displayName,members,roles,entitlements,externalId,groups",
3556
Response: Group{
36-
DisplayName: "ds",
37-
ID: "eerste",
57+
DisplayName: "product",
58+
ID: "abc",
3859
Entitlements: []ComplexValue{
3960
{
40-
Value: "allow-cluster-create",
61+
Value: "allow-instance-pool-create",
62+
},
63+
},
64+
Roles: []ComplexValue{
65+
{
66+
Value: "b",
4167
},
4268
},
69+
Members: []ComplexValue{
70+
{
71+
Value: "1113",
72+
},
73+
},
74+
},
75+
},
76+
},
77+
Read: true,
78+
NonWritable: true,
79+
Resource: DataSourceGroup(),
80+
ID: ".",
81+
State: map[string]any{
82+
"display_name": "ds",
83+
},
84+
}.ApplyAndExpectData(t, map[string]any{
85+
"acl_principal_id": "groups/ds",
86+
"instance_profiles": []string{"a", "b"},
87+
"members": []string{"1112", "1113", "1114"},
88+
"groups": []string{"abc"},
89+
"allow_instance_pool_create": true,
90+
"allow_cluster_create": true,
91+
"users": []string{"1112"},
92+
"service_principals": []string{"1113"},
93+
"child_groups": []string{"1114"},
94+
"id": "eerste",
95+
})
96+
}
97+
98+
func TestDataSourceGroupAccountClient(t *testing.T) {
99+
qa.ResourceFixture{
100+
Fixtures: []qa.HTTPFixture{
101+
{
102+
Method: "GET",
103+
Resource: `/api/2.0/accounts/1234567890/scim/v2/Groups?attributes=id&filter=displayName%20eq%20%22ds%22`,
104+
Response: GroupList{
105+
Resources: []Group{
106+
{
107+
DisplayName: "ds",
108+
ID: "eerste",
109+
},
110+
},
111+
},
112+
},
113+
{
114+
Method: "GET",
115+
Resource: "/api/2.0/accounts/1234567890/scim/v2/Groups/eerste?attributes=displayName,members,roles,entitlements,externalId,groups",
116+
Response: Group{
117+
DisplayName: "ds",
118+
ID: "eerste",
43119
Members: []ComplexValue{
44120
{
45121
Ref: "Users/1112",
@@ -59,29 +135,14 @@ func TestDataSourceGroup(t *testing.T) {
59135
Value: "abc",
60136
},
61137
},
62-
Roles: []ComplexValue{
63-
{
64-
Value: "a",
65-
},
66-
},
67138
},
68139
},
69140
{
70141
Method: "GET",
71-
Resource: "/api/2.0/preview/scim/v2/Groups/abc?attributes=members,roles,entitlements,externalId",
142+
Resource: "/api/2.0/accounts/1234567890/scim/v2/Groups/abc?attributes=displayName,members,roles,entitlements,externalId,groups",
72143
Response: Group{
73144
DisplayName: "product",
74145
ID: "abc",
75-
Entitlements: []ComplexValue{
76-
{
77-
Value: "allow-instance-pool-create",
78-
},
79-
},
80-
Roles: []ComplexValue{
81-
{
82-
Value: "b",
83-
},
84-
},
85146
Members: []ComplexValue{
86147
{
87148
Value: "1113",
@@ -93,23 +154,19 @@ func TestDataSourceGroup(t *testing.T) {
93154
Read: true,
94155
NonWritable: true,
95156
Resource: DataSourceGroup(),
157+
AccountID: "1234567890",
96158
ID: ".",
97159
State: map[string]any{
98160
"display_name": "ds",
99161
},
100-
}.Apply(t)
101-
require.NoError(t, err)
102-
assert.Equal(t, "eerste", d.Id())
103-
assert.Equal(t, d.Get("acl_principal_id"), "groups/ds")
104-
assertContains(t, d.Get("instance_profiles"), "a")
105-
assertContains(t, d.Get("instance_profiles"), "b")
106-
assertContains(t, d.Get("members"), "1112")
107-
assertContains(t, d.Get("members"), "1113")
108-
assertContains(t, d.Get("groups"), "abc")
109-
assert.Equal(t, true, d.Get("allow_instance_pool_create"))
110-
assert.Equal(t, true, d.Get("allow_cluster_create"))
162+
}.ApplyAndExpectData(t, map[string]any{
163+
"acl_principal_id": "groups/ds",
164+
"members": []string{"1112", "1113", "1114"},
165+
"groups": []string{"abc"},
166+
"users": []string{"1112"},
167+
"service_principals": []string{"1113"},
168+
"child_groups": []string{"1114"},
169+
"id": "eerste",
170+
})
111171

112-
assertContains(t, d.Get("users"), "1112")
113-
assertContains(t, d.Get("service_principals"), "1113")
114-
assertContains(t, d.Get("child_groups"), "1114")
115172
}

0 commit comments

Comments
 (0)