Skip to content

Commit 689cc85

Browse files
alexottalexott
authored
Use stable IDs for settings resources (v2) (#3314)
* Use stable IDs for settings resources Before this change, generated settings resource were using `etag` value as resource ID. This lead to a situation that importing of these resources was problematic because ID was changing all the time. This PR introduces stable IDs for generated resources. By default it uses a constant ID: `global`, similar to other resources, like, `databricks_sql_global_config`, but the resource implementor may provide a function that will generate a resource ID from the settings' data. This fixes #3270 * address review comments * Fix tests for `databricks_restrict_workspace_admins_setting` * Fix integration tests * Review feedback
1 parent d3a6204 commit 689cc85

File tree

7 files changed

+229
-149
lines changed

7 files changed

+229
-149
lines changed

‎docs/resources/default_namespace_settings.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,3 +29,11 @@ The resource supports the following arguments:
2929

3030
* `namespace` - (Required) The configuration details.
3131
* `value` - (Required) The value for the setting.
32+
33+
## Import
34+
35+
This resource can be imported by predefined name `global`:
36+
37+
```bash
38+
terraform import databricks_default_namespace_setting.this global
39+
```

‎docs/resources/restrict_workspace_admins_setting.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,3 +38,11 @@ The resource supports the following arguments:
3838

3939
* `restrict_workspace_admins` - (Required) The configuration details.
4040
* `status` - (Required) The restrict workspace admins status for the workspace.
41+
42+
## Import
43+
44+
This resource can be imported by predefined name `global`:
45+
46+
```bash
47+
terraform import databricks_restrict_workspace_admins_setting.this global
48+
```

‎internal/acceptance/default_namespace_test.go‎

Lines changed: 28 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -8,42 +8,44 @@ import (
88
"github.com/databricks/databricks-sdk-go/apierr"
99
"github.com/databricks/databricks-sdk-go/service/settings"
1010
"github.com/databricks/terraform-provider-databricks/common"
11+
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
1112
"github.com/stretchr/testify/assert"
13+
"github.com/stretchr/testify/require"
1214
)
1315

1416
func TestAccDefaultNamespaceSetting(t *testing.T) {
15-
workspaceLevel(t, step{
16-
Template: `
17-
resource "databricks_default_namespace_setting" "this" {
18-
namespace {
19-
value = "namespace_value"
20-
}
17+
template := `
18+
resource "databricks_default_namespace_setting" "this" {
19+
namespace {
20+
value = "namespace_value"
2121
}
22-
`,
23-
Check: resourceCheck("databricks_default_namespace_setting.this", func(ctx context.Context, client *common.DatabricksClient, id string) error {
24-
ctx = context.WithValue(ctx, common.Api, common.API_2_1)
25-
w, err := client.WorkspaceClient()
26-
assert.NoError(t, err)
27-
res, err := w.Settings.GetDefaultNamespaceSetting(ctx, settings.GetDefaultNamespaceSettingRequest{
28-
Etag: id,
29-
})
30-
assert.NoError(t, err)
31-
// Check that the resource has been created and that it has the correct value.
32-
assert.Equal(t, res.Namespace.Value, "namespace_value")
33-
return nil
34-
}),
22+
}
23+
`
24+
workspaceLevel(t, step{
25+
Template: template,
26+
Check: resourceCheckWithState("databricks_default_namespace_setting.this",
27+
func(ctx context.Context, client *common.DatabricksClient, state *terraform.InstanceState) error {
28+
ctx = context.WithValue(ctx, common.Api, common.API_2_1)
29+
w, err := client.WorkspaceClient()
30+
require.NoError(t, err)
31+
etag := state.Attributes["etag"]
32+
require.NotEmpty(t, etag)
33+
res, err := w.Settings.GetDefaultNamespaceSetting(ctx, settings.GetDefaultNamespaceSettingRequest{
34+
Etag: etag,
35+
})
36+
require.NoError(t, err)
37+
// Check that the resource has been created and that it has the correct value.
38+
assert.Equal(t, res.Namespace.Value, "namespace_value")
39+
return nil
40+
}),
3541
},
3642
step{
37-
Template: `resource "databricks_default_namespace_setting" "this" {
38-
namespace {
39-
value = "namespace_value"
40-
}
41-
}`,
42-
Destroy: true,
43+
Template: template,
44+
Destroy: true,
4345
Check: resourceCheck("databricks_default_namespace_setting.this", func(ctx context.Context, client *common.DatabricksClient, id string) error {
4446
ctx = context.WithValue(ctx, common.Api, common.API_2_1)
4547
w, err := client.WorkspaceClient()
46-
assert.NoError(t, err)
48+
require.NoError(t, err)
4749
// Terraform Check returns the latest resource status before it is destroyed, which has an outdated eTag.
4850
// We are making an update call to get the correct eTag in the response error.
4951
_, err = w.Settings.UpdateDefaultNamespaceSetting(ctx, settings.UpdateDefaultNamespaceSettingRequest{

‎internal/acceptance/restrict_workspace_admins_test.go‎

Lines changed: 52 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -8,68 +8,71 @@ import (
88
"github.com/databricks/databricks-sdk-go/apierr"
99
"github.com/databricks/databricks-sdk-go/service/settings"
1010
"github.com/databricks/terraform-provider-databricks/common"
11+
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
1112
"github.com/stretchr/testify/assert"
13+
"github.com/stretchr/testify/require"
1214
)
1315

1416
func TestAccRestrictWorkspaceAdminsSetting(t *testing.T) {
15-
workspaceLevel(t, step{
16-
Template: `
17-
resource "databricks_restrict_workspace_admins_setting" "this" {
18-
restrict_workspace_admins {
19-
status = "RESTRICT_TOKENS_AND_JOB_RUN_AS"
20-
}
17+
template := `
18+
resource "databricks_restrict_workspace_admins_setting" "this" {
19+
restrict_workspace_admins {
20+
status = "RESTRICT_TOKENS_AND_JOB_RUN_AS"
2121
}
22-
`,
23-
Check: resourceCheck("databricks_restrict_workspace_admins_setting.this", func(ctx context.Context, client *common.DatabricksClient, id string) error {
24-
ctx = context.WithValue(ctx, common.Api, common.API_2_1)
25-
w, err := client.WorkspaceClient()
26-
assert.NoError(t, err)
27-
res, err := w.Settings.GetRestrictWorkspaceAdminsSetting(ctx, settings.GetRestrictWorkspaceAdminsSettingRequest{
28-
Etag: id,
29-
})
30-
assert.NoError(t, err)
31-
// Check that the resource has been created and that it has the correct value.
32-
assert.Equal(t, res.RestrictWorkspaceAdmins.Status.String(), "RESTRICT_TOKENS_AND_JOB_RUN_AS")
33-
return nil
34-
}),
35-
},
36-
step{
37-
Template: `resource "databricks_restrict_workspace_admins_setting" "this" {
38-
restrict_workspace_admins {
39-
status = "RESTRICT_TOKENS_AND_JOB_RUN_AS"
40-
}
41-
}`,
42-
Destroy: true,
43-
Check: resourceCheck("databricks_restrict_workspace_admins_setting.this", func(ctx context.Context, client *common.DatabricksClient, id string) error {
22+
}
23+
`
24+
workspaceLevel(t, step{
25+
Template: template,
26+
Check: resourceCheckWithState("databricks_restrict_workspace_admins_setting.this",
27+
func(ctx context.Context, client *common.DatabricksClient, state *terraform.InstanceState) error {
4428
ctx = context.WithValue(ctx, common.Api, common.API_2_1)
4529
w, err := client.WorkspaceClient()
46-
assert.NoError(t, err)
47-
// Terraform Check returns the latest resource status before it is destroyed, which has an outdated eTag.
48-
// We are making an update call to get the correct eTag in the response error.
49-
_, err = w.Settings.UpdateRestrictWorkspaceAdminsSetting(ctx, settings.UpdateRestrictWorkspaceAdminsSettingRequest{
50-
AllowMissing: true,
51-
Setting: settings.RestrictWorkspaceAdminsSetting{
52-
RestrictWorkspaceAdmins: settings.RestrictWorkspaceAdminsMessage{
53-
Status: "RESTRICT_TOKENS_AND_JOB_RUN_AS",
54-
},
55-
},
56-
FieldMask: "restrict_workspace_admins.status",
57-
})
58-
assert.Error(t, err)
59-
var aerr *apierr.APIError
60-
if !errors.As(err, &aerr) {
61-
assert.FailNow(t, "cannot parse error message %v", err)
62-
}
63-
etag := aerr.Details[0].Metadata["etag"]
30+
require.NoError(t, err)
31+
etag := state.Attributes["etag"]
32+
require.NotEmpty(t, etag)
6433
res, err := w.Settings.GetRestrictWorkspaceAdminsSetting(ctx, settings.GetRestrictWorkspaceAdminsSettingRequest{
6534
Etag: etag,
6635
})
67-
// we should not be getting any error
6836
assert.NoError(t, err)
69-
// workspace should go back to default
70-
assert.Equal(t, res.RestrictWorkspaceAdmins.Status.String(), "ALLOW_ALL")
37+
// Check that the resource has been created and that it has the correct value.
38+
assert.Equal(t, "RESTRICT_TOKENS_AND_JOB_RUN_AS", res.RestrictWorkspaceAdmins.Status.String())
7139
return nil
7240
}),
41+
},
42+
step{
43+
Template: template,
44+
Destroy: true,
45+
Check: resourceCheck("databricks_restrict_workspace_admins_setting.this",
46+
func(ctx context.Context, client *common.DatabricksClient, id string) error {
47+
ctx = context.WithValue(ctx, common.Api, common.API_2_1)
48+
w, err := client.WorkspaceClient()
49+
assert.NoError(t, err)
50+
// Terraform Check returns the latest resource status before it is destroyed, which has an outdated eTag.
51+
// We are making an update call to get the correct eTag in the response error.
52+
_, err = w.Settings.UpdateRestrictWorkspaceAdminsSetting(ctx, settings.UpdateRestrictWorkspaceAdminsSettingRequest{
53+
AllowMissing: true,
54+
Setting: settings.RestrictWorkspaceAdminsSetting{
55+
RestrictWorkspaceAdmins: settings.RestrictWorkspaceAdminsMessage{
56+
Status: "RESTRICT_TOKENS_AND_JOB_RUN_AS",
57+
},
58+
},
59+
FieldMask: "restrict_workspace_admins.status",
60+
})
61+
assert.Error(t, err)
62+
var aerr *apierr.APIError
63+
if !errors.As(err, &aerr) {
64+
assert.FailNow(t, "cannot parse error message %v", err)
65+
}
66+
etag := aerr.Details[0].Metadata["etag"]
67+
res, err := w.Settings.GetRestrictWorkspaceAdminsSetting(ctx, settings.GetRestrictWorkspaceAdminsSettingRequest{
68+
Etag: etag,
69+
})
70+
// we should not be getting any error
71+
assert.NoError(t, err)
72+
// workspace should go back to default
73+
assert.Equal(t, res.RestrictWorkspaceAdmins.Status.String(), "ALLOW_ALL")
74+
return nil
75+
}),
7376
},
7477
)
7578
}

‎settings/generic_setting.go‎

Lines changed: 42 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,11 @@ import (
1212
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
1313
)
1414

15+
var (
16+
defaultSettingId = "global"
17+
etagAttrName = "etag"
18+
)
19+
1520
func retryOnEtagError[Req, Resp any](f func(req Req) (Resp, error), firstReq Req, updateReq func(req *Req, newEtag string), retriableErrors []error) (Resp, error) {
1621
req := firstReq
1722
// Retry once on etag error.
@@ -43,7 +48,7 @@ func getEtagFromError(err error) (string, error) {
4348
errorInfos := apierr.GetErrorInfo(err)
4449
if len(errorInfos) > 0 {
4550
metadata := errorInfos[0].Metadata
46-
if etag, ok := metadata["etag"]; ok {
51+
if etag, ok := metadata[etagAttrName]; ok {
4752
return etag, nil
4853
}
4954
}
@@ -68,6 +73,9 @@ type genericSettingDefinition[T, U any] interface {
6873

6974
// Update the etag in the setting.
7075
SetETag(t *T, newEtag string)
76+
77+
// Generate resource ID from settings instance
78+
GetId(t *T) string
7179
}
7280

7381
func getEtag[T any](t T) string {
@@ -104,6 +112,9 @@ type workspaceSetting[T any] struct {
104112

105113
// Delete the setting with the given etag, and return the new etag.
106114
deleteFunc func(ctx context.Context, w *databricks.WorkspaceClient, etag string) (string, error)
115+
116+
// Optional function to generate resource ID from the settings. If not provided, will use predefined value `global`
117+
generateIdFunc func(setting *T) string
107118
}
108119

109120
func (w workspaceSetting[T]) SettingStruct() T {
@@ -121,6 +132,15 @@ func (w workspaceSetting[T]) Delete(ctx context.Context, c *databricks.Workspace
121132
func (w workspaceSetting[T]) GetETag(t *T) string {
122133
return getEtag(t)
123134
}
135+
136+
func (w workspaceSetting[T]) GetId(t *T) string {
137+
id := defaultSettingId
138+
if w.generateIdFunc != nil {
139+
id = w.generateIdFunc(t)
140+
}
141+
return id
142+
}
143+
124144
func (w workspaceSetting[T]) SetETag(t *T, newEtag string) {
125145
setEtag(t, newEtag)
126146
}
@@ -145,6 +165,9 @@ type accountSetting[T any] struct {
145165

146166
// Delete the setting with the given etag, and return the new etag.
147167
deleteFunc func(ctx context.Context, w *databricks.AccountClient, etag string) (string, error)
168+
169+
// Optional function to generate resource ID from the settings. If not provided, will use predefined value `global`
170+
generateIdFunc func(setting *T) string
148171
}
149172

150173
func (w accountSetting[T]) SettingStruct() T {
@@ -166,12 +189,20 @@ func (w accountSetting[T]) SetETag(t *T, newEtag string) {
166189
setEtag(t, newEtag)
167190
}
168191

192+
func (w accountSetting[T]) GetId(t *T) string {
193+
id := defaultSettingId
194+
if w.generateIdFunc != nil {
195+
id = w.generateIdFunc(t)
196+
}
197+
return id
198+
}
199+
169200
var _ accountSettingDefinition[struct{}] = accountSetting[struct{}]{}
170201

171202
func makeSettingResource[T, U any](defn genericSettingDefinition[T, U]) common.Resource {
172203
resourceSchema := common.StructToSchema(defn.SettingStruct(),
173204
func(s map[string]*schema.Schema) map[string]*schema.Schema {
174-
s["etag"].Computed = true
205+
s[etagAttrName].Computed = true
175206
// Note: this may not always be computed, but it is for the default namespace setting. If other settings
176207
// are added for which setting_name is not computed, we'll need to expose this somehow as part of the setting
177208
// definition.
@@ -217,7 +248,8 @@ func makeSettingResource[T, U any](defn genericSettingDefinition[T, U]) common.R
217248
default:
218249
return fmt.Errorf("unexpected setting type: %T", defn)
219250
}
220-
d.SetId(res)
251+
d.Set(etagAttrName, res)
252+
d.SetId(defn.GetId(&setting))
221253
return nil
222254
}
223255

@@ -235,7 +267,7 @@ func makeSettingResource[T, U any](defn genericSettingDefinition[T, U]) common.R
235267
if err != nil {
236268
return err
237269
}
238-
res, err = defn.Read(ctx, w, d.Id())
270+
res, err = defn.Read(ctx, w, d.Get(etagAttrName).(string))
239271
if err != nil {
240272
return err
241273
}
@@ -244,7 +276,7 @@ func makeSettingResource[T, U any](defn genericSettingDefinition[T, U]) common.R
244276
if err != nil {
245277
return err
246278
}
247-
res, err = defn.Read(ctx, a, d.Id())
279+
res, err = defn.Read(ctx, a, d.Get(etagAttrName).(string))
248280
if err != nil {
249281
return err
250282
}
@@ -259,12 +291,12 @@ func makeSettingResource[T, U any](defn genericSettingDefinition[T, U]) common.R
259291
// with a response which is at least as recent as the etag.
260292
// Updating, while not always necessary, ensures that the
261293
// server responds with an updated response.
262-
d.SetId(defn.GetETag(res))
294+
d.Set(etagAttrName, defn.GetETag(res))
263295
return nil
264296
},
265297
Update: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
266298
var setting T
267-
defn.SetETag(&setting, d.Id())
299+
defn.SetETag(&setting, d.Get(etagAttrName).(string))
268300
return createOrUpdate(ctx, d, c, setting)
269301
},
270302
Delete: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
@@ -280,7 +312,7 @@ func makeSettingResource[T, U any](defn genericSettingDefinition[T, U]) common.R
280312
func(etag string) (string, error) {
281313
return defn.Delete(ctx, w, etag)
282314
},
283-
d.Id(),
315+
d.Get(etagAttrName).(string),
284316
updateETag,
285317
deleteRetriableErrors)
286318
if err != nil {
@@ -295,7 +327,7 @@ func makeSettingResource[T, U any](defn genericSettingDefinition[T, U]) common.R
295327
func(etag string) (string, error) {
296328
return defn.Delete(ctx, a, etag)
297329
},
298-
d.Id(),
330+
d.Get(etagAttrName).(string),
299331
updateETag,
300332
deleteRetriableErrors)
301333
if err != nil {
@@ -304,7 +336,7 @@ func makeSettingResource[T, U any](defn genericSettingDefinition[T, U]) common.R
304336
default:
305337
return fmt.Errorf("unexpected setting type: %T", defn)
306338
}
307-
d.SetId(etag)
339+
d.Set(etagAttrName, etag)
308340
return nil
309341
},
310342
}

0 commit comments

Comments
 (0)