-
Notifications
You must be signed in to change notification settings - Fork 478
[Internal] Unified Provider support for UC resources #5149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ricks into go-sdk-unified-1
…ricks into go-sdk-unified-1
catalog/resource_catalog.go
Outdated
| d.SetId(ci.Name) | ||
|
|
||
| // Bind the current workspace if the catalog is isolated, otherwise the read will fail | ||
| return bindings.AddCurrentWorkspaceBindings(ctx, d, w, ci.Name, bindings.BindingsSecurableTypeCatalog) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably needs to be moved up to the top of Update(), otherwise the WorkspaceClient used when changing provider_config.workspace_id may not have access to the isolated catalog (unless that workspace also happens to have a workspace binding separately).
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
alexott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general should be ok, but we may need to discuss how to handle bindings
| updateCatalogRequest.Name = d.Id() | ||
|
|
||
| // Bind the current workspace if the catalog is isolated, otherwise the read will fail | ||
| err = bindings.AddCurrentWorkspaceBindings(ctx, d, w, updateCatalogRequest.Name, bindings.BindingsSecurableTypeCatalog) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, typically the binding is done on creation. Should we add a check if the isolation mode is changing?
| // Bind the current workspace if the catalog is isolated, otherwise the read will fail | ||
| err = bindings.AddCurrentWorkspaceBindings(ctx, d, w, updateCatalogRequest.Name, bindings.BindingsSecurableTypeCatalog) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to handle error like this: https://github.com/databricks/terraform-provider-databricks/pull/5149/files#diff-2dee1865e5fcab955309d55246c5d88e1692d4ea8008c9d5c133505fa6877130R187
Changes
Unified Provider Support for the following resources:
Note: UC Resources will not have
NamespaceCustomizeDiffwhich doesd.ForceNewTests
Added in main branch
NO_CHANGELOG=true