From e3f6a154fc92b1bfd47fb68ae5ec6478aa971437 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Fri, 12 Apr 2024 10:23:21 +0200 Subject: [PATCH] fix: Fix several small issues (#2697) Fixed a few issues (from GH and reported internally): - Do not swallow error in access token setup (References: #2678) - Fix import example after v0.85.0 changes (References: #993) - Fix alter allowed values for tags (reported internally) - Suppress diff for quoted on_table/on_view in stream resource (References: #2672) - Test primary key creation (References: #2674 #2629) - Fix v0.85.0 state migrator for external functions (References: #2694) --- docs/resources/external_function.md | 4 +- docs/resources/function.md | 4 +- docs/resources/procedure.md | 4 +- .../snowflake_external_function/import.sh | 4 +- .../resources/snowflake_function/import.sh | 4 +- .../resources/snowflake_procedure/import.sh | 4 +- pkg/provider/provider.go | 2 +- pkg/resources/common.go | 18 +++ pkg/resources/common_test.go | 49 ++++++ .../external_function_acceptance_test.go | 103 ++++++++++++- .../external_function_state_upgraders.go | 2 +- .../masking_policy_acceptance_test.go | 2 +- pkg/resources/stream.go | 22 +-- pkg/resources/stream_acceptance_test.go | 142 ++++++++++++++++++ pkg/resources/table_constraint.go | 3 +- .../table_constraint_acceptance_test.go | 86 +++++++++++ pkg/resources/tag.go | 23 ++- pkg/resources/tag_acceptance_test.go | 36 +++-- .../testdata/TestAcc_Tag/basic/test.tf | 2 +- .../testdata/TestAcc_Tag/basic/variables.tf | 4 + 20 files changed, 467 insertions(+), 51 deletions(-) create mode 100644 pkg/resources/common_test.go diff --git a/docs/resources/external_function.md b/docs/resources/external_function.md index 73cce06a0c..e912577aed 100644 --- a/docs/resources/external_function.md +++ b/docs/resources/external_function.md @@ -84,6 +84,6 @@ Required: Import is supported using the following syntax: ```shell -# format is database name | schema name | external function name | -terraform import snowflake_external_function.example 'dbName|schemaName|externalFunctionName|varchar-varchar-varchar' +# format is ..() +terraform import snowflake_external_function.example 'dbName.schemaName.externalFunctionName(varchar, varchar, varchar)' ``` diff --git a/docs/resources/function.md b/docs/resources/function.md index 82c0945881..a407dabb49 100644 --- a/docs/resources/function.md +++ b/docs/resources/function.md @@ -129,6 +129,6 @@ Required: Import is supported using the following syntax: ```shell -# format is database name | schema name | function name | -terraform import snowflake_function.example 'dbName|schemaName|functionName|varchar-varchar-varchar' +# format is ..() +terraform import snowflake_function.example 'dbName.schemaName.functionName(varchar, varchar, varchar)' ``` diff --git a/docs/resources/procedure.md b/docs/resources/procedure.md index 7be3453407..ad04ae8fce 100644 --- a/docs/resources/procedure.md +++ b/docs/resources/procedure.md @@ -90,6 +90,6 @@ Required: Import is supported using the following syntax: ```shell -# format is database name | schema name | stored procedure name | -terraform import snowflake_procedure.example 'dbName|schemaName|procedureName|varchar-varchar-varchar' +# format is ..() +terraform import snowflake_procedure.example 'dbName.schemaName.procedureName(varchar, varchar, varchar)' ``` diff --git a/examples/resources/snowflake_external_function/import.sh b/examples/resources/snowflake_external_function/import.sh index 5210458738..6ff0345c55 100644 --- a/examples/resources/snowflake_external_function/import.sh +++ b/examples/resources/snowflake_external_function/import.sh @@ -1,2 +1,2 @@ -# format is database name | schema name | external function name | -terraform import snowflake_external_function.example 'dbName|schemaName|externalFunctionName|varchar-varchar-varchar' +# format is ..() +terraform import snowflake_external_function.example 'dbName.schemaName.externalFunctionName(varchar, varchar, varchar)' diff --git a/examples/resources/snowflake_function/import.sh b/examples/resources/snowflake_function/import.sh index 0e35583e51..62c2ca5681 100644 --- a/examples/resources/snowflake_function/import.sh +++ b/examples/resources/snowflake_function/import.sh @@ -1,2 +1,2 @@ -# format is database name | schema name | function name | -terraform import snowflake_function.example 'dbName|schemaName|functionName|varchar-varchar-varchar' +# format is ..() +terraform import snowflake_function.example 'dbName.schemaName.functionName(varchar, varchar, varchar)' diff --git a/examples/resources/snowflake_procedure/import.sh b/examples/resources/snowflake_procedure/import.sh index 9d0a343fe9..9ba6623768 100644 --- a/examples/resources/snowflake_procedure/import.sh +++ b/examples/resources/snowflake_procedure/import.sh @@ -1,2 +1,2 @@ -# format is database name | schema name | stored procedure name | -terraform import snowflake_procedure.example 'dbName|schemaName|procedureName|varchar-varchar-varchar' +# format is ..() +terraform import snowflake_procedure.example 'dbName.schemaName.procedureName(varchar, varchar, varchar)' diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 6a0f8042b8..462bf5efa3 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -720,7 +720,7 @@ func ConfigureProvider(s *schema.ResourceData) (interface{}, error) { redirectURI := tokenAccessor["redirect_uri"].(string) accessToken, err := GetAccessTokenWithRefreshToken(tokenEndpoint, clientID, clientSecret, refreshToken, redirectURI) if err != nil { - return nil, fmt.Errorf("could not retrieve access token from refresh token") + return nil, fmt.Errorf("could not retrieve access token from refresh token, err = %w", err) } config.Token = accessToken config.Authenticator = gosnowflake.AuthTypeOAuth diff --git a/pkg/resources/common.go b/pkg/resources/common.go index 909306f301..34b7307f26 100644 --- a/pkg/resources/common.go +++ b/pkg/resources/common.go @@ -3,6 +3,7 @@ package resources import ( "strings" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/helpers" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) @@ -23,3 +24,20 @@ func DiffSuppressStatement(_, old, new string, _ *schema.ResourceData) bool { func normalizeQuery(str string) string { return strings.TrimSpace(space.ReplaceAllString(str, " ")) } + +// TODO [SNOW-999049]: address during identifiers rework +func suppressIdentifierQuoting(k, oldValue, newValue string, d *schema.ResourceData) bool { + if oldValue == "" || newValue == "" { + return false + } else { + oldId, err := helpers.DecodeSnowflakeParameterID(oldValue) + if err != nil { + return false + } + newId, err := helpers.DecodeSnowflakeParameterID(newValue) + if err != nil { + return false + } + return oldId.FullyQualifiedName() == newId.FullyQualifiedName() + } +} diff --git a/pkg/resources/common_test.go b/pkg/resources/common_test.go new file mode 100644 index 0000000000..1212114e11 --- /dev/null +++ b/pkg/resources/common_test.go @@ -0,0 +1,49 @@ +package resources + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func Test_suppressIdentifierQuoting(t *testing.T) { + firstId := "a.b.c" + firstIdQuoted := "\"a\".b.\"c\"" + secondId := "d.e.f" + incorrectId := "a.b.c.d.e.f" + + t.Run("old identifier with too many parts", func(t *testing.T) { + result := suppressIdentifierQuoting("", incorrectId, firstId, nil) + require.False(t, result) + }) + + t.Run("new identifier with too many parts", func(t *testing.T) { + result := suppressIdentifierQuoting("", firstId, incorrectId, nil) + require.False(t, result) + }) + + t.Run("old identifier empty", func(t *testing.T) { + result := suppressIdentifierQuoting("", "", firstId, nil) + require.False(t, result) + }) + + t.Run("new identifier empty", func(t *testing.T) { + result := suppressIdentifierQuoting("", firstId, "", nil) + require.False(t, result) + }) + + t.Run("identifiers the same", func(t *testing.T) { + result := suppressIdentifierQuoting("", firstId, firstId, nil) + require.True(t, result) + }) + + t.Run("identifiers the same (but different quoting)", func(t *testing.T) { + result := suppressIdentifierQuoting("", firstId, firstIdQuoted, nil) + require.True(t, result) + }) + + t.Run("identifiers different", func(t *testing.T) { + result := suppressIdentifierQuoting("", firstId, secondId, nil) + require.False(t, result) + }) +} diff --git a/pkg/resources/external_function_acceptance_test.go b/pkg/resources/external_function_acceptance_test.go index c8eafbb5b0..dcd2783781 100644 --- a/pkg/resources/external_function_acceptance_test.go +++ b/pkg/resources/external_function_acceptance_test.go @@ -273,6 +273,97 @@ func TestAcc_ExternalFunction_migrateFromVersion085(t *testing.T) { }) } +func TestAcc_ExternalFunction_migrateFromVersion085_issue2694_previousValuePresent(t *testing.T) { + name := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + resourceName := "snowflake_external_function.f" + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + CheckDestroy: nil, + + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.85.0", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: externalFunctionConfigWithReturnNullAllowed(acc.TestDatabaseName, acc.TestSchemaName, name, sdk.Bool(true)), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(resourceName, "return_null_allowed", "true"), + ), + ExpectNonEmptyPlan: true, + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: externalFunctionConfig(acc.TestDatabaseName, acc.TestSchemaName, name), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{plancheck.ExpectEmptyPlan()}, + }, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(resourceName, "return_null_allowed", "true"), + ), + }, + }, + }) +} + +func TestAcc_ExternalFunction_migrateFromVersion085_issue2694_previousValueRemoved(t *testing.T) { + name := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + resourceName := "snowflake_external_function.f" + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + CheckDestroy: nil, + + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.85.0", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: externalFunctionConfigWithReturnNullAllowed(acc.TestDatabaseName, acc.TestSchemaName, name, sdk.Bool(true)), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(resourceName, "return_null_allowed", "true"), + ), + ExpectNonEmptyPlan: true, + }, + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.85.0", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: externalFunctionConfig(acc.TestDatabaseName, acc.TestSchemaName, name), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckNoResourceAttr(resourceName, "return_null_allowed"), + ), + ExpectNonEmptyPlan: true, + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: externalFunctionConfig(acc.TestDatabaseName, acc.TestSchemaName, name), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{plancheck.ExpectEmptyPlan()}, + }, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(resourceName, "return_null_allowed", "true"), + ), + }, + }, + }) +} + // Proves issue https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2528. // The problem originated from ShowById without IN clause. There was no IN clause in the docs at the time. // It was raised with the appropriate team in Snowflake. @@ -301,6 +392,15 @@ func TestAcc_ExternalFunction_issue2528(t *testing.T) { } func externalFunctionConfig(database string, schema string, name string) string { + return externalFunctionConfigWithReturnNullAllowed(database, schema, name, nil) +} + +func externalFunctionConfigWithReturnNullAllowed(database string, schema string, name string, returnNullAllowed *bool) string { + returnNullAllowedText := "" + if returnNullAllowed != nil { + returnNullAllowedText = fmt.Sprintf("return_null_allowed = \"%t\"", *returnNullAllowed) + } + return fmt.Sprintf(` resource "snowflake_api_integration" "test_api_int" { name = "%[3]s" @@ -326,9 +426,10 @@ resource "snowflake_external_function" "f" { return_behavior = "IMMUTABLE" api_integration = snowflake_api_integration.test_api_int.name url_of_proxy_and_resource = "https://123456.execute-api.us-west-2.amazonaws.com/prod/test_func" + %[4]s } -`, database, schema, name) +`, database, schema, name, returnNullAllowedText) } func externalFunctionConfigIssue2528(database string, schema string, name string, schema2 string) string { diff --git a/pkg/resources/external_function_state_upgraders.go b/pkg/resources/external_function_state_upgraders.go index b8c9abf723..55d2b95868 100644 --- a/pkg/resources/external_function_state_upgraders.go +++ b/pkg/resources/external_function_state_upgraders.go @@ -69,7 +69,7 @@ func v085ExternalFunctionStateUpgrader(ctx context.Context, rawState map[string] rawState["database"] = strings.Trim(oldDatabase, "\"") rawState["schema"] = strings.Trim(oldSchema, "\"") - if old, isPresent := rawState["return_null_allowed"]; !isPresent || old == nil || old.(string) == "" { + if old, isPresent := rawState["return_null_allowed"]; !isPresent || old == nil { rawState["return_null_allowed"] = "true" } diff --git a/pkg/resources/masking_policy_acceptance_test.go b/pkg/resources/masking_policy_acceptance_test.go index 20bb5e3a1b..2a3b5650e8 100644 --- a/pkg/resources/masking_policy_acceptance_test.go +++ b/pkg/resources/masking_policy_acceptance_test.go @@ -2,7 +2,6 @@ package resources_test import ( "fmt" - "github.com/hashicorp/terraform-plugin-testing/plancheck" "strings" "testing" @@ -10,6 +9,7 @@ import ( "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/plancheck" "github.com/hashicorp/terraform-plugin-testing/tfversion" ) diff --git a/pkg/resources/stream.go b/pkg/resources/stream.go index 50c2a48ba3..da8b2ae833 100644 --- a/pkg/resources/stream.go +++ b/pkg/resources/stream.go @@ -38,18 +38,20 @@ var streamSchema = map[string]*schema.Schema{ Description: "Specifies a comment for the stream.", }, "on_table": { - Type: schema.TypeString, - Optional: true, - ForceNew: true, - Description: "Specifies an identifier for the table the stream will monitor.", - ExactlyOneOf: []string{"on_table", "on_view", "on_stage"}, + Type: schema.TypeString, + Optional: true, + ForceNew: true, + Description: "Specifies an identifier for the table the stream will monitor.", + ExactlyOneOf: []string{"on_table", "on_view", "on_stage"}, + DiffSuppressFunc: suppressIdentifierQuoting, }, "on_view": { - Type: schema.TypeString, - Optional: true, - ForceNew: true, - Description: "Specifies an identifier for the view the stream will monitor.", - ExactlyOneOf: []string{"on_table", "on_view", "on_stage"}, + Type: schema.TypeString, + Optional: true, + ForceNew: true, + Description: "Specifies an identifier for the view the stream will monitor.", + ExactlyOneOf: []string{"on_table", "on_view", "on_stage"}, + DiffSuppressFunc: suppressIdentifierQuoting, }, "on_stage": { Type: schema.TypeString, diff --git a/pkg/resources/stream_acceptance_test.go b/pkg/resources/stream_acceptance_test.go index 630b4ae7d8..a979835967 100644 --- a/pkg/resources/stream_acceptance_test.go +++ b/pkg/resources/stream_acceptance_test.go @@ -11,6 +11,7 @@ import ( "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance/testenvs" "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/plancheck" "github.com/hashicorp/terraform-plugin-testing/tfversion" ) @@ -59,6 +60,81 @@ func TestAcc_StreamCreateOnStage(t *testing.T) { }) } +// proves issue https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2672 +func TestAcc_Stream_OnTable(t *testing.T) { + tableName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + tableName2 := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + name := acctest.RandStringFromCharSet(10, acctest.CharSetAlpha) + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + PreCheck: func() { acc.TestAccPreCheck(t) }, + CheckDestroy: nil, + Steps: []resource.TestStep{ + { + Config: streamConfigOnTable(acc.TestDatabaseName, acc.TestSchemaName, tableName, name), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "name", name), + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "database", acc.TestDatabaseName), + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "schema", acc.TestSchemaName), + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "on_table", fmt.Sprintf("\"%s\".\"%s\".%s", acc.TestDatabaseName, acc.TestSchemaName, tableName)), + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "comment", "Terraform acceptance test"), + ), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PostApplyPreRefresh: []plancheck.PlanCheck{plancheck.ExpectEmptyPlan()}, + }, + }, + { + Config: streamConfigOnTable(acc.TestDatabaseName, acc.TestSchemaName, tableName2, name), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "name", name), + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "database", acc.TestDatabaseName), + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "schema", acc.TestSchemaName), + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "on_table", fmt.Sprintf("\"%s\".\"%s\".%s", acc.TestDatabaseName, acc.TestSchemaName, tableName2)), + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "comment", "Terraform acceptance test"), + ), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PostApplyPreRefresh: []plancheck.PlanCheck{plancheck.ExpectEmptyPlan()}, + }, + }, + }, + }) +} + +// proves issue https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2672 +func TestAcc_Stream_OnView(t *testing.T) { + tableName := acctest.RandStringFromCharSet(10, acctest.CharSetAlpha) + viewName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + name := acctest.RandStringFromCharSet(10, acctest.CharSetAlpha) + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + PreCheck: func() { acc.TestAccPreCheck(t) }, + CheckDestroy: nil, + Steps: []resource.TestStep{ + { + Config: streamConfigOnView(acc.TestDatabaseName, acc.TestSchemaName, tableName, viewName, name), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "name", name), + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "database", acc.TestDatabaseName), + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "schema", acc.TestSchemaName), + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "on_view", fmt.Sprintf("\"%s\".\"%s\".%s", acc.TestDatabaseName, acc.TestSchemaName, viewName)), + resource.TestCheckResourceAttr("snowflake_stream.test_stream", "comment", "Terraform acceptance test"), + ), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PostApplyPreRefresh: []plancheck.PlanCheck{plancheck.ExpectEmptyPlan()}, + }, + }, + }, + }) +} + func TestAcc_Stream(t *testing.T) { // Current error is User: is not authorized to perform: sts:AssumeRole on resource: duration 1.162414333s args {}] () t.Skip("Skipping TestAcc_Stream") @@ -160,6 +236,72 @@ func TestAcc_Stream(t *testing.T) { }) } +func streamConfigOnTable(databaseName string, schemaName string, tableName string, name string) string { + return fmt.Sprintf(` +resource "snowflake_table" "test_stream_on_table" { + database = "%[1]s" + schema = "%[2]s" + name = "%[3]s" + comment = "Terraform acceptance test" + change_tracking = true + + column { + name = "column1" + type = "VARIANT" + } + column { + name = "column2" + type = "VARCHAR" + } +} + +resource "snowflake_stream" "test_stream" { + database = "%[1]s" + schema = "%[2]s" + name = "%[4]s" + comment = "Terraform acceptance test" + on_table = "\"%[1]s\".\"%[2]s\".\"${snowflake_table.test_stream_on_table.name}\"" +} +`, databaseName, schemaName, tableName, name) +} + +func streamConfigOnView(databaseName string, schemaName string, tableName string, viewName string, name string) string { + return fmt.Sprintf(` +resource "snowflake_table" "test" { + database = "%[1]s" + schema = "%[2]s" + name = "%[3]s" + comment = "Terraform acceptance test" + change_tracking = true + + column { + name = "column1" + type = "VARIANT" + } + column { + name = "column2" + type = "VARCHAR" + } +} + +resource "snowflake_view" "test" { + database = "%[1]s" + schema = "%[2]s" + name = "%[4]s" + + statement = "select * from \"${snowflake_table.test.name}\"" +} + +resource "snowflake_stream" "test_stream" { + database = "%[1]s" + schema = "%[2]s" + name = "%[5]s" + comment = "Terraform acceptance test" + on_view = "\"%[1]s\".\"%[2]s\".\"${snowflake_view.test.name}\"" +} +`, databaseName, schemaName, tableName, viewName, name) +} + func streamConfig(name string, appendOnly bool) string { appendOnlyConfig := "" if appendOnly { diff --git a/pkg/resources/table_constraint.go b/pkg/resources/table_constraint.go index bccb968ad7..eff442d0bc 100644 --- a/pkg/resources/table_constraint.go +++ b/pkg/resources/table_constraint.go @@ -5,9 +5,8 @@ import ( "fmt" "strings" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/helpers" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/snowflake" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" diff --git a/pkg/resources/table_constraint_acceptance_test.go b/pkg/resources/table_constraint_acceptance_test.go index dc3499108c..29324b03e1 100644 --- a/pkg/resources/table_constraint_acceptance_test.go +++ b/pkg/resources/table_constraint_acceptance_test.go @@ -1,6 +1,7 @@ package resources_test import ( + "context" "fmt" "regexp" "strings" @@ -8,8 +9,11 @@ import ( acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/terraform" "github.com/hashicorp/terraform-plugin-testing/tfversion" ) @@ -82,6 +86,88 @@ resource "snowflake_table_constraint" "fk" { `, n, databaseName, schemaName, n, databaseName, schemaName, n) } +// proves issue https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2674 +// It is connected with https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2629. +// Provider defaults will be reworked during resources redesign. +func TestAcc_TableConstraint_pk(t *testing.T) { + tableName := acctest.RandStringFromCharSet(10, acctest.CharSetAlpha) + constraintName := fmt.Sprintf("%s_pk", tableName) + + resource.Test(t, resource.TestCase{ + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + CheckDestroy: nil, + Steps: []resource.TestStep{ + { + Config: tableConstraintPKConfig(acc.TestDatabaseName, acc.TestSchemaName, tableName, constraintName), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_table_constraint.pk", "type", "PRIMARY KEY"), + resource.TestCheckResourceAttr("snowflake_table_constraint.pk", "comment", "hello pk"), + checkPrimaryKeyExists(sdk.NewSchemaObjectIdentifier(acc.TestDatabaseName, acc.TestSchemaName, tableName), constraintName), + ), + }, + }, + }) +} + +func tableConstraintPKConfig(databaseName string, schemaName string, tableName string, constraintName string) string { + return fmt.Sprintf(` +resource "snowflake_table" "t" { + database = "%[1]s" + schema = "%[2]s" + name = "%[3]s" + + column { + name = "col1" + type = "NUMBER(38,0)" + } +} + +resource "snowflake_table_constraint" "pk" { + name = "%[4]s" + type = "PRIMARY KEY" + table_id = snowflake_table.t.qualified_name + columns = ["col1"] + enable = false + deferrable = false + comment = "hello pk" +} +`, databaseName, schemaName, tableName, constraintName) +} + +type PrimaryKeys struct { + ConstraintName string `db:"constraint_name"` +} + +func checkPrimaryKeyExists(tableId sdk.SchemaObjectIdentifier, constraintName string) func(s *terraform.State) error { + return func(s *terraform.State) error { + client := acc.TestAccProvider.Meta().(*provider.Context).Client + ctx := context.Background() + + var keys []PrimaryKeys + err := client.QueryForTests(ctx, &keys, fmt.Sprintf("show primary keys in %s", tableId.FullyQualifiedName())) + if err != nil { + return err + } + + var found bool + for _, pk := range keys { + if pk.ConstraintName == strings.ToUpper(constraintName) { + found = true + } + } + + if !found { + return fmt.Errorf("unable to find primary key %s on table %s, found: %v", constraintName, tableId.FullyQualifiedName(), keys) + } + + return nil + } +} + func TestAcc_TableConstraint_unique(t *testing.T) { name := acctest.RandStringFromCharSet(10, acctest.CharSetAlpha) diff --git a/pkg/resources/tag.go b/pkg/resources/tag.go index ac82ceac0b..5029a5d3d8 100644 --- a/pkg/resources/tag.go +++ b/pkg/resources/tag.go @@ -92,7 +92,7 @@ func Tag() *schema.Resource { } } -func CreateContextTag(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { +func CreateContextTag(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { client := meta.(*provider.Context).Client name := d.Get("name").(string) schema := d.Get("schema").(string) @@ -104,7 +104,7 @@ func CreateContextTag(ctx context.Context, d *schema.ResourceData, meta interfac request.WithComment(sdk.String(v.(string))) } if v, ok := d.GetOk("allowed_values"); ok { - request.WithAllowedValues(expandStringList(v.([]interface{}))) + request.WithAllowedValues(expandStringListAllowEmpty(v.([]any))) } if err := client.Tags.Create(ctx, request); err != nil { return diag.FromErr(err) @@ -113,7 +113,7 @@ func CreateContextTag(ctx context.Context, d *schema.ResourceData, meta interfac return ReadContextTag(ctx, d, meta) } -func ReadContextTag(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { +func ReadContextTag(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { diags := diag.Diagnostics{} client := meta.(*provider.Context).Client id := helpers.DecodeSnowflakeID(d.Id()).(sdk.SchemaObjectIdentifier) @@ -140,7 +140,7 @@ func ReadContextTag(ctx context.Context, d *schema.ResourceData, meta interface{ return diags } -func UpdateContextTag(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { +func UpdateContextTag(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { client := meta.(*provider.Context).Client id := helpers.DecodeSnowflakeID(d.Id()).(sdk.SchemaObjectIdentifier) if d.HasChange("comment") { @@ -158,9 +158,9 @@ func UpdateContextTag(ctx context.Context, d *schema.ResourceData, meta interfac } } if d.HasChange("allowed_values") { - old, new := d.GetChange("allowed_values") - oldAllowedValues := expandStringList(old.([]interface{})) - newAllowedValues := expandStringList(new.([]interface{})) + o, n := d.GetChange("allowed_values") + oldAllowedValues := expandStringListAllowEmpty(o.([]any)) + newAllowedValues := expandStringListAllowEmpty(n.([]any)) var allowedValuesToAdd, allowedValuesToRemove []string for _, oldAllowedValue := range oldAllowedValues { @@ -182,8 +182,7 @@ func UpdateContextTag(ctx context.Context, d *schema.ResourceData, meta interfac } if len(allowedValuesToRemove) > 0 { - req := sdk.NewAlterTagRequest(id).WithUnset(sdk.NewTagUnsetRequest().WithAllowedValues(true)) - if err := client.Tags.Alter(ctx, req); err != nil { + if err := client.Tags.Alter(ctx, sdk.NewAlterTagRequest(id).WithDrop(allowedValuesToRemove)); err != nil { return diag.FromErr(err) } } @@ -191,7 +190,7 @@ func UpdateContextTag(ctx context.Context, d *schema.ResourceData, meta interfac return ReadContextTag(ctx, d, meta) } -func DeleteContextTag(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { +func DeleteContextTag(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { client := meta.(*provider.Context).Client id := helpers.DecodeSnowflakeID(d.Id()).(sdk.SchemaObjectIdentifier) if err := client.Tags.Drop(ctx, sdk.NewDropTagRequest(id)); err != nil { @@ -202,8 +201,8 @@ func DeleteContextTag(ctx context.Context, d *schema.ResourceData, meta interfac } // Returns the slice of strings for inputed allowed values. -func expandAllowedValues(avChangeSet interface{}) []string { - avList := avChangeSet.([]interface{}) +func expandAllowedValues(avChangeSet any) []string { + avList := avChangeSet.([]any) newAvs := make([]string, len(avList)) for idx, value := range avList { newAvs[idx] = fmt.Sprintf("%v", value) diff --git a/pkg/resources/tag_acceptance_test.go b/pkg/resources/tag_acceptance_test.go index bf9f5be837..727f4f8823 100644 --- a/pkg/resources/tag_acceptance_test.go +++ b/pkg/resources/tag_acceptance_test.go @@ -5,6 +5,7 @@ import ( "testing" acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" + "github.com/hashicorp/terraform-plugin-testing/config" "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" @@ -16,14 +17,20 @@ func TestAcc_Tag(t *testing.T) { resourceName := "snowflake_tag.t" m := func() map[string]config.Variable { return map[string]config.Variable{ - "name": config.StringVariable(name), - "database": config.StringVariable(acc.TestDatabaseName), - "schema": config.StringVariable(acc.TestSchemaName), - "comment": config.StringVariable("Terraform acceptance test"), + "name": config.StringVariable(name), + "database": config.StringVariable(acc.TestDatabaseName), + "schema": config.StringVariable(acc.TestSchemaName), + "comment": config.StringVariable("Terraform acceptance test"), + "allowed_values": config.ListVariable(config.StringVariable("")), } } + variableSet2 := m() - variableSet2["comment"] = config.StringVariable("Terraform acceptance test - updated") + variableSet2["allowed_values"] = config.ListVariable(config.StringVariable("alv1"), config.StringVariable("alv2")) + + variableSet3 := m() + variableSet3["comment"] = config.StringVariable("Terraform acceptance test - updated") + resource.Test(t, resource.TestCase{ ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, PreCheck: func() { acc.TestAccPreCheck(t) }, @@ -40,21 +47,30 @@ func TestAcc_Tag(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "name", name), resource.TestCheckResourceAttr(resourceName, "database", acc.TestDatabaseName), resource.TestCheckResourceAttr(resourceName, "schema", acc.TestSchemaName), + resource.TestCheckResourceAttr(resourceName, "allowed_values.#", "1"), + resource.TestCheckResourceAttr(resourceName, "allowed_values.0", ""), + resource.TestCheckResourceAttr(resourceName, "comment", "Terraform acceptance test"), + ), + }, + + // test - change allowed values + { + ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Tag/basic"), + ConfigVariables: variableSet2, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(resourceName, "name", name), resource.TestCheckResourceAttr(resourceName, "allowed_values.#", "2"), resource.TestCheckResourceAttr(resourceName, "allowed_values.0", "alv1"), resource.TestCheckResourceAttr(resourceName, "allowed_values.1", "alv2"), - resource.TestCheckResourceAttr(resourceName, "comment", "Terraform acceptance test"), ), }, // test - change comment { ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Tag/basic"), - ConfigVariables: variableSet2, + ConfigVariables: variableSet3, Check: resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr(resourceName, "name", name), - resource.TestCheckResourceAttr(resourceName, "database", acc.TestDatabaseName), - resource.TestCheckResourceAttr(resourceName, "schema", acc.TestSchemaName), resource.TestCheckResourceAttr(resourceName, "comment", "Terraform acceptance test - updated"), ), }, @@ -62,7 +78,7 @@ func TestAcc_Tag(t *testing.T) { // test - import { ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Tag/basic"), - ConfigVariables: variableSet2, + ConfigVariables: variableSet3, ResourceName: resourceName, ImportState: true, ImportStateVerify: true, diff --git a/pkg/resources/testdata/TestAcc_Tag/basic/test.tf b/pkg/resources/testdata/TestAcc_Tag/basic/test.tf index 4db5ecd505..10673ee3b9 100644 --- a/pkg/resources/testdata/TestAcc_Tag/basic/test.tf +++ b/pkg/resources/testdata/TestAcc_Tag/basic/test.tf @@ -3,5 +3,5 @@ resource "snowflake_tag" "t" { database = var.database schema = var.schema comment = var.comment - allowed_values = ["alv1", "alv2"] + allowed_values = var.allowed_values } diff --git a/pkg/resources/testdata/TestAcc_Tag/basic/variables.tf b/pkg/resources/testdata/TestAcc_Tag/basic/variables.tf index 68541424ae..ba89ff9f86 100644 --- a/pkg/resources/testdata/TestAcc_Tag/basic/variables.tf +++ b/pkg/resources/testdata/TestAcc_Tag/basic/variables.tf @@ -13,3 +13,7 @@ variable "name" { variable "comment" { type = string } + +variable "allowed_values" { + type = list(string) +} \ No newline at end of file