feat: Warehouse redesign part2 (#2887)

Warehouses redesign part2 (continuation of
#2864):
- state upgraders for all cases (with acceptance tests)
- TODOs in acceptance and integration tests solved
- suspending the resource on the SDK level
- change resource monitor type in the SDK
- add missing UNSET tests
- handle not working UNSETs with proper SETs
- migration guide updated

Other changes:
- Use `Name()` for account object identifiers in show output

Still missing (next PR):
- datasource

Note:
- parts of the logic may be still changed after our internal
discussions; it may affect migration guide and the resource behavior

MIGRATION_GUIDE.md

@@ -23,22 +23,34 @@ Force new was added for the following attributes (because no usable SQL alter st
 - `run_as_role`
 
 ### snowflake_warehouse resource changes
+
+Because of the multiple changes in the resource, the easiest migration way is to follow our [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/resource_migration.md) to perform zero downtime migration. Alternatively, it is possible to follow some pointers below. Either way, familiarize yourself with the resource changes before version bumping.
+
 #### *(potential behavior change)* Default values removed
 As part of the [redesign](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#preparing-essential-ga-objects-for-the-provider-v1) we are removing the default values for attributes having their defaults on Snowflake side to reduce coupling with the provider. Because of that the following defaults were removed:
-- `comment`
-- `statement_timeout_in_seconds`
-- `statement_queued_timeout_in_seconds`
-- `max_concurrency_level`
-- `enable_query_acceleration`
-- `query_acceleration_max_scale_factor`
-- `warehouse_type`
+- `comment` (previously `""`)
+- `enable_query_acceleration` (previously `false`)
+- `query_acceleration_max_scale_factor` (previously `8`)
+- `warehouse_type` (previously `"STANDARD"`)
+- `max_concurrency_level` (previously `8`)
+- `statement_queued_timeout_in_seconds` (previously `0`)
+- `statement_timeout_in_seconds` (previously `172800`)
+
+**Beware!** For attributes being Snowflake parameters (in case of warehouse: `max_concurrency_level`, `statement_queued_timeout_in_seconds`, and `statement_timeout_in_seconds`), this is a breaking change. Previously, not setting a value for them was treated as a fallback to values hardcoded on the provider side. This caused warehouse creation with these parameters set on the warehouse level (and not using the Snowflake default from hierarchy; read more in the [parameters documentation](https://docs.snowflake.com/en/sql-reference/parameters)). To keep the previous values, fill in your configs to the default values listed above.
 
-All previous defaults were aligned with the current Snowflake ones, however:
+All previous defaults were aligned with the current Snowflake ones, however it's not possible to distinguish between filled out value and no value in the automatic state upgrader. Therefore, if the given attribute is not filled out in your configuration, terraform will try to perform update after the change (to UNSET the given attribute to the Snowflake default); it should result in no changes on Snowflake object side, but it is required to make Terraform state aligned with your config. **All** other optional fields that were not set inside the config at all (because of the change in handling state logic on our provider side) will follow the same logic. To avoid the need for the changes, fill out the default fields in your config. Alternatively run apply; no further changes should be shown as a part of the plan.
 
-[//]: # (TODO [SNOW-1348102 - next PR]: state migrator?)
-- if the given parameter was changed on the account level, terraform will try to update it
+#### *(note)* Automatic state migrations
+There are three migrations that should happen automatically with the version bump:
+- incorrect `2XLARGE`, `3XLARGE`, `4XLARGE`, `5XLARGE`, `6XLARGE` values for warehouse size are changed to the proper ones
+- deprecated `wait_for_provisioning` attribute is removed from the state
+- old empty resource monitor attribute is cleaned (earlier it was set to `"null"` string) 
 
-[//]: # (- TODO [SNOW-1348102 - next PR]: describe the new state approach if decided)
+[//]: # (TODO [SNOW-1348102 - after discussion]: describe the new state approach if decided)
+
+#### *(fix)* Warehouse size UNSET
+
+Before the changes, removing warehouse size from the config was not handled properly. Because UNSET is not supported for warehouse size (check the [docs](https://docs.snowflake.com/en/sql-reference/sql/alter-warehouse#properties-parameters) - usage notes for unset) and there are multiple defaults possible, removing the size from config will result in the resource recreation.
 
 #### *(behavior change)* Validation changes
 As part of the [redesign](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#preparing-essential-ga-objects-for-the-provider-v1) we are adjusting validations or removing them to reduce coupling between Snowflake and the provider. Because of that the following validations were removed/adjusted/added:
@@ -57,6 +69,9 @@ As part of the [redesign](https://github.com/Snowflake-Labs/terraform-provider-s
 #### *(behavior change)* `query_acceleration_max_scale_factor` conditional logic removed
 Previously, the `query_acceleration_max_scale_factor` was depending on `enable_query_acceleration` parameter, but it is not required on Snowflake side. After migration, `terraform plan` should suggest changes if `enable_query_acceleration` was earlier set to false (manually or from default) and if `query_acceleration_max_scale_factor` was set in config.
 
+#### *(behavior change)* `initially_suspended` forceNew removed
+Previously, the `initially_suspended` attribute change caused the resource recreation. This attribute is used only during creation (to create suspended warehouse). There is no reason to recreate the whole object just to have initial state changed.
+
 #### *(behavior change)* Boolean type changes
 To easily handle three-value logic (true, false, unknown) in provider's configs, type of `auto_resume` and `enable_query_acceleration` was changed from boolean to string. This should not require updating existing configs (boolean/int value should be accepted and state will be migrated to string automatically), however we recommend changing config values to strings. Terraform should perform an action for configs lacking `auto_resume` or `enable_query_acceleration` (`ALTER WAREHOUSE UNSET AUTO_RESUME` and/or `ALTER WAREHOUSE UNSET ENABLE_QUERY_ACCELERATION` will be run underneath which should not affect the Snowflake object, because `auto_resume` and `enable_query_acceleration` are false by default).
 

Makefile

@@ -62,7 +62,7 @@ sweep: ## destroy the whole architecture; USE ONLY FOR DEVELOPMENT ACCOUNTS
 		fi;
 
 test: test-client ## run unit and integration tests
-	go test -v -cover -timeout=30m ./...
+	go test -v -cover -timeout=45m ./...
 
 test-acceptance: ## run acceptance tests
 	TF_ACC=1 SF_TF_ACC_TEST_CONFIGURE_CLIENT_ONCE=true TEST_SF_TF_REQUIRE_TEST_OBJECT_SUFFIX=1 go test -run "^TestAcc_" -v -cover -timeout=60m ./...
@@ -76,6 +76,9 @@ test-architecture: ## check architecture constraints between packages
 test-client: ## runs test that checks sdk.Client without instrumentedsql
 	SF_TF_NO_INSTRUMENTED_SQL=1 SF_TF_GOSNOWFLAKE_LOG_LEVEL=debug go test ./pkg/sdk/internal/client/... -v
 
+test-acceptance-%: ## run acceptance tests for the given resource only, e.g. test-acceptance-Warehouse
+	TF_ACC=1 SF_TF_ACC_TEST_CONFIGURE_CLIENT_ONCE=true go test -run ^TestAcc_$*_ -v -timeout=20m ./...
+
 build-local: ## build the binary locally
 	go build -o $(BASE_BINARY_NAME) .
 

docs/resources/warehouse.md

@@ -41,8 +41,8 @@ resource "snowflake_warehouse" "warehouse" {
 - `scaling_policy` (String) Specifies the policy for automatically starting and shutting down clusters in a multi-cluster warehouse running in Auto-scale mode. Valid values are (case-insensitive): `STANDARD` | `ECONOMY`.
 - `statement_queued_timeout_in_seconds` (Number) Object parameter that specifies the time, in seconds, a SQL statement (query, DDL, DML, etc.) can be queued on a warehouse before it is canceled by the system.
 - `statement_timeout_in_seconds` (Number) Specifies the time, in seconds, after which a running SQL statement (query, DDL, DML, etc.) is canceled by the system
-- `warehouse_size` (String) Specifies the size of the virtual warehouse. Valid values are (case-insensitive): `XSMALL` | `X-SMALL` | `SMALL` | `MEDIUM` | `LARGE` | `XLARGE` | `X-LARGE` | `XXLARGE` | `X2LARGE` | `2X-LARGE` | `XXXLARGE` | `X3LARGE` | `3X-LARGE` | `X4LARGE` | `4X-LARGE` | `X5LARGE` | `5X-LARGE` | `X6LARGE` | `6X-LARGE`. Consult [warehouse documentation](https://docs.snowflake.com/en/sql-reference/sql/create-warehouse#optional-properties-objectproperties) for the details.
-- `warehouse_type` (String) Specifies warehouse type. Valid values are (case-insensitive): `STANDARD` | `SNOWPARK-OPTIMIZED`.
+- `warehouse_size` (String) Specifies the size of the virtual warehouse. Valid values are (case-insensitive): `XSMALL` | `X-SMALL` | `SMALL` | `MEDIUM` | `LARGE` | `XLARGE` | `X-LARGE` | `XXLARGE` | `X2LARGE` | `2X-LARGE` | `XXXLARGE` | `X3LARGE` | `3X-LARGE` | `X4LARGE` | `4X-LARGE` | `X5LARGE` | `5X-LARGE` | `X6LARGE` | `6X-LARGE`. Consult [warehouse documentation](https://docs.snowflake.com/en/sql-reference/sql/create-warehouse#optional-properties-objectproperties) for the details. Note: removing the size from config will result in the resource recreation.
+- `warehouse_type` (String) Specifies warehouse type. Valid values are (case-insensitive): `STANDARD` | `SNOWPARK-OPTIMIZED`. Warehouse needs to be suspended to change its type. Provider will handle automatic suspension and resumption if needed.
 
 ### Read-Only
 

pkg/acceptance/helpers/parameter_client.go

@@ -47,6 +47,17 @@ func (c *ParameterClient) ShowDatabaseParameters(t *testing.T, id sdk.AccountObj
 	return params
 }
 
+func (c *ParameterClient) ShowWarehouseParameters(t *testing.T, id sdk.AccountObjectIdentifier) []*sdk.Parameter {
+	t.Helper()
+	params, err := c.client().ShowParameters(context.Background(), &sdk.ShowParametersOptions{
+		In: &sdk.ParametersIn{
+			Warehouse: id,
+		},
+	})
+	require.NoError(t, err)
+	return params
+}
+
 func (c *ParameterClient) UpdateAccountParameterTemporarily(t *testing.T, parameter sdk.AccountParameter, newValue string) func() {
 	t.Helper()
 	ctx := context.Background()

pkg/acceptance/helpers/warehouse_client.go

@@ -43,9 +43,14 @@ func (c *WarehouseClient) CreateWarehouse(t *testing.T) (*sdk.Warehouse, func())
 func (c *WarehouseClient) CreateWarehouseWithOptions(t *testing.T, id sdk.AccountObjectIdentifier, opts *sdk.CreateWarehouseOptions) (*sdk.Warehouse, func()) {
 	t.Helper()
 	ctx := context.Background()
+
 	err := c.client().Create(ctx, id, opts)
 	require.NoError(t, err)
-	return &sdk.Warehouse{Name: id.Name()}, c.DropWarehouseFunc(t, id)
+
+	warehouse, err := c.client().ShowByID(ctx, id)
+	require.NoError(t, err)
+
+	return warehouse, c.DropWarehouseFunc(t, id)
 }
 
 func (c *WarehouseClient) DropWarehouseFunc(t *testing.T, id sdk.AccountObjectIdentifier) func() {
@@ -96,6 +101,15 @@ func (c *WarehouseClient) UpdateStatementTimeoutInSeconds(t *testing.T, id sdk.A
 	require.NoError(t, err)
 }
 
+func (c *WarehouseClient) UnsetStatementTimeoutInSeconds(t *testing.T, id sdk.AccountObjectIdentifier) {
+	t.Helper()
+
+	ctx := context.Background()
+
+	err := c.client().Alter(ctx, id, &sdk.AlterWarehouseOptions{Unset: &sdk.WarehouseUnset{StatementTimeoutInSeconds: sdk.Bool(true)}})
+	require.NoError(t, err)
+}
+
 func (c *WarehouseClient) UpdateAutoResume(t *testing.T, id sdk.AccountObjectIdentifier, newAutoResume bool) {
 	t.Helper()
 
@@ -105,6 +119,15 @@ func (c *WarehouseClient) UpdateAutoResume(t *testing.T, id sdk.AccountObjectIde
 	require.NoError(t, err)
 }
 
+func (c *WarehouseClient) UpdateAutoSuspend(t *testing.T, id sdk.AccountObjectIdentifier, newAutoSuspend int) {
+	t.Helper()
+
+	ctx := context.Background()
+
+	err := c.client().Alter(ctx, id, &sdk.AlterWarehouseOptions{Set: &sdk.WarehouseSet{AutoSuspend: sdk.Int(newAutoSuspend)}})
+	require.NoError(t, err)
+}
+
 func (c *WarehouseClient) Suspend(t *testing.T, id sdk.AccountObjectIdentifier) {
 	t.Helper()
 

pkg/acceptance/snowflakechecks/warehouse.go

@@ -52,3 +52,17 @@ func CheckAutoResume(t *testing.T, id sdk.AccountObjectIdentifier, expectedAutoR
 		return nil
 	}
 }
+
+func CheckAutoSuspendCount(t *testing.T, id sdk.AccountObjectIdentifier, expectedAutoSuspend int) func(state *terraform.State) error {
+	t.Helper()
+	return func(_ *terraform.State) error {
+		warehouse, err := acc.TestClient().Warehouse.Show(t, id)
+		if err != nil {
+			return err
+		}
+		if warehouse.AutoSuspend != expectedAutoSuspend {
+			return fmt.Errorf("expected auto suspend: %d; got: %d", expectedAutoSuspend, warehouse.AutoSuspend)
+		}
+		return nil
+	}
+}

pkg/resources/custom_diffs.go

@@ -84,6 +84,9 @@ func ComputedIfAnyAttributeChanged(key string, changedAttributeKeys ...string) s
 	return customdiff.ComputedIf(key, func(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) bool {
 		var result bool
 		for _, changedKey := range changedAttributeKeys {
+			if diff.HasChange(changedKey) {
+				log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: changed key: %s\n", changedKey)
+			}
 			result = result || diff.HasChange(changedKey)
 		}
 		return result

pkg/resources/diff_suppressions.go

@@ -1,6 +1,11 @@
 package resources
 
-import "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+import (
+	"fmt"
+	"log"
+
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+)
 
 func NormalizeAndCompare[T comparable](normalize func(string) (T, error)) schema.SchemaDiffSuppressFunc {
 	return func(_, oldValue, newValue string, _ *schema.ResourceData) bool {
@@ -15,3 +20,39 @@ func NormalizeAndCompare[T comparable](normalize func(string) (T, error)) schema
 		return oldNormalized == newNormalized
 	}
 }
+
+// IgnoreAfterCreation should be used to ignore changes to the given attribute post creation.
+func IgnoreAfterCreation(_, _, _ string, d *schema.ResourceData) bool {
+	// For new resources always show the diff and in every other case we do not want to use this attribute
+	return d.Id() != ""
+}
+
+func IgnoreChangeToCurrentSnowflakeValue(keyInShowOutput string) schema.SchemaDiffSuppressFunc {
+	return func(_, _, new string, d *schema.ResourceData) bool {
+		if d.Id() == "" {
+			return false
+		}
+
+		if showOutput, ok := d.GetOk(showOutputAttributeName); ok {
+			showOutputList := showOutput.([]any)
+			if len(showOutputList) == 1 {
+				result := showOutputList[0].(map[string]any)
+				log.Printf("[DEBUG] IgnoreChangeToCurrentSnowflakeValue: value for key %s is %v, new value is %s, comparison result is: %t", keyInShowOutput, result[keyInShowOutput], new, new == fmt.Sprintf("%v", result[keyInShowOutput]))
+				if new == fmt.Sprintf("%v", result[keyInShowOutput]) {
+					return true
+				}
+			}
+		}
+		return false
+	}
+}
+
+func SuppressIfAny(diffSuppressFunctions ...schema.SchemaDiffSuppressFunc) schema.SchemaDiffSuppressFunc {
+	return func(k, old, new string, d *schema.ResourceData) bool {
+		var suppress bool
+		for _, f := range diffSuppressFunctions {
+			suppress = suppress || f(k, old, new, d)
+		}
+		return suppress
+	}
+}

pkg/resources/diff_suppressions_test.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/resources"
 	"github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -53,3 +54,29 @@ func Test_NormalizeAndCompare(t *testing.T) {
 		assert.False(t, result)
 	})
 }
+
+func Test_IgnoreAfterCreation(t *testing.T) {
+	testSchema := map[string]*schema.Schema{
+		"value": {
+			Type:     schema.TypeString,
+			Optional: true,
+		},
+	}
+
+	t.Run("without id", func(t *testing.T) {
+		in := map[string]any{}
+		d := schema.TestResourceDataRaw(t, testSchema, in)
+
+		result := resources.IgnoreAfterCreation("", "", "", d)
+		assert.False(t, result)
+	})
+
+	t.Run("with id", func(t *testing.T) {
+		in := map[string]any{}
+		d := schema.TestResourceDataRaw(t, testSchema, in)
+		d.SetId("something")
+
+		result := resources.IgnoreAfterCreation("", "", "", d)
+		assert.True(t, result)
+	})
+}