Introduce Generic Settings Resource

[mgyucht]

Changes

This is an experimental PR for a generic settings resource in the TF provider. Because all Settings API settings have a consistent schema & mechanism for interaction, the definition of a setting resource in Terraform can also be made generic with several simple assumptions about each setting:

1. Each setting needs a name (in this case, the name would be "default_namespace", so the resource name would be "databricks_default_namespace_setting") and a field mask used to update the value for that setting.
2. Each setting needs a structure T containing an Etag field.
3. Each setting has an API to read the setting by Etag.
4. Each setting has an API to delete the setting by Etag.
5. Each setting has a single API to update the setting. The update request must have an AllowMissing boolean field, a Setting field of type *T, and a field mask.

These above requirements are encapsulated in the genericSettingDefinition interface. Because API calls require a specific client depending on whether the setting is an account-level or workspace-level setting, we define two interfaces, one for workspace-level settings where the workspace client is provided, and one for account-level settings where the account client is provided.

After this change, to add a new setting:

1. Create a new instance of either the workspaceSetting or accountSetting for your setting. The type parameter should be your setting resource.

var defaultNamespaceSetting = workspaceSetting[settings.DefaultNamespaceSetting]{
	settingStruct: settings.DefaultNamespaceSetting{},
	readFunc: func(ctx context.Context, w *databricks.WorkspaceClient, etag string) (*settings.DefaultNamespaceSetting, error) {
		return w.Settings.ReadDefaultWorkspaceNamespace(ctx, settings.ReadDefaultWorkspaceNamespaceRequest{
			Etag: etag,
		})
	},
	updateFunc: func(ctx context.Context, w *databricks.WorkspaceClient, t settings.DefaultNamespaceSetting) (string, error) {
		t.SettingName = "default"
		res, err := w.Settings.UpdateDefaultWorkspaceNamespace(ctx, settings.UpdateDefaultWorkspaceNamespaceRequest{
			AllowMissing: true,
			Setting:      &t,
			FieldMask:    "namespace.value",
		})
		if err != nil {
			return "", err
		}
		return res.Etag, err
	},
	deleteFunc: func(ctx context.Context, w *databricks.WorkspaceClient, etag string) (string, error) {
		res, err := w.Settings.DeleteDefaultWorkspaceNamespace(ctx, settings.DeleteDefaultWorkspaceNamespaceRequest{
			Etag: etag,
		})
		if err != nil {
			return "", err
		}
		return res.Etag, err
	},
}

2. Add a new constant for the name of the setting and to the AllSettingsResources() function:

func AllSettingsResources() map[string]*schema.Resource {
  return {
    ...
    "my_new_setting": makeSettingResource[settings.MyNewSetting, *databricks.WorkspaceClient](myNewSetting),
  }
}

Additionally, I migrated the tests for this resource to use the Go SDK.

Tests

• Unit tests verify that the generic setting
• Process for change is documented in this PR description as well as in all_settings.go.
• covered with integration tests in internal/acceptance
• relevant acceptance tests are passing
• using Go SDK

[codecov-commenter]

Codecov Report

Attention: 53 lines in your changes are missing coverage. Please review.

Comparison is base (fbb6552) 83.82% compared to head (798736f) 83.64%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2997      +/-   ##
==========================================
- Coverage   83.82%   83.64%   -0.19%     
==========================================
  Files         165      167       +2     
  Lines       14590    14636      +46     
==========================================
+ Hits        12230    12242      +12     
- Misses       1649     1682      +33     
- Partials      711      712       +1     

Files	Coverage Δ
provider/provider.go	94.44% <100.00%> (ø)
settings/all_settings.go	100.00% <100.00%> (ø)
settings/resource_default_namespace_setting.go	100.00% <100.00%> (+19.58%)	⬆️
settings/generic_setting.go	55.83% <55.83%> (ø)

[alexott]

If the schema is consistent, I'm wondering if we need to have individual resources, because we may have dozens of settings and this may lead to the proliferation of resources and resource instances that may significantly slow down Terraform in the big installations.

Why not define two resources - one for workspace settings, and another - for account? Both will support patch method, not put, so it will be possible to define multiple instances if necessary.

[deryasusm-db]

This should be tested by running the TF integration test for Default Namespace against a test shard

[deryasusm-db]

The common resource definition and the common methods should move out of this file

[deryasusm-db]

Create a struct corresponding to your setting,

Are you referring to settings.DefaultNamespaceSetting as this struct to be created or type defaultNamespaceSetting struct{}?

[mgyucht]

The latter, I can clarify that.

[mgyucht]

@alexott We could also do that, however, 1) it probably has less to do with Terraform than it does with the API itself, as there are still many API calls that need to be made to retrieve all values of all settings. There definitely will be TF overhead, but I suspect it would be comparable or maybe even smaller than the cost of making requests to the backend. 2) This keeps TF as close as possible to the REST API, which is generally a design goal. I think for consistency & as a general principle we should keep these as close as possible to one another.

[deryasusm-db]

@hectorcast-db Can you review this one as you did the TF integration for Default Namespace setting? Thanks.

[hectorcast-db]

Can read fail on a EtagError?
From the docs:

The server will accept any etag and respond with a response which is at least as recent as the etag.

[mgyucht]

Good point, removed need to retry in reads.

[alexott]

Do we really need to retry on the NotFound error? If it's not found then this typically it means that it was removed by external agent, so we just need to record deletion...

[mgyucht]

Good point. Removed retry on Not Found, so we only retry on conflict.