Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Handle old reference for table_id in table constraint resource #2558

Merged
merged 5 commits into from
Feb 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions MIGRATION_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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 ./...
Expand Down
2 changes: 1 addition & 1 deletion docs/resources/table_constraint.md
Original file line number Diff line number Diff line change
Expand Up @@ -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: "\"<db_name>\".\"<schema_name>\".\"<table_name>\"" or "<db_name>.<schema_name>.<table_name>" (snowflake_table.my_table.id)
- `table_id` (String) Identifier for table to create constraint on. Format must follow: "\"&lt;db_name&gt;\".\"&lt;schema_name&gt;\".\"&lt;table_name&gt;\"" or "&lt;db_name&gt;.&lt;schema_name&gt;.&lt;table_name&gt;" (snowflake_table.my_table.id)
- `type` (String) Type of constraint, one of 'UNIQUE', 'PRIMARY KEY', or 'FOREIGN KEY'

### Optional
Expand Down
12 changes: 10 additions & 2 deletions pkg/resources/table_constraint.go
Original file line number Diff line number Diff line change
Expand Up @@ -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: "\"<db_name>\".\"<schema_name>\".\"<table_name>\"" or "<db_name>.<schema_name>.<table_name>" (snowflake_table.my_table.id)`,
Description: `Identifier for table to create constraint on. Format must follow: "\"&lt;db_name&gt;\".\"&lt;schema_name&gt;\".\"&lt;table_name&gt;\"" or "&lt;db_name&gt;.&lt;schema_name&gt;.&lt;table_name&gt;" (snowflake_table.my_table.id)`,
},
"columns": {
Type: schema.TypeList,
Expand Down Expand Up @@ -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)
}
Expand Down
125 changes: 125 additions & 0 deletions pkg/resources/table_constraint_acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package resources_test

import (
"fmt"
"regexp"
"strings"
"testing"

acc "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/acceptance"
Expand Down Expand Up @@ -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)
}