From f1b12360fd0cb91b614a87bea769a12c1be84f0e Mon Sep 17 00:00:00 2001 From: Vuong Date: Fri, 5 Jan 2024 22:17:58 +0000 Subject: [PATCH 1/3] allow updating owner of `databricks_connection` --- catalog/resource_connection.go | 22 +++++++++------ catalog/resource_connection_test.go | 43 +++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 9 deletions(-) diff --git a/catalog/resource_connection.go b/catalog/resource_connection.go index af2dbdfd30..191df6d50b 100644 --- a/catalog/resource_connection.go +++ b/catalog/resource_connection.go @@ -5,7 +5,6 @@ import ( "github.com/databricks/databricks-sdk-go/service/catalog" "github.com/databricks/terraform-provider-databricks/common" - "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "golang.org/x/exp/slices" ) @@ -35,7 +34,7 @@ type ConnectionInfo struct { ReadOnly bool `json:"read_only,omitempty" tf:"force_new,computed"` } -var sensitiveOptions = []string{"user", "password", "personalAccessToken", "access_token", "client_secret", "OAuthPvtKey"} +var sensitiveOptions = []string{"user", "password", "personalAccessToken", "access_token", "client_secret", "OAuthPvtKey", "GoogleServiceAccountKeyJson"} func ResourceConnection() *schema.Resource { s := common.StructToSchema(ConnectionInfo{}, @@ -47,9 +46,6 @@ func ResourceConnection() *schema.Resource { return common.Resource{ Schema: s, Create: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error { - if d.Get("owner") != "" { - tflog.Warn(context.Background(), "owner field not currently supported. Support will be enabled in a future update.") - } w, err := c.WorkspaceClient() if err != nil { return err @@ -60,7 +56,18 @@ func ResourceConnection() *schema.Resource { } var createConnectionRequest catalog.CreateConnection common.DataToStructPointer(d, s, &createConnectionRequest) - conn, err := w.Connections.Create(ctx, createConnectionRequest) + _, err = w.Connections.Create(ctx, createConnectionRequest) + if err != nil { + return err + } + // Update owner if it is provided + if d.Get("owner") == "" { + return nil + } + var updateConnectionRequest catalog.UpdateConnection + common.DataToStructPointer(d, s, &updateConnectionRequest) + updateConnectionRequest.NameArg = updateConnectionRequest.Name + conn, err := w.Connections.Update(ctx, updateConnectionRequest) if err != nil { return err } @@ -92,9 +99,6 @@ func ResourceConnection() *schema.Resource { return common.StructToData(conn, s, d) }, Update: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error { - if d.Get("owner") != "" { - tflog.Warn(context.Background(), "owner field not currently supported. Support will be enabled in a future update.") - } w, err := c.WorkspaceClient() if err != nil { return err diff --git a/catalog/resource_connection_test.go b/catalog/resource_connection_test.go index cf908e1a5f..a233504b6d 100644 --- a/catalog/resource_connection_test.go +++ b/catalog/resource_connection_test.go @@ -64,6 +64,49 @@ func TestConnectionsCreate(t *testing.T) { }, }, }, + { + Method: http.MethodPatch, + Resource: "/api/2.1/unity-catalog/connections/testConnectionName", + ExpectedRequest: catalog.UpdateConnection{ + Name: "testConnectionName", + Options: map[string]string{ + "host": "test.com", + }, + Owner: "InitialOwner", + }, + Response: catalog.ConnectionInfo{ + Name: "testConnectionName", + ConnectionType: catalog.ConnectionType("testConnectionType"), + Comment: "This is a test comment.", + FullName: "testConnectionName", + MetastoreId: "abc", + Owner: "InitialOwner", + Options: map[string]string{ + "host": "test.com", + }, + Properties: map[string]string{ + "purpose": "testing", + }, + }, + }, + { + Method: http.MethodGet, + Resource: "/api/2.1/unity-catalog/connections/testConnectionName?", + Response: catalog.ConnectionInfo{ + Name: "testConnectionName", + ConnectionType: catalog.ConnectionType("testConnectionType"), + Comment: "This is a test comment.", + FullName: "testConnectionName", + Owner: "InitialOwner", + MetastoreId: "abc", + Options: map[string]string{ + "host": "test.com", + }, + Properties: map[string]string{ + "purpose": "testing", + }, + }, + }, }, Resource: ResourceConnection(), Create: true, From fa4d3c6dcb9c34c44f55514004fbcc637b62dc05 Mon Sep 17 00:00:00 2001 From: Vuong Date: Mon, 8 Jan 2024 14:26:21 +0000 Subject: [PATCH 2/3] split owner update logic --- catalog/resource_connection.go | 34 ++++++-- catalog/resource_connection_test.go | 116 ++++++++++++++++++++++--- internal/acceptance/connection_test.go | 34 +++++--- 3 files changed, 153 insertions(+), 31 deletions(-) diff --git a/catalog/resource_connection.go b/catalog/resource_connection.go index 191df6d50b..3f556f1631 100644 --- a/catalog/resource_connection.go +++ b/catalog/resource_connection.go @@ -26,7 +26,7 @@ type ConnectionInfo struct { // A map of key-value properties attached to the securable. Options map[string]string `json:"options" tf:"sensitive"` // Username of current owner of the connection. - Owner string `json:"owner,omitempty" tf:"force_new,suppress_diff"` + Owner string `json:"owner,omitempty" tf:"computed"` // An object containing map of key-value properties attached to the // connection. Properties map[string]string `json:"properties,omitempty" tf:"force_new"` @@ -110,17 +110,39 @@ func ResourceConnection() *schema.Resource { var updateConnectionRequest catalog.UpdateConnection common.DataToStructPointer(d, s, &updateConnectionRequest) _, connName, err := pi.Unpack(d) - updateConnectionRequest.NameArg = connName if err != nil { return err } - conn, err := w.Connections.Update(ctx, updateConnectionRequest) + updateConnectionRequest.NameArg = connName + + if d.HasChange("owner") { + _, err = w.Connections.Update(ctx, catalog.UpdateConnection{ + Name: updateConnectionRequest.Name, + NameArg: updateConnectionRequest.Name, + Owner: updateConnectionRequest.Owner, + }) + if err != nil { + return err + } + } + + updateConnectionRequest.Owner = "" + _, err = w.Connections.Update(ctx, updateConnectionRequest) if err != nil { + if d.HasChange("owner") { + // Rollback + old, new := d.GetChange("owner") + _, rollbackErr := w.Connections.Update(ctx, catalog.UpdateConnection{ + Name: updateConnectionRequest.Name, + NameArg: updateConnectionRequest.Name, + Owner: old.(string), + }) + if rollbackErr != nil { + return common.OwnerRollbackError(err, rollbackErr, old.(string), new.(string)) + } + } return err } - // We need to repack the Id as the name may have changed - d.Set("name", conn.Name) - pi.Pack(d) return nil }, Delete: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error { diff --git a/catalog/resource_connection_test.go b/catalog/resource_connection_test.go index a233504b6d..8035d79948 100644 --- a/catalog/resource_connection_test.go +++ b/catalog/resource_connection_test.go @@ -226,7 +226,7 @@ func TestConnectionRead_Error(t *testing.T) { } func TestConnectionsUpdate(t *testing.T) { - d, err := qa.ResourceFixture{ + qa.ResourceFixture{ Fixtures: []qa.HTTPFixture{ { Method: http.MethodGet, @@ -242,13 +242,13 @@ func TestConnectionsUpdate(t *testing.T) { Method: http.MethodPatch, Resource: "/api/2.1/unity-catalog/connections/testConnectionName", ExpectedRequest: catalog.UpdateConnection{ - Name: "testConnectionNameNew", + Name: "testConnectionName", Options: map[string]string{ "host": "test.com", }, }, Response: catalog.ConnectionInfo{ - Name: "testConnectionNameNew", + Name: "testConnectionName", ConnectionType: catalog.ConnectionType("testConnectionType"), Comment: "testComment", MetastoreId: "abc", @@ -259,9 +259,9 @@ func TestConnectionsUpdate(t *testing.T) { }, { Method: http.MethodGet, - Resource: "/api/2.1/unity-catalog/connections/testConnectionNameNew?", + Resource: "/api/2.1/unity-catalog/connections/testConnectionName?", Response: catalog.ConnectionInfo{ - Name: "testConnectionNameNew", + Name: "testConnectionName", ConnectionType: catalog.ConnectionType("testConnectionType"), Comment: "testComment", MetastoreId: "abc", @@ -279,18 +279,108 @@ func TestConnectionsUpdate(t *testing.T) { "comment": "testComment", }, HCL: ` - name = "testConnectionNameNew" + name = "testConnectionName" connection_type = "testConnectionType" comment = "testComment" options = { host = "test.com" } `, - }.Apply(t) - assert.NoError(t, err) - assert.Equal(t, "testConnectionNameNew", d.Get("name")) - assert.Equal(t, "testConnectionType", d.Get("connection_type")) - assert.Equal(t, "testComment", d.Get("comment")) + }.ApplyAndExpectData(t, map[string]any{ + "name": "testConnectionName", + "connection_type": "testConnectionType", + "comment": "testComment", + }) +} + +func TestConnectionsUpdateOwnerAndOtherFields(t *testing.T) { + qa.ResourceFixture{ + Fixtures: []qa.HTTPFixture{ + { + Method: http.MethodGet, + Resource: "/api/2.1/unity-catalog/connections/testConnectionName?", + Response: catalog.ConnectionInfo{ + Name: "testConnectionName", + ConnectionType: catalog.ConnectionType("testConnectionType"), + MetastoreId: "abc", + Comment: "testComment", + }, + }, + { + Method: http.MethodPatch, + Resource: "/api/2.1/unity-catalog/connections/testConnectionName", + ExpectedRequest: catalog.UpdateConnection{ + Name: "testConnectionName", + Owner: "admin", + }, + Response: catalog.ConnectionInfo{ + Name: "testConnectionName", + ConnectionType: catalog.ConnectionType("testConnectionType"), + Comment: "testComment", + MetastoreId: "abc", + Options: map[string]string{ + "host": "test.com", + }, + Owner: "admin", + }, + }, + { + Method: http.MethodPatch, + Resource: "/api/2.1/unity-catalog/connections/testConnectionName", + ExpectedRequest: catalog.UpdateConnection{ + Name: "testConnectionName", + Options: map[string]string{ + "host": "test.com", + }, + }, + Response: catalog.ConnectionInfo{ + Name: "testConnectionName", + ConnectionType: catalog.ConnectionType("testConnectionType"), + Comment: "testComment", + MetastoreId: "abc", + Options: map[string]string{ + "host": "test.com", + }, + Owner: "admin", + }, + }, + { + Method: http.MethodGet, + Resource: "/api/2.1/unity-catalog/connections/testConnectionName?", + Response: catalog.ConnectionInfo{ + Name: "testConnectionName", + ConnectionType: catalog.ConnectionType("testConnectionType"), + Comment: "testComment", + MetastoreId: "abc", + Options: map[string]string{ + "host": "test.com", + }, + Owner: "admin", + }, + }, + }, + Resource: ResourceConnection(), + Update: true, + ID: "abc|testConnectionName", + InstanceState: map[string]string{ + "connection_type": "testConnectionType", + "comment": "testComment", + }, + HCL: ` + name = "testConnectionName" + connection_type = "testConnectionType" + comment = "testComment" + options = { + host = "test.com" + } + owner = "admin" + `, + }.ApplyAndExpectData(t, map[string]any{ + "name": "testConnectionName", + "connection_type": "testConnectionType", + "comment": "testComment", + "owner": "admin", + }) } func TestConnectionUpdate_Error(t *testing.T) { @@ -300,7 +390,7 @@ func TestConnectionUpdate_Error(t *testing.T) { Method: http.MethodPatch, Resource: "/api/2.1/unity-catalog/connections/testConnectionName", ExpectedRequest: catalog.UpdateConnection{ - Name: "testConnectionNameNew", + Name: "testConnectionName", Options: map[string]string{ "host": "test.com", }, @@ -320,7 +410,7 @@ func TestConnectionUpdate_Error(t *testing.T) { "comment": "testComment", }, HCL: ` - name = "testConnectionNameNew" + name = "testConnectionName" connection_type = "testConnectionType" options = { host = "test.com" diff --git a/internal/acceptance/connection_test.go b/internal/acceptance/connection_test.go index 320b64a4c2..9f0fa6c2d6 100644 --- a/internal/acceptance/connection_test.go +++ b/internal/acceptance/connection_test.go @@ -1,22 +1,32 @@ package acceptance import ( + "fmt" "testing" ) +func connectionTemplateWithOwner(host string, owner string) string { + return fmt.Sprintf(` + resource "databricks_connection" "this" { + name = "name-{var.STICKY_RANDOM}" + connection_type = "MYSQL" + comment = "this is a connection to mysql db" + options = { + host = %s + port = "3306" + user = "user" + password = "password" + } + owner = %s + } + `, host, owner) +} func TestUcAccConnectionsResourceFullLifecycle(t *testing.T) { unityWorkspaceLevel(t, step{ - Template: ` - resource "databricks_connection" "this" { - name = "name-{var.STICKY_RANDOM}" - connection_type = "MYSQL" - comment = "this is a connection to mysql db" - options = { - host = "test.mysql.database.azure.com" - port = "3306" - user = "user" - password = "password" - } - }`, + Template: connectionTemplateWithOwner("test.mysql.database.azure.com", "account users"), + }, step{ + Template: connectionTemplateWithOwner("test.mysql.database.aws.com", "account users"), + }, step{ + Template: connectionTemplateWithOwner("test.mysql.database.azure.com", "{env.TEST_METASTORE_ADMIN_GROUP_NAME}"), }) } From 67401484c86fc8fe8874b1388643923a502d1017 Mon Sep 17 00:00:00 2001 From: Vuong Date: Mon, 8 Jan 2024 14:57:50 +0000 Subject: [PATCH 3/3] fix test --- internal/acceptance/connection_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/acceptance/connection_test.go b/internal/acceptance/connection_test.go index 9f0fa6c2d6..714ea33a53 100644 --- a/internal/acceptance/connection_test.go +++ b/internal/acceptance/connection_test.go @@ -12,12 +12,12 @@ func connectionTemplateWithOwner(host string, owner string) string { connection_type = "MYSQL" comment = "this is a connection to mysql db" options = { - host = %s + host = "%s" port = "3306" user = "user" password = "password" } - owner = %s + owner = "%s" } `, host, owner) }