[Internal] Refactor provider and related packages (#3940)

## Changes
<!-- Summary of your changes that are easy to understand -->
## Package organization for Providers
We are migrating the resource from SDKv2 to Plugin Framework provider
and hence both of them exist in the codebase. For uniform code
convention, readability and development, they are organized in the
`internal/providers` directory under root as follows:
- `providers`: Contains the changes that `depends` on both
internal/providers/sdkv2 and internal/providers/pluginfw packages, eg:
`GetProviderServer`.
- `common`: Contains the changes `used by` both internal/providers/sdkv2
and internal/providers/pluginfw packages, eg: `ProviderName`.
- `pluginfw`: Contains the changes specific to Plugin Framework. This
package shouldn't depend on sdkv2 or common.
- `sdkv2`: Contains the changes specific to SDKv2. This package
shouldn't depend on pluginfw or common.

This has been updated in CONTRIBUTING.md

Package comments have been added, also some tests have been refactored
to reduce redundancy example: we define test data path at multiple
places.

## Tests
<!-- 
How is this tested? Please see the checklist below and also describe any
other relevant tests
-->
Modified existing 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

CONTRIBUTING.md

@@ -109,6 +109,14 @@ provider_installation {
 
 After installing the necessary software for building provider from sources, you should be able to run `make coverage` to run the tests and see the coverage.
 
+## Package organization for Providers
+We are migrating the resource from SDKv2 to Plugin Framework provider and hence both of them exist in the codebase. For uniform code convention, readability and development, they are organized in the `internal/providers` directory under root as follows: 
+- `providers`: Contains the changes that `depends` on both internal/providers/sdkv2 and internal/providers/pluginfw packages, eg: `GetProviderServer`.
+- `common`: Contains the changes `used by` both internal/providers/sdkv2 and internal/providers/pluginfw packages, eg: `ProviderName`.
+- `pluginfw`: Contains the changes specific to Plugin Framework. This package shouldn't depend on sdkv2 or common.
+- `sdkv2`: Contains the changes specific to SDKv2. This package shouldn't depend on pluginfw or common.
+
+
 ## Debugging
 
 **TF_LOG_PROVIDER=DEBUG terraform apply** allows you to see the internal logs from `terraform apply`.

exporter/context.go

@@ -22,7 +22,7 @@ import (
 
 	"github.com/databricks/terraform-provider-databricks/commands"
 	"github.com/databricks/terraform-provider-databricks/common"
-	"github.com/databricks/terraform-provider-databricks/provider"
+	"github.com/databricks/terraform-provider-databricks/internal/providers/sdkv2"
 	"github.com/databricks/terraform-provider-databricks/scim"
 	"github.com/databricks/terraform-provider-databricks/workspace"
 
@@ -221,7 +221,7 @@ func makeResourcesChannels() map[string]resourceChannel {
 }
 
 func newImportContext(c *common.DatabricksClient) *importContext {
-	p := provider.DatabricksProvider()
+	p := sdkv2.DatabricksProvider()
 	p.TerraformVersion = "exporter"
 	p.SetMeta(c)
 	ctx := context.WithValue(context.Background(), common.Provider, p)

exporter/context_test.go

@@ -7,7 +7,7 @@ import (
 	"sync"
 	"testing"
 
-	"github.com/databricks/terraform-provider-databricks/provider"
+	"github.com/databricks/terraform-provider-databricks/internal/providers/sdkv2"
 	"github.com/databricks/terraform-provider-databricks/qa"
 	"github.com/databricks/terraform-provider-databricks/workspace"
 	"github.com/hashicorp/hcl/v2/hclwrite"
@@ -373,7 +373,7 @@ func TestLoadingLastRun(t *testing.T) {
 }
 
 func TestGenerateResourceIdForWsObject(t *testing.T) {
-	p := provider.DatabricksProvider()
+	p := sdkv2.DatabricksProvider()
 	ic := &importContext{
 		Importables: resourcesMap,
 		Resources:   p.ResourcesMap,

exporter/importables_test.go

@@ -26,10 +26,10 @@ import (
 	"github.com/databricks/terraform-provider-databricks/jobs"
 	"github.com/databricks/terraform-provider-databricks/permissions"
 
+	"github.com/databricks/terraform-provider-databricks/internal/providers/sdkv2"
 	dlt_pipelines "github.com/databricks/terraform-provider-databricks/pipelines"
 	"github.com/databricks/terraform-provider-databricks/policies"
 	"github.com/databricks/terraform-provider-databricks/pools"
-	"github.com/databricks/terraform-provider-databricks/provider"
 	"github.com/databricks/terraform-provider-databricks/qa"
 	"github.com/databricks/terraform-provider-databricks/repos"
 	"github.com/databricks/terraform-provider-databricks/scim"
@@ -45,7 +45,7 @@ import (
 )
 
 func importContextForTest() *importContext {
-	p := provider.DatabricksProvider()
+	p := sdkv2.DatabricksProvider()
 	supportedResources := maps.Keys(resourcesMap)
 	return &importContext{
 		Importables:               resourcesMap,

internal/acceptance/init_test.go

@@ -21,8 +21,8 @@ import (
 	"github.com/databricks/databricks-sdk-go/logger"
 	"github.com/databricks/terraform-provider-databricks/commands"
 	"github.com/databricks/terraform-provider-databricks/common"
+	"github.com/databricks/terraform-provider-databricks/internal/providers/sdkv2"
 	dbproviderlogger "github.com/databricks/terraform-provider-databricks/logger"
-	"github.com/databricks/terraform-provider-databricks/provider"
 	"github.com/databricks/terraform-provider-databricks/qa"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
@@ -138,7 +138,7 @@ func run(t *testing.T, steps []step) {
 		t.Skip("Acceptance tests skipped unless env 'CLOUD_ENV' is set")
 	}
 	t.Parallel()
-	provider := provider.DatabricksProvider()
+	provider := sdkv2.DatabricksProvider()
 	cwd, err := os.Getwd()
 	if err != nil {
 		t.Skip(err.Error())

internal/acceptance/mws_workspaces_test.go

@@ -11,7 +11,7 @@ import (
 	"github.com/databricks/databricks-sdk-go"
 	"github.com/databricks/databricks-sdk-go/logger"
 	"github.com/databricks/terraform-provider-databricks/common"
-	"github.com/databricks/terraform-provider-databricks/provider"
+	"github.com/databricks/terraform-provider-databricks/internal/providers/sdkv2"
 	"github.com/databricks/terraform-provider-databricks/tokens"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
@@ -378,14 +378,14 @@ func TestMwsAccAwsChangeToServicePrincipal(t *testing.T) {
 			spId := s.RootModule().Resources["databricks_service_principal.this"].Primary.ID
 			spAppId := s.RootModule().Resources["databricks_service_principal.this"].Primary.Attributes["application_id"]
 			spSecret := s.RootModule().Resources["databricks_service_principal_secret.this"].Primary.Attributes["secret"]
-			pr = provider.DatabricksProvider()
+			pr = sdkv2.DatabricksProvider()
 			rd := schema.TestResourceDataRaw(t, pr.Schema, map[string]interface{}{
 				"client_id":     spAppId,
 				"client_secret": spSecret,
 			})
 			fmt.Printf("client_id: %s, client_secret: %s\n", spAppId, spSecret)
 			pr.ConfigureContextFunc = func(ctx context.Context, c *schema.ResourceData) (interface{}, diag.Diagnostics) {
-				return provider.ConfigureDatabricksClient(ctx, rd)
+				return sdkv2.ConfigureDatabricksClient(ctx, rd)
 			}
 			logger.DefaultLogger = &logger.SimpleLogger{
 				Level: logger.LevelDebug,

internal/acceptance/tf_provider_test.go

@@ -3,7 +3,7 @@ package acceptance
 import (
 	"testing"
 
-	"github.com/databricks/terraform-provider-databricks/provider"
+	"github.com/databricks/terraform-provider-databricks/internal/providers/sdkv2"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 )
@@ -28,7 +28,7 @@ func TestAccProviderPlanShouldSucceedWithIncompleteConfiguration(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		IsUnitTest: true,
 		ProviderFactories: map[string]func() (*schema.Provider, error){
-			"databricks": func() (*schema.Provider, error) { return provider.DatabricksProvider(), nil },
+			"databricks": func() (*schema.Provider, error) { return sdkv2.DatabricksProvider(), nil },
 			"noop":       func() (*schema.Provider, error) { return noOpProvider(), nil },
 		},
 		Steps: []resource.TestStep{

internal/providers/common/common.go

@@ -0,0 +1,8 @@
+// Package common contains the changes used by both internal/providers/sdkv2 and internal/providers/pluginfw packages.
+//
+// Note: This is different from internal/providers which contains the changes that *depends* on both:
+// internal/providers/sdkv2 and internal/providers/pluginfw packages. Whereas, internal/providers/common package contains
+// the changes *used* by both internal/providers/sdkv2 and internal/providers/pluginfw packages.
+package internal
+
+const ProviderName = "databricks-tf-provider"

internal/pluginframework/provider/provider_plugin_framework.go → internal/providers/pluginfw/pluginfw.go

@@ -1,4 +1,7 @@
-package provider
+// Package pluginfw contains the changes specific to the plugin framework
+//
+// Note: This shouldn't depend on internal/providers/sdkv2 or internal/providers
+package pluginfw
 
 import (
 	"context"
@@ -12,6 +15,7 @@ import (
 	"github.com/databricks/databricks-sdk-go/config"
 	"github.com/databricks/terraform-provider-databricks/commands"
 	"github.com/databricks/terraform-provider-databricks/common"
+	providercommon "github.com/databricks/terraform-provider-databricks/internal/providers/common"
 
 	"github.com/hashicorp/terraform-plugin-framework/datasource"
 	"github.com/hashicorp/terraform-plugin-framework/diag"
@@ -23,10 +27,6 @@ import (
 	"github.com/hashicorp/terraform-plugin-log/tflog"
 )
 
-func GetProviderName() string {
-	return "databricks-tf-provider"
-}
-
 func GetDatabricksProviderPluginFramework() provider.Provider {
 	p := &DatabricksProviderPluginFramework{}
 	return p
@@ -50,7 +50,7 @@ func (p *DatabricksProviderPluginFramework) Schema(ctx context.Context, req prov
 }
 
 func (p *DatabricksProviderPluginFramework) Metadata(ctx context.Context, req provider.MetadataRequest, resp *provider.MetadataResponse) {
-	resp.TypeName = GetProviderName()
+	resp.TypeName = providercommon.ProviderName
 	resp.Version = common.Version()
 }
 

provider/provider_factory.go → internal/providers/providers.go

@@ -1,10 +1,14 @@
-package provider
+// Package providers contains the changes for both SDKv2 and Plugin Framework which are defined in their respective sub packages.
+//
+// Note: Top level files under providers package only contains the changes that depends on both internal/providers/sdkv2 and internal/providers/pluginfw packages
+package providers
 
 import (
 	"context"
 	"log"
 
-	pluginframeworkprovider "github.com/databricks/terraform-provider-databricks/internal/pluginframework/provider"
+	"github.com/databricks/terraform-provider-databricks/internal/providers/pluginfw"
+	"github.com/databricks/terraform-provider-databricks/internal/providers/sdkv2"
 	"github.com/hashicorp/terraform-plugin-framework/providerserver"
 	"github.com/hashicorp/terraform-plugin-go/tfprotov6"
 	"github.com/hashicorp/terraform-plugin-mux/tf5to6server"
@@ -20,7 +24,7 @@ import (
 // Protocol v6 providers to be served together. The function returns the multiplexed
 // ProviderServer, or an error if any part of the process fails.
 func GetProviderServer(ctx context.Context) (tfprotov6.ProviderServer, error) {
-	sdkPluginProvider := DatabricksProvider()
+	sdkPluginProvider := sdkv2.DatabricksProvider()
 
 	upgradedSdkPluginProvider, err := tf5to6server.UpgradeServer(
 		context.Background(),
@@ -30,7 +34,7 @@ func GetProviderServer(ctx context.Context) (tfprotov6.ProviderServer, error) {
 		log.Fatal(err)
 	}
 
-	pluginFrameworkProvider := pluginframeworkprovider.GetDatabricksProviderPluginFramework()
+	pluginFrameworkProvider := pluginfw.GetDatabricksProviderPluginFramework()
 
 	providers := []func() tfprotov6.ProviderServer{
 		func() tfprotov6.ProviderServer {

provider/provider_test.go → internal/providers/providers_test.go

@@ -1,4 +1,4 @@
-package provider
+package providers
 
 import (
 	"context"
@@ -12,6 +12,7 @@ import (
 	"time"
 
 	"github.com/databricks/terraform-provider-databricks/common"
+	"github.com/databricks/terraform-provider-databricks/internal/providers/sdkv2"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -179,7 +180,7 @@ func TestConfig_PatFromDatabricksCfg(t *testing.T) {
 	providerFixture{
 		// loading with DEFAULT profile in databrickscfs
 		env: map[string]string{
-			"HOME": "../common/testdata",
+			"HOME": testDataPath,
 		},
 		assertHost: "https://dbc-XXXXXXXX-YYYY.cloud.databricks.com",
 		assertAuth: "pat",
@@ -190,7 +191,7 @@ func TestConfig_PatFromDatabricksCfg_NohostProfile(t *testing.T) {
 	providerFixture{
 		// loading with nohost profile in databrickscfs
 		env: map[string]string{
-			"HOME":                      "../common/testdata",
+			"HOME":                      testDataPath,
 			"DATABRICKS_CONFIG_PROFILE": "nohost",
 		},
 		assertError: common.NoAuth +
@@ -203,7 +204,7 @@ func TestConfig_ConfigProfileAndToken(t *testing.T) {
 		env: map[string]string{
 			"DATABRICKS_TOKEN":          "x",
 			"DATABRICKS_CONFIG_PROFILE": "nohost",
-			"HOME":                      "../common/testdata",
+			"HOME":                      testDataPath,
 		},
 		assertError: common.NoAuth +
 			". Config: token=***, profile=nohost. Env: DATABRICKS_TOKEN, DATABRICKS_CONFIG_PROFILE",
@@ -215,7 +216,7 @@ func TestConfig_ConfigProfileAndPassword(t *testing.T) {
 		env: map[string]string{
 			"DATABRICKS_USERNAME":       "x",
 			"DATABRICKS_CONFIG_PROFILE": "nohost",
-			"HOME":                      "../common/testdata",
+			"HOME":                      testDataPath,
 		},
 		assertError: "validate: more than one authorization method configured: basic and pat. " +
 			"Config: token=***, username=x, profile=nohost. Env: DATABRICKS_USERNAME, DATABRICKS_CONFIG_PROFILE",
@@ -225,7 +226,7 @@ func TestConfig_ConfigProfileAndPassword(t *testing.T) {
 var azResourceID = "/subscriptions/a/resourceGroups/b/providers/Microsoft.Databricks/workspaces/c"
 
 func TestConfig_AzureCliHost(t *testing.T) {
-	p, _ := filepath.Abs("../common/testdata")
+	p, _ := filepath.Abs(testDataPath)
 	providerFixture{
 		// this test will skip ensureWorkspaceUrl
 		host:            "x",
@@ -243,7 +244,7 @@ func TestConfig_AzureCliHost(t *testing.T) {
 }
 
 func TestConfig_AzureCliHost_Fail(t *testing.T) {
-	p, _ := filepath.Abs("../common/testdata")
+	p, _ := filepath.Abs(testDataPath)
 	providerFixture{
 		azureResourceID: azResourceID,
 		env: map[string]string{
@@ -262,14 +263,14 @@ func TestConfig_AzureCliHost_AzNotInstalled(t *testing.T) {
 		azureResourceID: azResourceID,
 		env: map[string]string{
 			"PATH": "whatever",
-			"HOME": "../common/testdata",
+			"HOME": "../../common/testdata",
 		},
 		assertError: common.NoAuth,
 	}.apply(t)
 }
 
 func TestConfig_AzureCliHost_PatConflict(t *testing.T) {
-	p, _ := filepath.Abs("../common/testdata")
+	p, _ := filepath.Abs(testDataPath)
 	providerFixture{
 		azureResourceID: azResourceID,
 		token:           "x",
@@ -283,7 +284,7 @@ func TestConfig_AzureCliHost_PatConflict(t *testing.T) {
 }
 
 func TestConfig_AzureCliHostAndResourceID(t *testing.T) {
-	p, _ := filepath.Abs("../common/testdata")
+	p, _ := filepath.Abs(testDataPath)
 	providerFixture{
 		// omit request to management endpoint to get workspace properties
 		azureResourceID: azResourceID,
@@ -301,7 +302,7 @@ func TestConfig_AzureCliHostAndResourceID(t *testing.T) {
 }
 
 func TestConfig_AzureAndPasswordConflict(t *testing.T) {
-	p, _ := filepath.Abs("../common/testdata")
+	p, _ := filepath.Abs(testDataPath)
 	providerFixture{
 		host:            "x",
 		azureResourceID: azResourceID,
@@ -318,7 +319,7 @@ func TestConfig_AzureAndPasswordConflict(t *testing.T) {
 func TestConfig_CorruptConfig(t *testing.T) {
 	providerFixture{
 		env: map[string]string{
-			"HOME": "../common/testdata/corrupt",
+			"HOME": testDataPath + "/corrupt",
 		},
 		assertError: common.NoAuth,
 	}.apply(t)
@@ -390,7 +391,7 @@ type parseUserAgentTestCase struct {
 	name string
 	env  string
 	err  error
-	out  []userAgentExtra
+	out  []sdkv2.UserAgentExtra
 }
 
 func Test_ParseUserAgentExtra(t *testing.T) {
@@ -399,34 +400,34 @@ func Test_ParseUserAgentExtra(t *testing.T) {
 			name: "single product",
 			env:  "databricks-cli/0.1.2",
 			err:  nil,
-			out: []userAgentExtra{
-				{"databricks-cli", "0.1.2"},
+			out: []sdkv2.UserAgentExtra{
+				{Key: "databricks-cli", Value: "0.1.2"},
 			},
 		},
 		{
 			name: "multiple products",
 			env:  "databricks-cli/0.1.2 custom-thing/0.0.1",
 			err:  nil,
-			out: []userAgentExtra{
-				{"databricks-cli", "0.1.2"},
-				{"custom-thing", "0.0.1"},
+			out: []sdkv2.UserAgentExtra{
+				{Key: "databricks-cli", Value: "0.1.2"},
+				{Key: "custom-thing", Value: "0.0.1"},
 			},
 		},
 		{
 			name: "multiple products with many separators",
 			env:  "\ta/0.0.1\tb/0.0.2 \t c/0.0.3",
 			err:  nil,
-			out: []userAgentExtra{
-				{"a", "0.0.1"},
-				{"b", "0.0.2"},
-				{"c", "0.0.3"},
+			out: []sdkv2.UserAgentExtra{
+				{Key: "a", Value: "0.0.1"},
+				{Key: "b", Value: "0.0.2"},
+				{Key: "c", Value: "0.0.3"},
 			},
 		},
 		{
 			name: "empty string",
 			env:  "",
 			err:  nil,
-			out:  []userAgentExtra{},
+			out:  []sdkv2.UserAgentExtra{},
 		},
 		{
 			name: "product with comment",
@@ -450,7 +451,7 @@ func Test_ParseUserAgentExtra(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			out, err := parseUserAgentExtra(tc.env)
+			out, err := sdkv2.ParseUserAgentExtra(tc.env)
 
 			assert.Equal(t, tc.err, err)
 			assert.Equal(t, tc.out, out)