Validate metastore_id field in worspace UC resources #3035
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3035 +/- ##
==========================================
- Coverage 84.32% 84.16% -0.16%
==========================================
Files 158 159 +1
Lines 13833 13897 +64
==========================================
+ Hits 11664 11696 +32
- Misses 1517 1533 +16
- Partials 652 668 +16
|
| @@ -56,7 +56,7 @@ func TestResourceCredentialsCreate(t *testing.T) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestAccountIdOnlyInState(t *testing.T) { | |||
| func TestCredentialsAccountIdOnlyInState(t *testing.T) { | |||
There was a problem hiding this comment.
Unrelated, but since the name starts with TestAcc it runs on the integration tests an fails due to the fixture.
docs/resources/catalog.md
Outdated
| @@ -25,6 +25,7 @@ resource "databricks_catalog" "sandbox" { | |||
| The following arguments are required: | |||
|
|
|||
| * `name` - Name of Catalog relative to parent metastore. | |||
| * `metastore_id` - (Required if changing the metastore assigned to a workspace) If set, it must match the ID of the metastore assigned to the worspace. If not set, the current ID is exported. | |||
There was a problem hiding this comment.
the statement (Required if changing the metastore assigned to a workspace) will likely cause confusion - let's just mention it in the troubleshooting guide
There was a problem hiding this comment.
+1. IMO we don't need to show this to customers explicitly; the happy path would be not to specify the metastore ID, I think. Once these resources are available at the account-level, we'll change the docs to match what you've done for storage credentials.
There was a problem hiding this comment.
we should also remove metastore_id = databricks_metastore.this.id from the example usage (and any other docs)
catalog/resource_catalog_test.go
Outdated
| comment = "c" | ||
| owner = "administrators" | ||
| `, | ||
| }.ApplyNoError(t) |
There was a problem hiding this comment.
For sanity, let's make sure the metastore ID is updated properly?
| "github.com/databricks/databricks-sdk-go" | ||
| ) | ||
|
|
||
| func validateMetastoreId(ctx context.Context, w *databricks.WorkspaceClient, metastoreId string) error { |
There was a problem hiding this comment.
nit: add a small doccomment here explaining the purpose of this and where it should be added (e.g. all create/update/delete methods for UC resources that accept a metastore_id parameter).
docs/resources/catalog.md
Outdated
| @@ -25,6 +25,7 @@ resource "databricks_catalog" "sandbox" { | |||
| The following arguments are required: | |||
|
|
|||
| * `name` - Name of Catalog relative to parent metastore. | |||
| * `metastore_id` - (Required if changing the metastore assigned to a workspace) If set, it must match the ID of the metastore assigned to the worspace. If not set, the current ID is exported. | |||
There was a problem hiding this comment.
+1. IMO we don't need to show this to customers explicitly; the happy path would be not to specify the metastore ID, I think. Once these resources are available at the account-level, we'll change the docs to match what you've done for storage credentials.
Changes
Validate metastore_id field in worspace UC resources
Tests
make testrun locallydocs/folderinternal/acceptance