Migrate resource databricks_catalog to Go SDK
#2357
Conversation
| // We need to update the resource data because Name is updatable | ||
| // So if we don't update the field then the requests would be made to old Name which doesn't exists. | ||
| d.SetId(ci.Name) | ||
| if err != nil { |
There was a problem hiding this comment.
return error after setting the ID, otherwise we're tainted
There was a problem hiding this comment.
changed the code to only set the new Id after a successful update
There was a problem hiding this comment.
Nice catch. This would need to be done for volumes as well. Created: #2374
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2357 +/- ##
==========================================
- Coverage 88.53% 88.47% -0.07%
==========================================
Files 141 141
Lines 11620 11628 +8
==========================================
Hits 10288 10288
- Misses 886 890 +4
- Partials 446 450 +4
|
| } | ||
|
|
||
| return a.client.Delete(a.context, "/unity-catalog/catalogs/"+name, nil) | ||
| Name string `json:"name"` |
There was a problem hiding this comment.
isn't the tf:"force_new" missing here?
There was a problem hiding this comment.
catalog name is updatable through our API but we force recreating the resource because the provider was not able to handle the change in name. Same explanation as the below comment from Alex
| var updateCatalogRequest catalog.UpdateCatalog | ||
| common.DataToStructPointer(d, catalogSchema, &updateCatalogRequest) | ||
| ci, err := w.Catalogs.Update(ctx, updateCatalogRequest) |
There was a problem hiding this comment.
How it will work when the name is already updated in resource - as I see in the docs, we address catalog by name: https://docs.databricks.com/api/workspace/catalogs/update, so if the name in resource is updated, it won't find an old one to change it.
There was a problem hiding this comment.
if the change happened out of bound, then there is not much we can do. what happens with the update operation here is that we call the Update API, update the resource Id to the new name, so that the Get API will use the new name and succeed
There was a problem hiding this comment.
credit to @tanmay-db for getting this to worked with the Volume resource, so I am just backporting the fix
There was a problem hiding this comment.
If we allow name change, then SDK needs to support that pattern. Inside provider we can find if the name field has changed, and then use previous name (the d.Id()) in the URL
There was a problem hiding this comment.
we need to do the same for all the UC resources then, as they are all addressed by names.
currently volume handles it like this https://github.com/databricks/terraform-provider-databricks/blob/master/catalog/resource_volume.go#L85
| func NewCatalogsAPI(ctx context.Context, m any) CatalogsAPI { | ||
| return CatalogsAPI{m.(*common.DatabricksClient), context.WithValue(ctx, common.Api, common.API_2_1)} | ||
| } | ||
|
|
||
| type CatalogInfo struct { |
There was a problem hiding this comment.
@nkvuong did you try using UpdateCatalog for the schema this way we won't have to worry about the new fields?...
| Comment string `json:"comment,omitempty"` | ||
| StorageRoot string `json:"storage_root,omitempty" tf:"force_new"` | ||
| ProviderName string `json:"provider_name,omitempty" tf:"force_new,conflicts:storage_root"` | ||
| ShareName string `json:"share_name,omitempty" tf:"force_new,conflicts:storage_root"` |
There was a problem hiding this comment.
this can go into schema callback.
Changes
Migrate
databricks_catalogresource to Go SDK.Tests
make testrun locallydocs/folderinternal/acceptance