Skip to content

Commit

Permalink
[Fix] Correct handling of updates for empty comments and `force_destr…
Browse files Browse the repository at this point in the history 
…oy` in UC catalog, schema, registered models and volumes (#4244)

## Changes
<!-- Summary of your changes that are easy to understand -->

Force sending of fields when `comment` is changing to empty value. Also,
don't call Update API when the only change is in `force_destroy`
attribute of schemas and catalogs

Resolves #4045

## Tests
<!-- 
How is this tested? Please see the checklist below and also describe any
other relevant tests
-->

- [x] `make test` run locally
- [ ] relevant change in `docs/` folder
- [ ] covered with integration tests in `internal/acceptance`
- [ ] relevant acceptance tests are passing
- [ ] using Go SDK
  • Loading branch information
@alexott
alexott authored Nov 19, 2024
1 parent cfd396c commit 03a2515
Show file tree
Hide file tree
Showing 8 changed files with 253 additions and 0 deletions.
7 changes: 7 additions & 0 deletions catalog/resource_catalog.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ func ResourceCatalog() common.Resource {
if err != nil {
return err
}
if !d.HasChangeExcept("force_destroy") {
return nil
}

var updateCatalogRequest catalog.UpdateCatalog
common.DataToStructPointer(d, catalogSchema, &updateCatalogRequest)
Expand All @@ -143,6 +146,10 @@ func ResourceCatalog() common.Resource {
return nil
}

if d.HasChange("comment") && updateCatalogRequest.Comment == "" {
updateCatalogRequest.ForceSendFields = append(updateCatalogRequest.ForceSendFields, "Comment")
}

updateCatalogRequest.Owner = ""
ci, err := w.Catalogs.Update(ctx, updateCatalogRequest)

Expand Down
61 changes: 61 additions & 0 deletions catalog/resource_catalog_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,67 @@ func TestUpdateCatalog(t *testing.T) {
}.ApplyNoError(t)
}

func TestUpdateCatalogSetEmptyComment(t *testing.T) {
qa.ResourceFixture{
MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) {
w.GetMockMetastoresAPI().EXPECT().Current(mock.Anything).Return(&catalog.MetastoreAssignment{
MetastoreId: "d",
}, nil)
e := w.GetMockCatalogsAPI().EXPECT()
e.Update(mock.Anything, catalog.UpdateCatalog{
Name: "a",
Comment: "",
ForceSendFields: []string{"Comment"},
}).Return(&catalog.CatalogInfo{
Name: "a",
Comment: "",
}, nil)
e.GetByName(mock.Anything, "a").Return(&catalog.CatalogInfo{
Name: "a",
Comment: "",
}, nil)
},
Resource: ResourceCatalog(),
Update: true,
ID: "a",
InstanceState: map[string]string{
"metastore_id": "d",
"name": "a",
"comment": "c",
},
HCL: `
name = "a"
comment = ""
`,
}.ApplyNoError(t)
}

func TestUpdateCatalogForceDestroyOnly(t *testing.T) {
qa.ResourceFixture{
MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) {
w.GetMockMetastoresAPI().EXPECT().Current(mock.Anything).Return(&catalog.MetastoreAssignment{
MetastoreId: "d",
}, nil)
e := w.GetMockCatalogsAPI().EXPECT()
e.GetByName(mock.Anything, "a").Return(&catalog.CatalogInfo{
Name: "a",
}, nil)
},
Resource: ResourceCatalog(),
Update: true,
ID: "a",
InstanceState: map[string]string{
"metastore_id": "d",
"name": "a",
"force_destroy": "true",
},
HCL: `
name = "a"
force_destroy = false
`,
}.ApplyNoError(t)
}

func TestUpdateCatalogOwnerOnly(t *testing.T) {
qa.ResourceFixture{
MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) {
Expand Down
3 changes: 3 additions & 0 deletions catalog/resource_registered_model.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ func ResourceRegisteredModel() common.Resource {
return nil
}

if d.HasChange("comment") && u.Comment == "" {
u.ForceSendFields = append(u.ForceSendFields, "Comment")
}
u.Owner = ""
_, err = w.RegisteredModels.Update(ctx, u)
if err != nil {
Expand Down
41 changes: 41 additions & 0 deletions catalog/resource_registered_model_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,47 @@ func TestRegisteredModelUpdate(t *testing.T) {
}.ApplyNoError(t)
}

func TestRegisteredModelUpdateCommentOnly(t *testing.T) {
qa.ResourceFixture{
MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) {
e := w.GetMockRegisteredModelsAPI().EXPECT()
e.Update(mock.Anything, catalog.UpdateRegisteredModelRequest{
FullName: "catalog.schema.model",
Comment: "",
ForceSendFields: []string{"Comment"},
}).Return(&catalog.RegisteredModelInfo{
Name: "model",
CatalogName: "catalog",
SchemaName: "schema",
FullName: "catalog.schema.model",
Comment: "",
}, nil)
e.GetByFullName(mock.Anything, "catalog.schema.model").Return(&catalog.RegisteredModelInfo{
Name: "model",
CatalogName: "catalog",
SchemaName: "schema",
FullName: "catalog.schema.model",
Comment: "",
}, nil)
},
Resource: ResourceRegisteredModel(),
Update: true,
ID: "catalog.schema.model",
InstanceState: map[string]string{
"name": "model",
"catalog_name": "catalog",
"schema_name": "schema",
"comment": "comment",
},
HCL: `
name = "model"
catalog_name = "catalog"
schema_name = "schema"
comment = ""
`,
}.ApplyNoError(t)
}

func TestRegisteredModelUpdateOwner(t *testing.T) {
qa.ResourceFixture{
MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) {
Expand Down
7 changes: 7 additions & 0 deletions catalog/resource_schema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ func ResourceSchema() common.Resource {
if err != nil {
return err
}
if !d.HasChangeExcept("force_destroy") {
return nil
}
var updateSchemaRequest catalog.UpdateSchema
common.DataToStructPointer(d, s, &updateSchemaRequest)
updateSchemaRequest.FullName = d.Id()
Expand All @@ -118,6 +121,10 @@ func ResourceSchema() common.Resource {
return nil
}

if d.HasChange("comment") && updateSchemaRequest.Comment == "" {
updateSchemaRequest.ForceSendFields = append(updateSchemaRequest.ForceSendFields, "Comment")
}

updateSchemaRequest.Owner = ""
schema, err := w.Schemas.Update(ctx, updateSchemaRequest)
if err != nil {
Expand Down
71 changes: 71 additions & 0 deletions catalog/resource_schema_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,77 @@ func TestUpdateSchema(t *testing.T) {
}.ApplyNoError(t)
}

func TestUpdateSchemaSetEmptyComment(t *testing.T) {
qa.ResourceFixture{
MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) {
w.GetMockMetastoresAPI().EXPECT().Current(mock.Anything).Return(&catalog.MetastoreAssignment{
MetastoreId: "d",
}, nil)
e := w.GetMockSchemasAPI().EXPECT()
e.Update(mock.Anything, catalog.UpdateSchema{
FullName: "b.a",
Comment: "",
ForceSendFields: []string{"Comment"},
}).Return(&catalog.SchemaInfo{
FullName: "b.a",
Owner: "administrators",
}, nil)
e.GetByFullName(mock.Anything, "b.a").Return(&catalog.SchemaInfo{
Name: "a",
CatalogName: "b",
MetastoreId: "d",
Owner: "administrators",
}, nil)
},
Resource: ResourceSchema(),
Update: true,
ID: "b.a",
InstanceState: map[string]string{
"metastore_id": "d",
"name": "a",
"catalog_name": "b",
"comment": "c",
},
HCL: `
name = "a"
catalog_name = "b"
`,
}.ApplyAndExpectData(t, map[string]any{
"comment": "",
})
}

func TestUpdateSchemaChangeForceDestroy(t *testing.T) {
qa.ResourceFixture{
MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) {
w.GetMockMetastoresAPI().EXPECT().Current(mock.Anything).Return(&catalog.MetastoreAssignment{
MetastoreId: "d",
}, nil)
e := w.GetMockSchemasAPI().EXPECT()
e.GetByFullName(mock.Anything, "b.a").Return(&catalog.SchemaInfo{
Name: "a",
CatalogName: "b",
MetastoreId: "d",
Owner: "administrators",
}, nil)
},
Resource: ResourceSchema(),
Update: true,
ID: "b.a",
InstanceState: map[string]string{
"metastore_id": "d",
"name": "a",
"catalog_name": "b",
"force_destroy": "true",
},
HCL: `
name = "a"
catalog_name = "b"
force_destroy = false
`,
}.ApplyNoError(t)
}

func TestUpdateSchemaOwnerWithOtherFields(t *testing.T) {
qa.ResourceFixture{
MockWorkspaceClientFunc: func(w *mocks.MockWorkspaceClient) {
Expand Down
4 changes: 4 additions & 0 deletions catalog/resource_volume.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,10 @@ func ResourceVolume() common.Resource {
return nil
}

if d.HasChange("comment") && updateVolumeRequestContent.Comment == "" {
updateVolumeRequestContent.ForceSendFields = append(updateVolumeRequestContent.ForceSendFields, "Comment")
}

updateVolumeRequestContent.Owner = ""
v, err := w.Volumes.Update(ctx, updateVolumeRequestContent)
if err != nil {
Expand Down
59 changes: 59 additions & 0 deletions catalog/resource_volume_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,65 @@ func TestVolumesUpdate(t *testing.T) {
assert.Equal(t, "/Volumes/testCatalogName/testSchemaName/testNameNew", d.Get("volume_path"))
}

func TestVolumesUpdateCommentOnly(t *testing.T) {
qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
{
Method: http.MethodPatch,
Resource: "/api/2.1/unity-catalog/volumes/testCatalogName.testSchemaName.testName",
ExpectedRequest: catalog.UpdateVolumeRequestContent{
Comment: "",
ForceSendFields: []string{"Comment"},
},
Response: catalog.VolumeInfo{
Name: "testName",
VolumeType: catalog.VolumeType("testVolumeType"),
CatalogName: "testCatalogName",
SchemaName: "testSchemaName",
Comment: "",
FullName: "testCatalogName.testSchemaName.testName",
},
},
{
Method: http.MethodGet,
Resource: "/api/2.1/unity-catalog/volumes/testCatalogName.testSchemaName.testName?",
Response: catalog.VolumeInfo{
Name: "testName",
VolumeType: catalog.VolumeType("testVolumeType"),
CatalogName: "testCatalogName",
SchemaName: "testSchemaName",
Comment: "",
FullName: "testCatalogName.testSchemaName.testNameNew",
},
},
},
Resource: ResourceVolume(),
Update: true,
InstanceState: map[string]string{
"name": "testName",
"catalog_name": "testCatalogName",
"schema_name": "testSchemaName",
"volume_type": "testVolumeType",
"comment": "this is a comment",
},
ID: "testCatalogName.testSchemaName.testName",
HCL: `
name = "testName"
volume_type = "testVolumeType"
catalog_name = "testCatalogName"
schema_name = "testSchemaName"
comment = ""
`,
}.ApplyAndExpectData(t, map[string]any{
"name": "testName",
"volume_type": "testVolumeType",
"catalog_name": "testCatalogName",
"schema_name": "testSchemaName",
"comment": "",
"volume_path": "/Volumes/testCatalogName/testSchemaName/testNameNew",
})
}

func TestVolumesUpdateForceNewOnCatalog(t *testing.T) {
d, err := qa.ResourceFixture{
Fixtures: []qa.HTTPFixture{
Expand Down

0 comments on commit 03a2515

Please sign in to comment.