[Internal] Migrate Share Resource to Plugin Framework #4047

rauchy · 2024-09-26T08:26:26Z

Changes

This PR migrates the share resource to the Plugin framework. The code was largely copied "as is" from the previous implementation of the share resource, with the necessary adaptations made for integration with the Plugin framework.

This implementation utilizes the newly generated Effective fields to provide the functionality that was previously achieved through diff suppression.

Tests

make test run locally
relevant change in docs/ folder
covered with integration tests in internal/acceptance
relevant acceptance tests are passing
using Go SDK

renaudhartert-db

No major concerns about the business logic (though I'm probably not the best person to review it). I left a few nit comments about style.

renaudhartert-db · 2024-10-16T09:29:00Z

internal/providers/pluginfw/resources/sharing/resource_share.go

+		_, exists := afterMap[beforeSdo.Name]
+		if exists {


[nit] inline to make it clear that exists is not used outside of the condition. Also, it is more idiomatic to call such variable ok:

Suggested change

_, exists := afterMap[beforeSdo.Name]

if exists {

if _, ok := afterMap[beforeSdo.Name]; ok {

renaudhartert-db · 2024-10-16T09:29:29Z

internal/providers/pluginfw/resources/sharing/resource_share.go

+		beforeSdo, exists := beforeMap[afterSdo.Name]
+		if exists {


Ditto

Suggested change

beforeSdo, exists := beforeMap[afterSdo.Name]

if exists {

if beforeSdo, ok := beforeMap[afterSdo.Name]; ok {

renaudhartert-db · 2024-10-16T09:30:28Z

internal/providers/pluginfw/resources/sharing/resource_share.go

+		changes = append(changes, sharing.SharedDataObjectUpdate{
+			Action:     sharing.SharedDataObjectUpdateAction(action),
+			DataObject: &obj,
+		},
+		)


Curious, did this pass gofmt?

Suggested change

changes = append(changes, sharing.SharedDataObjectUpdate{

Action: sharing.SharedDataObjectUpdateAction(action),

DataObject: &obj,

},

)

changes = append(changes, sharing.SharedDataObjectUpdate{

Action: sharing.SharedDataObjectUpdateAction(action),

DataObject: &obj,

})

with flying colors 🌈

renaudhartert-db · 2024-10-16T09:35:50Z

internal/providers/pluginfw/resources/sharing/resource_share.go

+	if len(changes) > 0 {
+		// if there are any other changes, update the share with the changes
+		updatedShareInfo, err := client.Shares.Update(ctx, sharing.UpdateShare{
+			Name:    plan.Name.ValueString(),
+			Updates: changes,
+		})
+		if err == nil {
+			resp.Diagnostics.Append(converters.GoSdkToTfSdkStruct(ctx, updatedShareInfo, &state)...)
+			if resp.Diagnostics.HasError() {
+				return
+			}
+		} else {
+			rollbackShareInfo, rollbackErr := client.Shares.Update(ctx, sharing.UpdateShare{
+				Name:  currentShareInfo.Name,
+				Owner: currentShareInfo.Owner,
+			})
+			if rollbackErr == nil {
+				resp.Diagnostics.Append(converters.GoSdkToTfSdkStruct(ctx, rollbackShareInfo, &state)...)
+				if resp.Diagnostics.HasError() {
+					return
+				}
+			} else {
+				resp.Diagnostics.AddError("failed to roll back", common.OwnerRollbackError(err, rollbackErr, currentShareInfo.Owner, plan.Owner.ValueString()).Error())
+			}
+
+			resp.Diagnostics.AddError("failed to update share", err.Error())
+			return
+		}
+	}


[nit] We can be a little more idiomatic here by only having the error flow at the highest level of indentation.

Ref: https://go.dev/wiki/CodeReviewComments#indent-error-flow

Suggested change

if len(changes) > 0 {

// if there are any other changes, update the share with the changes

updatedShareInfo, err := client.Shares.Update(ctx, sharing.UpdateShare{

Name: plan.Name.ValueString(),

Updates: changes,

})

if err == nil {

resp.Diagnostics.Append(converters.GoSdkToTfSdkStruct(ctx, updatedShareInfo, &state)...)

if resp.Diagnostics.HasError() {

return

}

} else {

rollbackShareInfo, rollbackErr := client.Shares.Update(ctx, sharing.UpdateShare{

Name: currentShareInfo.Name,

Owner: currentShareInfo.Owner,

})

if rollbackErr == nil {

resp.Diagnostics.Append(converters.GoSdkToTfSdkStruct(ctx, rollbackShareInfo, &state)...)

if resp.Diagnostics.HasError() {

return

}

} else {

resp.Diagnostics.AddError("failed to roll back", common.OwnerRollbackError(err, rollbackErr, currentShareInfo.Owner, plan.Owner.ValueString()).Error())

}

resp.Diagnostics.AddError("failed to update share", err.Error())

return

}

}

if len(changes) > 0 {

// if there are any other changes, update the share with the changes

updatedShareInfo, err := client.Shares.Update(ctx, sharing.UpdateShare{

Name: plan.Name.ValueString(),

Updates: changes,

})

if err != nil {

rollbackShareInfo, rollbackErr := client.Shares.Update(ctx, sharing.UpdateShare{

Name: currentShareInfo.Name,

Owner: currentShareInfo.Owner,

})

if rollbackErr == nil {

resp.Diagnostics.Append(converters.GoSdkToTfSdkStruct(ctx, rollbackShareInfo, &state)...)

if resp.Diagnostics.HasError() {

return

}

} else {

resp.Diagnostics.AddError("failed to roll back", common.OwnerRollbackError(err, rollbackErr, currentShareInfo.Owner, plan.Owner.ValueString()).Error())

}

resp.Diagnostics.AddError("failed to update share", err.Error())

return

}

resp.Diagnostics.Append(converters.GoSdkToTfSdkStruct(ctx, updatedShareInfo, &state)...)

if resp.Diagnostics.HasError() {

return

}

}

renaudhartert-db · 2024-10-16T09:39:59Z

internal/providers/pluginfw/resources/sharing/resource_share.go

+				resp.Diagnostics.AddError("failed to roll back", common.OwnerRollbackError(err, rollbackErr, currentShareInfo.Owner, plan.Owner.ValueString()).Error())
+			}
+
+			resp.Diagnostics.AddError("failed to update share", err.Error())


[nit] Can the last line resp.Diagnostics.AddError("failed to update share", err.Error()) come at the beginning of this else block? If so, we can make the code more readable by reducing the overall level of indentation.

Ref: https://go.dev/wiki/CodeReviewComments#indent-error-flow

resp.Diagnostics.AddError("failed to update share", err.Error()) rollbackShareInfo, rollbackErr := client.Shares.Update(ctx, sharing.UpdateShare{ Name: currentShareInfo.Name, Owner: currentShareInfo.Owner, }) if rollbackErr != nil { resp.Diagnostics.AddError("failed to roll back", common.OwnerRollbackError(err, rollbackErr, currentShareInfo.Owner, plan.Owner.ValueString()).Error()) return } resp.Diagnostics.Append(converters.GoSdkToTfSdkStruct(ctx, rollbackShareInfo, &state)...) if resp.Diagnostics.HasError() { return }

rauchy · 2024-10-16T12:04:02Z

Thanks @renaudhartert-db for the review! Most of the issues are inherited from the migrated code, but no reason not to tidy up now :) Appreciate the comments.

tanmay-db

Thanks for the PR, I had some comments, rest LGTM, stamping to unblock merge once addressed.

tanmay-db · 2024-10-18T09:13:20Z

internal/providers/pluginfw/resources/sharing/resource_acc_test.go

+		}`, owner, comment)
+}
+
+func TestUcAccUpdateShare(t *testing.T) {


Can you please add an integration test which adds an object in middle in the update step? This was a limitation in SDKv2 as ordering used to matter whereas now it should work with plugin framework.

I think this would fail currently because object in ShareInfo that we use for schema is a list and it should be set instead.

type ShareInfoExtended struct { sharing_tf.ShareInfo } .... Objects []SharedDataObject `tfsdk:"object" tf:"optional"`

Good catch! Added support + integration test in 4b7eb0e

tanmay-db · 2024-10-18T09:13:48Z

internal/providers/pluginfw/resources/sharing/resource_share.go

+	sharing_tf.ShareInfo
+}
+
+func matchOrder[T any, K comparable](target, reference []T, keyFunc func(T) K) {


Having this covered with unit test might be good.

I'm not entirely sure. Testing this on a unit level feels like testing implementation details. If this element becomes reusable across different resources, which could happen soon, I'd be more inclined to test it at a unit level. Right now, it seems like we might tweak the implementation as we figure out how to scale this method (like switching to sets). So, unit tests might end up breaking for reasons that aren't really important, like when we refactor the code. We can definitely revisit this and consider unit tests if we start reusing the code and it makes sense to treat it as its own unit.

sounds good 👍

tanmay-db · 2024-10-18T09:17:02Z

internal/providers/pluginfw/resources/sharing/resource_share.go

+	if err != nil {
+		// delete orphaned share if update fails
+		if d_err := w.Shares.DeleteByName(ctx, shareInfo.Name); d_err != nil {
+			resp.Diagnostics.AddError("failed to delete orphaned share", d_err.Error())


We should also add err along with d_err in the diagnostics here as we return so line 205 won't execute.

tanmay-db · 2024-10-18T09:21:24Z

internal/providers/pluginfw/resources/sharing/resource_share.go

+}
+
+func (r *ShareResource) Read(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) {
+	logger.SetTfLogger(logger.NewTfLogger(ctx))


I think we should do this while we configure the client so it gets set for each CRUD in all resources like we do in sdkv2 :https://github.com/databricks/terraform-provider-databricks/blob/main/internal/providers/sdkv2/sdkv2.go#L235.

It doesn't have to be in this PR so we can remove this and create a ticket to track adding this properly.

Heh, no this is just sloppy leftovers from debugging. Oops!

tanmay-db · 2024-10-18T09:22:39Z

internal/providers/pluginfw/resources/sharing/resource_share.go

+}
+
+func (r *ShareResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) {
+	w, diags := r.Client.GetWorkspaceClient()


Also need to add
ctx = pluginfwcontext.SetUserAgentInResourceContext(ctx, resourceName) in each CRUD, ref: https://github.com/databricks/terraform-provider-databricks/blob/main/internal/providers/pluginfw/resources/qualitymonitor/resource_quality_monitor.go#L101

👍 e350013

tanmay-db · 2024-10-18T09:27:36Z

internal/providers/pluginfw/resources/sharing/resource_share.go

+		}
+	}
+
+	state.SyncEffectiveFieldsDuringCreateOrUpdate(plan.ShareInfo)


Is there an easy way to test this? Also is this something we do this right after update/create is called? i.e. line 328

tanmay-db

I think we might need to use set instead of list here: https://github.com/databricks/terraform-provider-databricks/pull/4047/files#r1806166473.

…e resource

…at case

eng-dev-ecosystem-bot · 2024-10-29T08:25:43Z

Test Details: go/deco-tests/11569951828

### New Features and Improvements * Added `databricks_functions` data source ([#4154](#4154)). ### Bug Fixes * Handle edge case for `effective_properties` in `databricks_sql_table` ([#4153](#4153)). * Provide more prescriptive error when users fail to create a single node cluster ([#4168](#4168)). ### Internal Changes * Add test instructions for external contributors ([#4169](#4169)). * Always write message for manual test integration ([#4188](#4188)). * Make `Read` after `Create`/`Update` configurable ([#4190](#4190)). * Migrate Share Data Source to Plugin Framework ([#4161](#4161)). * Migrate Share Resource to Plugin Framework ([#4047](#4047)). * Rollout Plugin Framework ([#4134](#4134)). ### Dependency Updates * Bump Go SDK to v0.50.0 ([#4178](#4178)). ### Exporter * Allow to match resource names by regular expression ([#4177](#4177)).

rauchy changed the title ~~Migrate Share Resource to Plugin Framework~~ [Internal] Migrate Share Resource to Plugin Framework Sep 26, 2024

rauchy force-pushed the rauchy/migrate-share-resource-to-plugin-framework branch from 9beb175 to 7a25dfd Compare September 26, 2024 09:09

rauchy force-pushed the rauchy/migrate-share-resource-to-plugin-framework branch 5 times, most recently from 37824e8 to 772f102 Compare October 15, 2024 12:03

rauchy marked this pull request as ready for review October 15, 2024 12:14

rauchy requested review from a team as code owners October 15, 2024 12:14

rauchy requested review from renaudhartert-db and removed request for a team October 15, 2024 12:14

rauchy force-pushed the rauchy/migrate-share-resource-to-plugin-framework branch from c0ef184 to 92d9927 Compare October 15, 2024 13:21

rauchy requested a review from tanmay-db October 15, 2024 13:27

rauchy force-pushed the rauchy/migrate-share-resource-to-plugin-framework branch from 92d9927 to 81769af Compare October 15, 2024 14:09

renaudhartert-db approved these changes Oct 16, 2024

View reviewed changes

tanmay-db approved these changes Oct 18, 2024

View reviewed changes

tanmay-db reviewed Oct 21, 2024

View reviewed changes

rauchy force-pushed the rauchy/migrate-share-resource-to-plugin-framework branch from 2efc446 to 4b7eb0e Compare October 24, 2024 13:49

rauchy temporarily deployed to test-trigger-is October 24, 2024 13:49 — with GitHub Actions Inactive

rauchy temporarily deployed to test-trigger-is October 24, 2024 15:43 — with GitHub Actions Inactive

rauchy added 8 commits October 29, 2024 09:25

[Internal] CRUD operations for plugin framework's version of the Shar…

52325e6

…e resource

fix failing integration test

8bdcf00

simplify variable names

7e1e37c

use effective fields

68ce58f

add some diagnostics checks

9a42a81

rename acceptance test file to match contribution guide

6761603

use GetDatabricksStagingName for resource name

f4cb006

addressed some PR comments

a40043e

rauchy added 4 commits October 29, 2024 09:25

copy over effective fields from existing state during read operation

559b1f9

support adding objects anywhere in the list + integration test for th…

54fe63a

…at case

remove redundant log

3cc7c62

set user agent in resource context

52c62e3

rauchy force-pushed the rauchy/migrate-share-resource-to-plugin-framework branch from e350013 to 52c62e3 Compare October 29, 2024 08:25

rauchy temporarily deployed to test-trigger-is October 29, 2024 08:25 — with GitHub Actions Inactive

rauchy enabled auto-merge October 29, 2024 08:25

rauchy added this pull request to the merge queue Oct 29, 2024

Merged via the queue into main with commit 0975310 Oct 29, 2024
10 checks passed

rauchy deleted the rauchy/migrate-share-resource-to-plugin-framework branch October 29, 2024 09:30

tanmay-db mentioned this pull request Nov 5, 2024

[Release] Release v1.57.0 #4193

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Internal] Migrate Share Resource to Plugin Framework #4047

[Internal] Migrate Share Resource to Plugin Framework #4047

rauchy commented Sep 26, 2024 •

edited

Loading

renaudhartert-db left a comment

renaudhartert-db Oct 16, 2024

renaudhartert-db Oct 16, 2024

renaudhartert-db Oct 16, 2024

rauchy Oct 16, 2024

renaudhartert-db Oct 16, 2024

renaudhartert-db Oct 16, 2024 •

edited

Loading

rauchy commented Oct 16, 2024

tanmay-db left a comment

tanmay-db Oct 18, 2024

tanmay-db Oct 21, 2024 •

edited

Loading

rauchy Oct 24, 2024

tanmay-db Oct 18, 2024

rauchy Oct 24, 2024

tanmay-db Oct 24, 2024

tanmay-db Oct 18, 2024

tanmay-db Oct 18, 2024

rauchy Oct 24, 2024

tanmay-db Oct 18, 2024

rauchy Oct 24, 2024

tanmay-db Oct 18, 2024

tanmay-db left a comment

eng-dev-ecosystem-bot commented Oct 29, 2024

	_, exists := afterMap[beforeSdo.Name]
	if exists {
	if _, ok := afterMap[beforeSdo.Name]; ok {

	beforeSdo, exists := beforeMap[afterSdo.Name]
	if exists {
	if beforeSdo, ok := beforeMap[afterSdo.Name]; ok {

[Internal] Migrate Share Resource to Plugin Framework #4047

[Internal] Migrate Share Resource to Plugin Framework #4047

Conversation

rauchy commented Sep 26, 2024 • edited Loading

Changes

Tests

renaudhartert-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renaudhartert-db Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

rauchy commented Oct 16, 2024

tanmay-db left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanmay-db Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanmay-db left a comment

Choose a reason for hiding this comment

eng-dev-ecosystem-bot commented Oct 29, 2024

rauchy commented Sep 26, 2024 •

edited

Loading

renaudhartert-db Oct 16, 2024 •

edited

Loading

tanmay-db Oct 21, 2024 •

edited

Loading