[Internal] Migrate Share Data Source to Plugin Framework #4161

rauchy · 2024-10-28T08:22:38Z

Changes

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

Tests

~~Note: current tests create shares using the SDKv2 resource, but fetch them using the new plugin framework data source. Once the resource migration will be merged, I will amend this.~~ Edit: Now that the resource itself is merged, the acceptance tests use the plugin framework's version of the resource.

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

alexott

Why do we need to do staging for data sources? Imho it's less critical than resources.

but the main issue IMHO is that we modify the acceptance tests to work only with the staged resources, and leaving a hole in acceptance testing of "normal" data sources

alexott · 2024-10-28T10:10:32Z

internal/providers/pluginfw/resources/share/data_share.go

+	attrs, blocks := tfschema.DataSourceStructToSchemaMap(sharing_tf.ShareInfo{}, nil)
+	resp.Schema = schema.Schema{
+		Attributes: attrs,
+		Blocks:     blocks,
+	}


It's not a direct problem, but such code is repeated everywhere. Should we add a helper function that we can use instead?

alexott · 2024-10-29T11:13:35Z

@rauchy I think that it makes sense just to duplicate tests, not to replace resources/data sources, so we keep tests for SDK-based resources as well, and remove the duplicate tests when we replace SDK-based with plugin framework-based

rauchy · 2024-10-29T11:22:25Z

@alexott that was exactly what I was about to propose :)

tanmay-db · 2024-10-29T14:19:42Z

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

+}
+
+func (d *ShareDataSource) Metadata(ctx context.Context, req datasource.MetadataRequest, resp *datasource.MetadataResponse) {
+	resp.TypeName = pluginfwcommon.GetDatabricksStagingName("share")


nit: We should abstract this out above like: https://github.com/databricks/terraform-provider-databricks/blob/main/internal/providers/pluginfw/resources/volume/data_volumes.go#L19

See my comment below.

tanmay-db · 2024-10-29T14:21:03Z

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

+		Name:              config.Name.ValueString(),
+		IncludeSharedData: true,
+	})
+	if err != nil {


We should remove from state if missing:

if apierr.IsMissing(err) { resp.State.RemoveResource(ctx) }

Good to know that's needed :) b7c32b7

tanmay-db · 2024-10-29T14:21:33Z

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

+}
+
+func (d *SharesDataSource) Metadata(ctx context.Context, req datasource.MetadataRequest, resp *datasource.MetadataResponse) {
+	resp.TypeName = pluginfwcommon.GetDatabricksStagingName("shares")


Initially, I planned to use a dataSourceName constant for both the share and shares data sources. However, since they share the same namespace, that wasn’t an option. So, I just included the constant within the Metadata function, particularly since it’s only used once in each data source. Alternatively, I could assign a dataSourceName constant to one data source and rename it for the other, but that didn't feel quite right.

Let me know if you have a preferred approach.

I see, yeah we could use dataSourceNameShare, dataSourceNameShares, mentioning this because we would also need to set the useragent (eg: https://github.com/databricks/terraform-provider-databricks/blob/main/internal/providers/pluginfw/resources/volume/data_volumes.go#L56) and use the same name there as well so might be better to abstract this in a single variable.

eng-dev-ecosystem-bot · 2024-10-30T10:54:54Z

Test Details: go/deco-tests/11591746843

### 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)).

…mework (#4187) ## Changes  Following up for #4161 (comment), I noticed some other resources also aren't setting the user agent. Creating a PR to fix them. Also added a note in the contributing guideline. ## Tests  NA - [ ] `make test` run locally - [ ] relevant change in `docs/` folder - [ ] covered with integration tests in `internal/acceptance` - [ ] relevant acceptance tests are passing - [ ] using Go SDK

rauchy requested review from a team as code owners October 28, 2024 08:22

rauchy requested review from parthban-db and removed request for a team October 28, 2024 08:22

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

tanmay-db self-requested a review October 28, 2024 09:42

alexott reviewed Oct 28, 2024

View reviewed changes

rauchy force-pushed the migrate-share-datasource-to-plugin-framework branch from 58c0ba5 to 2235d30 Compare October 29, 2024 10:46

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

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

rauchy force-pushed the migrate-share-datasource-to-plugin-framework branch from 227dbb6 to 74ad0f6 Compare October 29, 2024 11:42

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

tanmay-db approved these changes Oct 29, 2024

View reviewed changes

alexott approved these changes Oct 29, 2024

View reviewed changes

rauchy force-pushed the migrate-share-datasource-to-plugin-framework branch from 74ad0f6 to b7c32b7 Compare October 30, 2024 06:01

rauchy had a problem deploying to test-trigger-is October 30, 2024 06:01 — with GitHub Actions Error

rauchy enabled auto-merge October 30, 2024 06:03

rauchy added 2 commits October 30, 2024 11:53

add data sources for Share and Shares

129edb4

remove resource from state if not found

7e28bac

rauchy force-pushed the migrate-share-datasource-to-plugin-framework branch from b7c32b7 to 7e28bac Compare October 30, 2024 10:54

rauchy temporarily deployed to test-trigger-is October 30, 2024 10:54 — with GitHub Actions Inactive

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

Merged via the queue into main with commit 613ed1a Oct 30, 2024
12 checks passed

rauchy deleted the migrate-share-datasource-to-plugin-framework branch October 30, 2024 12:39

tanmay-db mentioned this pull request Oct 31, 2024

[Internal] Set user agent in some resources implemented in plugin framework #4187

Merged

5 tasks

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 Data Source to Plugin Framework #4161

[Internal] Migrate Share Data Source to Plugin Framework #4161

rauchy commented Oct 28, 2024 •

edited

Loading

alexott left a comment

alexott Oct 28, 2024

alexott commented Oct 29, 2024

rauchy commented Oct 29, 2024

tanmay-db Oct 29, 2024

rauchy Oct 30, 2024

tanmay-db Oct 29, 2024

rauchy Oct 30, 2024

tanmay-db Oct 29, 2024

rauchy Oct 30, 2024

tanmay-db Oct 30, 2024

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

[Internal] Migrate Share Data Source to Plugin Framework #4161

[Internal] Migrate Share Data Source to Plugin Framework #4161

Conversation

rauchy commented Oct 28, 2024 • edited Loading

Changes

Tests

alexott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexott commented Oct 29, 2024

rauchy commented Oct 29, 2024

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

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

rauchy commented Oct 28, 2024 •

edited

Loading