fix: SNOW-59564 remove default data retention in days value #2029
Conversation
sfc-gh-pbosak
changed the title
|
Integration tests failure for b725423006ab36c08f55a39c6bb34fc4f4e9087c |
pkg/resources/schema.go
Outdated
| @@ -52,7 +52,7 @@ var schemaSchema = map[string]*schema.Schema{ | |||
| Default: false, | |||
| Description: "Specifies a managed schema. Managed access schemas centralize privilege management with the schema owner.", | |||
| }, | |||
| "data_retention_days": { | |||
| "data_retention_time_in_days": { | |||
There was a problem hiding this comment.
It is not necessary to make this breaking change. Please see: https://developer.hashicorp.com/terraform/plugin/sdkv2/best-practices/deprecations#renaming-an-optional-attribute. We can still leave the old attribute as it is.
| @@ -25,7 +25,7 @@ func TestAcc_Table(t *testing.T) { | |||
| resource.TestCheckResourceAttr("snowflake_table.test_table", "name", accName), | |||
| resource.TestCheckResourceAttr("snowflake_table.test_table", "database", accName), | |||
| resource.TestCheckResourceAttr("snowflake_table.test_table", "schema", accName), | |||
| resource.TestCheckResourceAttr("snowflake_table.test_table", "data_retention_days", "1"), | |||
| resource.TestCheckResourceAttr("snowflake_table.test_table", "data_retention_time_in_days", "1"), | |||
There was a problem hiding this comment.
Do we have any acceptance test showing bad behaviour before and the right one after? I think it would be nice to proceed with bugs by first introducing a test showing the error and then fixing it.
There was a problem hiding this comment.
its not really bad behavior, its changing the default value of this parameter, as well as changing the name, and adding documentation on how to use this table resource alongside object parameter.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
pkg/snowflake/table.go
Outdated
| @@ -4,6 +4,7 @@ import ( | |||
| "database/sql" | |||
| "errors" | |||
| "fmt" | |||
| "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" | |||
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
File is not gofumpt-ed (gofumpt)
pkg/resources/table.go
Outdated
| q := builder.ChangeDataRetention(ndr.(int)) | ||
| if err := snowflake.Exec(db, q); err != nil { | ||
| return fmt.Errorf("error changing property on %v", d.Id()) | ||
| var updateDataRetention = func(key string) error { |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
File is not gofumpt-ed (gofumpt)
|
Integration tests failure for c68c82fe40e3237abed4c270c5af455c47f1a990 |
| @@ -25,7 +25,7 @@ func TestAcc_Table(t *testing.T) { | |||
| resource.TestCheckResourceAttr("snowflake_table.test_table", "name", accName), | |||
| resource.TestCheckResourceAttr("snowflake_table.test_table", "database", accName), | |||
| resource.TestCheckResourceAttr("snowflake_table.test_table", "schema", accName), | |||
| resource.TestCheckResourceAttr("snowflake_table.test_table", "data_retention_days", "1"), | |||
| resource.TestCheckResourceAttr("snowflake_table.test_table", "data_retention_time_in_days", "1"), | |||
There was a problem hiding this comment.
its not really bad behavior, its changing the default value of this parameter, as well as changing the name, and adding documentation on how to use this table resource alongside object parameter.
|
Integration tests failure for 6ac72676d8ab523a68d3e9044b4affc4840671ac |
|
Integration tests failure for fdc4ec99b698485074b2b38dd7ddb28f45b46a9d |
|
Integration tests failure for fdc4ec99b698485074b2b38dd7ddb28f45b46a9d |
|
Integration tests failure for 2e843ca4d6679e6fa6862e33f5047dd457810c8a |
…' of https://github.com/Snowflake-Labs/terraform-provider-snowflake into snow-859564-remove-default-data-retention-in-days-value
|
Integration tests failure for 74c81a308569560fc4150fe0582d3bf7a9661406 |
|
Integration tests success for ad2255c6375b32beb364da2b57c4efddbefba842 |
sfc-gh-swinkler
deleted the
snow-859564-remove-default-data-retention-in-days-value
branch
Having a default value caused problems when value for
data_retention_time_in_dayswas set with a separate resource.Terraform would read the current value set by the dedicated parameter resource, compare it to the default value for the table resource and plan to update it since the two values don't match. This gave the illusion that creating another table somehow interferes with the first one, where in fact any
terraform updatewould give a similar resultdata_retention_days->data_retention_time_in_daysfor consistencydata_retention_time_in_daysas not deprecatedfixes #1938