From ddaaa24cdbe5967e570fc63cb69681e09a39beb8 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 26 Feb 2024 16:27:53 +0100 Subject: [PATCH 1/5] Add tests proving #2535 --- .../table_constraint_acceptance_test.go | 125 ++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/pkg/resources/table_constraint_acceptance_test.go b/pkg/resources/table_constraint_acceptance_test.go index efdf4b5d08..dc3499108c 100644 --- a/pkg/resources/table_constraint_acceptance_test.go +++ b/pkg/resources/table_constraint_acceptance_test.go @@ -2,6 +2,8 @@ package resources_test import ( "fmt" + "regexp" + "strings" "testing" acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance" @@ -131,3 +133,126 @@ resource "snowflake_table_constraint" "unique" { `, n, databaseName, schemaName, n) } + +// proves issue https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2535 +func TestAcc_Table_issue2535_newConstraint(t *testing.T) { + accName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + + 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.86.0", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: tableConstraintUniqueConfigUsingTableId(accName, acc.TestDatabaseName, acc.TestSchemaName), + ExpectError: regexp.MustCompile(`.*table id is incorrect.*`), + }, + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: tableConstraintUniqueConfigUsingTableId(accName, acc.TestDatabaseName, acc.TestSchemaName), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_table_constraint.unique", "type", "UNIQUE"), + ), + }, + }, + }) +} + +// proves issue https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2535 +func TestAcc_Table_issue2535_existingTable(t *testing.T) { + accName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { acc.TestAccPreCheck(t) }, + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.RequireAbove(tfversion.Version1_5_0), + }, + CheckDestroy: nil, + Steps: []resource.TestStep{ + // reference done by table.id in 0.85.0 + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.85.0", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: tableConstraintUniqueConfigUsingTableId(accName, acc.TestDatabaseName, acc.TestSchemaName), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_table_constraint.unique", "type", "UNIQUE"), + ), + }, + // switched to qualified_name in 0.86.0 + { + ExternalProviders: map[string]resource.ExternalProvider{ + "snowflake": { + VersionConstraint: "=0.86.0", + Source: "Snowflake-Labs/snowflake", + }, + }, + Config: tableConstraintUniqueConfigUsingFullyQualifiedName(accName, acc.TestDatabaseName, acc.TestSchemaName), + ExpectError: regexp.MustCompile(`.*table id is incorrect.*`), + }, + // fixed in the current version + { + ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories, + Config: tableConstraintUniqueConfigUsingFullyQualifiedName(accName, acc.TestDatabaseName, acc.TestSchemaName), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_table_constraint.unique", "type", "UNIQUE"), + ), + }, + }, + }) +} + +func tableConstraintUniqueConfigUsingFullyQualifiedName(n string, databaseName string, schemaName string) string { + return fmt.Sprintf(` +resource "snowflake_table" "t" { + name = "%s" + database = "%s" + schema = "%s" + + column { + name = "col1" + type = "NUMBER(38,0)" + } +} + +resource "snowflake_table_constraint" "unique" { + name = "%s" + type = "UNIQUE" + table_id = snowflake_table.t.qualified_name + columns = ["col1"] +} +`, n, databaseName, schemaName, n) +} + +func tableConstraintUniqueConfigUsingTableId(n string, databaseName string, schemaName string) string { + return fmt.Sprintf(` +resource "snowflake_table" "t" { + name = "%s" + database = "%s" + schema = "%s" + + column { + name = "col1" + type = "NUMBER(38,0)" + } +} + +resource "snowflake_table_constraint" "unique" { + name = "%s" + type = "UNIQUE" + table_id = snowflake_table.t.id + columns = ["col1"] +} +`, n, databaseName, schemaName, n) +} From 7d4244f9d47de5610bb68c60267dc4a339b88ddb Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 26 Feb 2024 16:57:08 +0100 Subject: [PATCH 2/5] Add fallback for pipe-delimited ids --- pkg/resources/table_constraint.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/pkg/resources/table_constraint.go b/pkg/resources/table_constraint.go index abc9ceb489..1ea1abf37f 100644 --- a/pkg/resources/table_constraint.go +++ b/pkg/resources/table_constraint.go @@ -208,7 +208,15 @@ func (v *tableConstraintID) parse(s string) { } func getTableIdentifier(s string) (*sdk.SchemaObjectIdentifier, error) { - objectIdentifier, err := helpers.DecodeSnowflakeParameterID(s) + var objectIdentifier sdk.ObjectIdentifier + var err error + // TODO [SNOW-999049]: Fallback for old implementations using table.id instead of table.qualified_name - probably will be removed later. + if strings.Contains(s, "|") { + objectIdentifier = helpers.DecodeSnowflakeID(s) + } else { + objectIdentifier, err = helpers.DecodeSnowflakeParameterID(s) + } + if err != nil { return nil, fmt.Errorf("table id is incorrect: %s, err: %w", objectIdentifier, err) } From 8f8a0168c49f7b8e79458137d4f2dd89c1ae1103 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 26 Feb 2024 16:57:56 +0100 Subject: [PATCH 3/5] Fix the table constraint docs --- docs/resources/table_constraint.md | 2 +- pkg/resources/table_constraint.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/resources/table_constraint.md b/docs/resources/table_constraint.md index a149e609d9..171febc02c 100644 --- a/docs/resources/table_constraint.md +++ b/docs/resources/table_constraint.md @@ -105,7 +105,7 @@ resource "snowflake_table_constraint" "unique" { - `columns` (List of String) Columns to use in constraint key - `name` (String) Name of constraint -- `table_id` (String) Identifier for table to create constraint on. Format must follow: "\"\".\"\".\"\"" or ".." (snowflake_table.my_table.id) +- `table_id` (String) Identifier for table to create constraint on. Format must follow: "\"<db_name>\".\"<schema_name>\".\"<table_name>\"" or "<db_name>.<schema_name>.<table_name>" (snowflake_table.my_table.id) - `type` (String) Type of constraint, one of 'UNIQUE', 'PRIMARY KEY', or 'FOREIGN KEY' ### Optional diff --git a/pkg/resources/table_constraint.go b/pkg/resources/table_constraint.go index 1ea1abf37f..6a2de92c94 100644 --- a/pkg/resources/table_constraint.go +++ b/pkg/resources/table_constraint.go @@ -41,7 +41,7 @@ var tableConstraintSchema = map[string]*schema.Schema{ Type: schema.TypeString, Required: true, ForceNew: true, - Description: `Identifier for table to create constraint on. Format must follow: "\"\".\"\".\"\"" or ".." (snowflake_table.my_table.id)`, + Description: `Identifier for table to create constraint on. Format must follow: "\"<db_name>\".\"<schema_name>\".\"<table_name>\"" or "<db_name>.<schema_name>.<table_name>" (snowflake_table.my_table.id)`, }, "columns": { Type: schema.TypeList, From 548deaecfb05de3682baabea76dfe7e419501825 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 26 Feb 2024 17:06:44 +0100 Subject: [PATCH 4/5] Add to migration guide --- MIGRATION_GUIDE.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/MIGRATION_GUIDE.md b/MIGRATION_GUIDE.md index 7eb2a326fb..a2c42bb525 100644 --- a/MIGRATION_GUIDE.md +++ b/MIGRATION_GUIDE.md @@ -41,6 +41,11 @@ It is noted as a behavior change but in some way it is not; with the previous im We will consider adding `NOT NULL` back because it can be set by `ALTER COLUMN columnX SET NOT NULL`, but first we want to revisit the whole resource design. +#### *(behavior change)* table_id reference +The docs were inconsistent. Example prior to 0.86.0 version showed using the `table.id` as the `table_id` reference. The description of the `table_id` parameter never allowed such a value (`table.id` is a `|`-delimited identifier representation and only the `.`-separated values were listed in the docs: https://registry.terraform.io/providers/Snowflake-Labs/snowflake/0.85.0/docs/resources/table_constraint#required. The misuse of `table.id` parameter will result in error after migrating to 0.86.0. To make the config work, please remove and reimport the constraint resource from the state as described in [resource migration doc](./docs/technical-documentation/resource_migration.md). + +After discussions in [#2535](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2535) we decided to provide a temporary workaround in 0.87.0 version, so that the manual migration is not necessary. It allows skipping the migration and jumping straight to 0.87.0 version. However, the temporary workaround will be gone in one of the future versions. Please adjust to the newly suggested reference with the new resources you create. + ### snowflake_external_function resource changes #### *(behavior change)* return_null_allowed default is now true From 286ba3f48ec4012cbf5e557c3012acde6e098625 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 26 Feb 2024 17:10:11 +0100 Subject: [PATCH 5/5] Bump the timeout of acceptance tests --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index a1a64c9adf..a0a9292bd7 100644 --- a/Makefile +++ b/Makefile @@ -64,7 +64,7 @@ test: test-client ## run unit and integration tests go test -v -cover -timeout=30m ./... test-acceptance: ## run acceptance tests - TF_ACC=1 go test -run "^TestAcc_" -v -cover -timeout=45m ./... + TF_ACC=1 go test -run "^TestAcc_" -v -cover -timeout=60m ./... test-integration: ## run SDK integration tests go test -run "^TestInt_" -v -cover -timeout=30m ./...