chore: Initialize the SDK client in fewer places (#2710)

Result of SNOW-935947. - the SDK default client was already setup earlier in the acceptance tests context; used in all valid places - added secondary client setup to acceptance tests context and used in valid places
Snowflake-Labs · Apr 16, 2024 · 382bfc1 · 382bfc1
1 parent ebc3bf8
commit 382bfc1
Show file tree

Hide file tree

Showing 19 changed files with 111 additions and 202 deletions.
diff --git a/pkg/acceptance/testing.go b/pkg/acceptance/testing.go
@@ -62,11 +62,22 @@ func init() {
 		log.Panicf("Cannot instantiate new client, err: %v", err)
 	}
 	atc.client = client
+
+	cfg, err := sdk.ProfileConfig(testprofiles.Secondary)
+	if err != nil {
+		log.Panicf("Config for the secondary client is needed to run acceptance tests, err: %v", err)
+	}
+	secondaryClient, err := sdk.NewClient(cfg)
+	if err != nil {
+		log.Panicf("Cannot instantiate new secondary client, err: %v", err)
+	}
+	atc.secondaryClient = secondaryClient
 }
 
 type acceptanceTestContext struct {
-	config *gosnowflake.Config
-	client *sdk.Client
+	config          *gosnowflake.Config
+	client          *sdk.Client
+	secondaryClient *sdk.Client
 }
 
 var TestAccProtoV6ProviderFactories = map[string]func() (tfprotov6.ProviderServer, error){
@@ -144,6 +155,11 @@ func Client(t *testing.T) *sdk.Client {
 	return atc.client
 }
 
+func SecondaryClient(t *testing.T) *sdk.Client {
+	t.Helper()
+	return atc.secondaryClient
+}
+
 func DefaultConfig(t *testing.T) *gosnowflake.Config {
 	t.Helper()
 	return atc.config

diff --git a/pkg/datasources/grants_acceptance_test.go b/pkg/datasources/grants_acceptance_test.go
@@ -8,7 +8,6 @@ import (
 
 	acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance"
 
-	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testprofiles"
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
 	"github.com/hashicorp/terraform-plugin-testing/config"
 	"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
@@ -373,18 +372,8 @@ func TestAcc_Grants_Of_Share(t *testing.T) {
 	getSecondaryAccountIdentifier := func(t *testing.T) *sdk.AccountIdentifier {
 		t.Helper()
 
-		client, err := sdk.NewDefaultClient()
-		if err != nil {
-			t.Fatal(err)
-		}
-		cfg, err := sdk.ProfileConfig(testprofiles.Secondary)
-		if err != nil {
-			t.Fatal(err)
-		}
-		secondaryClient, err := sdk.NewClient(cfg)
-		if err != nil {
-			t.Fatal(err)
-		}
+		client := acc.Client(t)
+		secondaryClient := acc.SecondaryClient(t)
 		ctx := context.Background()
 
 		replicationAccounts, err := client.ReplicationFunctions.ShowReplicationAccounts(ctx)
@@ -688,10 +677,7 @@ func checkAtLeastOneGrantPresentLimited() resource.TestCheckFunc {
 
 func getCurrentUser(t *testing.T) string {
 	t.Helper()
-	client, err := sdk.NewDefaultClient()
-	if err != nil {
-		t.Fatal(err)
-	}
+	client := acc.Client(t)
 	user, err := client.ContextFunctions.CurrentUser(context.Background())
 	if err != nil {
 		t.Fatal(err)

diff --git a/pkg/resources/database_acceptance_test.go b/pkg/resources/database_acceptance_test.go
@@ -11,7 +11,6 @@ import (
 
 	acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance"
 
-	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testprofiles"
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
 	"github.com/hashicorp/terraform-plugin-testing/config"
 	"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
@@ -198,8 +197,7 @@ func TestAcc_Database_DefaultDataRetentionTime(t *testing.T) {
 		return vars
 	}
 
-	client, err := sdk.NewDefaultClient()
-	require.NoError(t, err)
+	client := acc.Client(t)
 
 	resource.Test(t, resource.TestCase{
 		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
@@ -288,8 +286,7 @@ func TestAcc_Database_DefaultDataRetentionTime_SetOutsideOfTerraform(t *testing.
 		return vars
 	}
 
-	client, err := sdk.NewDefaultClient()
-	require.NoError(t, err)
+	client := acc.Client(t)
 
 	resource.Test(t, resource.TestCase{
 		ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
@@ -310,7 +307,7 @@ func TestAcc_Database_DefaultDataRetentionTime_SetOutsideOfTerraform(t *testing.
 			},
 			{
 				PreConfig: func() {
-					err = client.Databases.Alter(context.Background(), id, &sdk.AlterDatabaseOptions{
+					err := client.Databases.Alter(context.Background(), id, &sdk.AlterDatabaseOptions{
 						Set: &sdk.DatabaseSet{
 							DataRetentionTimeInDays: sdk.Int(20),
 						},
@@ -382,23 +379,17 @@ resource "snowflake_database" "db" {
 func dropDatabaseOutsideTerraform(t *testing.T, id string) {
 	t.Helper()
 
-	client, err := sdk.NewDefaultClient()
-	require.NoError(t, err)
+	client := acc.Client(t)
 	ctx := context.Background()
 
-	err = client.Databases.Drop(ctx, sdk.NewAccountObjectIdentifier(id), &sdk.DropDatabaseOptions{})
+	err := client.Databases.Drop(ctx, sdk.NewAccountObjectIdentifier(id), &sdk.DropDatabaseOptions{})
 	require.NoError(t, err)
 }
 
 func getSecondaryAccount(t *testing.T) string {
 	t.Helper()
 
-	secondaryConfig, err := sdk.ProfileConfig(testprofiles.Secondary)
-	require.NoError(t, err)
-
-	secondaryClient, err := sdk.NewClient(secondaryConfig)
-	require.NoError(t, err)
-
+	secondaryClient := acc.SecondaryClient(t)
 	ctx := context.Background()
 
 	account, err := secondaryClient.ContextFunctions.CurrentAccount(ctx)
@@ -410,11 +401,10 @@ func getSecondaryAccount(t *testing.T) string {
 func testAccCheckDatabaseExistence(t *testing.T, id string, shouldExist bool) func(state *terraform.State) error {
 	t.Helper()
 	return func(state *terraform.State) error {
-		client, err := sdk.NewDefaultClient()
-		require.NoError(t, err)
+		client := acc.Client(t)
 		ctx := context.Background()
 
-		_, err = client.Databases.ShowByID(ctx, sdk.NewAccountObjectIdentifier(id))
+		_, err := client.Databases.ShowByID(ctx, sdk.NewAccountObjectIdentifier(id))
 		if shouldExist {
 			if err != nil {
 				return fmt.Errorf("error while retrieving database %s, err = %w", id, err)
@@ -431,10 +421,7 @@ func testAccCheckDatabaseExistence(t *testing.T, id string, shouldExist bool) fu
 func testAccCheckIfDatabaseIsReplicated(t *testing.T, id string) func(state *terraform.State) error {
 	t.Helper()
 	return func(state *terraform.State) error {
-		client, err := sdk.NewDefaultClient()
-		if err != nil {
-			return err
-		}
+		client := acc.Client(t)
 
 		ctx := context.Background()
 		replicationDatabases, err := client.ReplicationFunctions.ShowReplicationDatabases(ctx, nil)
@@ -492,10 +479,7 @@ func checkAccountAndDatabaseDataRetentionTime(id sdk.AccountObjectIdentifier, ex
 
 func createDatabaseOutsideTerraform(t *testing.T, name string) func() {
 	t.Helper()
-	client, err := sdk.NewDefaultClient()
-	if err != nil {
-		t.Fatal(err)
-	}
+	client := acc.Client(t)
 	ctx := context.Background()
 
 	if err := client.Databases.Create(ctx, sdk.NewAccountObjectIdentifier(name), new(sdk.CreateDatabaseOptions)); err != nil {

diff --git a/pkg/resources/dynamic_table_acceptance_test.go b/pkg/resources/dynamic_table_acceptance_test.go
@@ -446,10 +446,7 @@ func testAccCheckDynamicTableDestroy(s *terraform.State) error {
 // TODO [SNOW-926148]: currently this dynamic table is not cleaned in the test; it is removed when the whole database is removed - this currently happens in a sweeper
 func createDynamicTableOutsideTerraform(t *testing.T, schemaName string, dynamicTableName string, query string) {
 	t.Helper()
-	client, err := sdk.NewDefaultClient()
-	if err != nil {
-		t.Fatal(err)
-	}
+	client := acc.Client(t)
 	ctx := context.Background()
 
 	dynamicTableId := sdk.NewSchemaObjectIdentifier(acc.TestDatabaseName, schemaName, dynamicTableName)

diff --git a/pkg/resources/external_table_acceptance_test.go b/pkg/resources/external_table_acceptance_test.go
@@ -73,7 +73,7 @@ func TestAcc_ExternalTable_basic(t *testing.T) {
 			},
 			{
 				PreConfig: func() {
-					publishExternalTablesTestData(sdk.NewSchemaObjectIdentifier(acc.TestDatabaseName, acc.TestSchemaName, name), data)
+					publishExternalTablesTestData(t, sdk.NewSchemaObjectIdentifier(acc.TestDatabaseName, acc.TestSchemaName, name), data)
 				},
 				ConfigDirectory: config.TestStepDirectory(),
 				ConfigVariables: configVariables,
@@ -344,14 +344,12 @@ func externalTableContainsData(name string, contains func(rows []map[string]*any
 	}
 }
 
-func publishExternalTablesTestData(stageName sdk.SchemaObjectIdentifier, data []byte) {
-	client, err := sdk.NewDefaultClient()
-	if err != nil {
-		log.Fatal(err)
-	}
+func publishExternalTablesTestData(t *testing.T, stageName sdk.SchemaObjectIdentifier, data []byte) {
+	t.Helper()
+	client := acc.Client(t)
 	ctx := context.Background()
 
-	_, err = client.ExecForTests(ctx, fmt.Sprintf(`copy into @%s/external_tables_test_data/test_data from (select parse_json('%s')) overwrite = true`, stageName.FullyQualifiedName(), string(data)))
+	_, err := client.ExecForTests(ctx, fmt.Sprintf(`copy into @%s/external_tables_test_data/test_data from (select parse_json('%s')) overwrite = true`, stageName.FullyQualifiedName(), string(data)))
 	if err != nil {
 		log.Fatal(err)
 	}

diff --git a/pkg/resources/function_acceptance_test.go b/pkg/resources/function_acceptance_test.go
@@ -2,7 +2,6 @@ package resources_test
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"strings"
 	"testing"
@@ -44,7 +43,7 @@ func testAccFunction(t *testing.T, configDirectory string) {
 		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
 			tfversion.RequireAbove(tfversion.Version1_5_0),
 		},
-		CheckDestroy: testAccCheckFunctionDestroy,
+		CheckDestroy: testAccCheckFunctionDestroy(t),
 		Steps: []resource.TestStep{
 			{
 				ConfigDirectory: acc.ConfigurationDirectory(configDirectory),
@@ -131,7 +130,7 @@ func TestAcc_Function_complex(t *testing.T) {
 		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
 			tfversion.RequireAbove(tfversion.Version1_5_0),
 		},
-		CheckDestroy: testAccCheckFunctionDestroy,
+		CheckDestroy: testAccCheckFunctionDestroy(t),
 		Steps: []resource.TestStep{
 			{
 				ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Function/complex"),
@@ -194,7 +193,7 @@ func TestAcc_Function_migrateFromVersion085(t *testing.T) {
 		TerraformVersionChecks: []tfversion.TerraformVersionCheck{
 			tfversion.RequireAbove(tfversion.Version1_5_0),
 		},
-		CheckDestroy: testAccCheckFunctionDestroy,
+		CheckDestroy: testAccCheckFunctionDestroy(t),
 
 		// Using the string config because of the validation in teststep_validate.go:
 		// teststep.Config.HasConfigurationFiles() returns true both for ConfigFile and ConfigDirectory.
@@ -248,22 +247,21 @@ resource "snowflake_function" "f" {
 `, database, schema, name)
 }
 
-func testAccCheckFunctionDestroy(s *terraform.State) error {
-	client, err := sdk.NewDefaultClient()
-	if err != nil {
-		return errors.New("client could not be instantiated")
-	}
-
-	for _, rs := range s.RootModule().Resources {
-		if rs.Type != "snowflake_function" {
-			continue
-		}
-		ctx := context.Background()
-		id := sdk.NewSchemaObjectIdentifier(rs.Primary.Attributes["database"], rs.Primary.Attributes["schema"], rs.Primary.Attributes["name"])
-		function, err := client.Functions.ShowByID(ctx, id)
-		if err == nil {
-			return fmt.Errorf("function %v still exists", function.Name)
+func testAccCheckFunctionDestroy(t *testing.T) func(s *terraform.State) error {
+	t.Helper()
+	client := acc.Client(t)
+	return func(s *terraform.State) error {
+		for _, rs := range s.RootModule().Resources {
+			if rs.Type != "snowflake_function" {
+				continue
+			}
+			ctx := context.Background()
+			id := sdk.NewSchemaObjectIdentifier(rs.Primary.Attributes["database"], rs.Primary.Attributes["schema"], rs.Primary.Attributes["name"])
+			function, err := client.Functions.ShowByID(ctx, id)
+			if err == nil {
+				return fmt.Errorf("function %v still exists", function.Name)
+			}
 		}
+		return nil
 	}
-	return nil
 }
diff --git a/pkg/resources/grant_ownership_acceptance_test.go b/pkg/resources/grant_ownership_acceptance_test.go
@@ -1078,14 +1078,12 @@ func TestAcc_GrantOwnership_OnAllTasks(t *testing.T) {
 
 func createDatabaseWithRoleAsOwner(t *testing.T, roleName string, databaseName string) func() {
 	t.Helper()
-	client, err := sdk.NewDefaultClient()
-	assert.NoError(t, err)
-
+	client := acc.Client(t)
 	ctx := context.Background()
 	databaseId := sdk.NewAccountObjectIdentifier(databaseName)
 	assert.NoError(t, client.Databases.Create(ctx, databaseId, &sdk.CreateDatabaseOptions{}))
 
-	err = client.Grants.GrantOwnership(
+	err := client.Grants.GrantOwnership(
 		ctx,
 		sdk.OwnershipGrantOn{
 			Object: &sdk.Object{
@@ -1108,11 +1106,9 @@ func createDatabaseWithRoleAsOwner(t *testing.T, roleName string, databaseName s
 func moveResourceOwnershipToAccountRole(t *testing.T, objectType sdk.ObjectType, objectName sdk.ObjectIdentifier, accountRoleName sdk.AccountObjectIdentifier) {
 	t.Helper()
 
-	client, err := sdk.NewDefaultClient()
-	assert.NoError(t, err)
-
+	client := acc.Client(t)
 	ctx := context.Background()
-	err = client.Grants.GrantOwnership(
+	err := client.Grants.GrantOwnership(
 		ctx,
 		sdk.OwnershipGrantOn{
 			Object: &sdk.Object{
@@ -1158,9 +1154,7 @@ func checkResourceOwnershipIsGranted(opts *sdk.ShowGrantOptions, grantOn sdk.Obj
 
 func createAccountRole(t *testing.T, name string) func() {
 	t.Helper()
-	client, err := sdk.NewDefaultClient()
-	assert.NoError(t, err)
-
+	client := acc.Client(t)
 	ctx := context.Background()
 	roleId := sdk.NewAccountObjectIdentifier(name)
 	assert.NoError(t, client.Roles.Create(ctx, sdk.NewCreateRoleRequest(roleId)))
@@ -1172,8 +1166,7 @@ func createAccountRole(t *testing.T, name string) func() {
 
 func createDatabase(t *testing.T, name string) func() {
 	t.Helper()
-	client, err := sdk.NewDefaultClient()
-	assert.NoError(t, err)
+	client := acc.Client(t)
 
 	ctx := context.Background()
 	roleId := sdk.NewAccountObjectIdentifier(name)
@@ -1186,8 +1179,7 @@ func createDatabase(t *testing.T, name string) func() {
 
 func grantOwnershipToTheCurrentRole(t *testing.T, on sdk.OwnershipGrantOn) {
 	t.Helper()
-	client, err := sdk.NewDefaultClient()
-	assert.NoError(t, err)
+	client := acc.Client(t)
 
 	ctx := context.Background()
 	currentRole, err := client.ContextFunctions.CurrentRole(ctx)
@@ -1206,8 +1198,7 @@ func grantOwnershipToTheCurrentRole(t *testing.T, on sdk.OwnershipGrantOn) {
 
 func getCurrentUser(t *testing.T) string {
 	t.Helper()
-	client, err := sdk.NewDefaultClient()
-	assert.NoError(t, err)
+	client := acc.Client(t)
 	currentUser, err := client.ContextFunctions.CurrentUser(context.Background())
 	assert.NoError(t, err)
 	return currentUser