From 99fa60313cffd41bb4388ab0b7fc49cdf50679de Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 2 Sep 2024 17:27:30 +0200 Subject: [PATCH 01/14] fix custom diffs for fields with diff supression starts here From 0d0d598d4605e008788838a49ee8a0e948536984 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 2 Sep 2024 17:47:17 +0200 Subject: [PATCH 02/14] Prepare new custom diff function taking the diff suppress into consideration --- pkg/resources/custom_diffs.go | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/pkg/resources/custom_diffs.go b/pkg/resources/custom_diffs.go index d495e7cd93..b516ef567b 100644 --- a/pkg/resources/custom_diffs.go +++ b/pkg/resources/custom_diffs.go @@ -97,7 +97,33 @@ func ComputedIfAnyAttributeChanged(key string, changedAttributeKeys ...string) s }) } -// TODO(SNOW-1629468): Adjust the function to make it more flexible +func ComputedIfAnyAttributeChangedCheckingDiffSuppression(resourceSchema map[string]*schema.Schema, key string, changedAttributeKeys ...string) schema.CustomizeDiffFunc { + 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) { + oldValue, newValue := diff.GetChange(changedKey) + log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: changed key: %s old: %s new: %s\n", changedKey, oldValue, newValue) + + if v, ok := resourceSchema[changedKey]; ok { + if diffSuppressFunc := v.DiffSuppressFunc; diffSuppressFunc != nil { + if !diffSuppressFunc(key, oldValue.(string), newValue.(string), nil) { + log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: key %s was changed and the diff is not suppressed", changedKey) + result = true + } else { + log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: key %s was changed but the diff is suppresed", changedKey) + } + } else { + log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: key %s was changed and it does not have a diff suppressor", changedKey) + result = true + } + } + } + } + return result + }) +} + func ComputedIfAnyAttributeChangedWithSuppressDiff(key string, suppressDiffFunc schema.SchemaDiffSuppressFunc, changedAttributeKeys ...string) schema.CustomizeDiffFunc { return customdiff.ComputedIf(key, func(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) bool { for _, changedKey := range changedAttributeKeys { From e3563701f2e2db64f570c5c500eddc58ca2c63c9 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 2 Sep 2024 18:10:33 +0200 Subject: [PATCH 03/14] Use the improved computed if any attribute changed implementation --- pkg/resources/account.go | 2 +- pkg/resources/account_role.go | 5 ++--- ...ntegration_with_authorization_code_grant.go | 4 ++-- ...tion_integration_with_client_credentials.go | 4 ++-- ...thentication_integration_with_jwt_bearer.go | 4 ++-- pkg/resources/custom_diffs.go | 18 +++--------------- pkg/resources/database.go | 2 +- pkg/resources/database_role.go | 3 +-- pkg/resources/external_oauth_integration.go | 4 ++-- pkg/resources/file_format.go | 2 +- pkg/resources/function.go | 2 +- pkg/resources/masking_policy.go | 2 +- pkg/resources/materialized_view.go | 2 +- pkg/resources/network_policy.go | 4 +++- .../oauth_integration_for_custom_clients.go | 2 ++ ...uth_integration_for_partner_applications.go | 2 ++ pkg/resources/password_policy.go | 2 +- pkg/resources/procedure.go | 2 +- pkg/resources/saml2_integration.go | 4 ++-- pkg/resources/schema.go | 9 ++++----- pkg/resources/scim_integration.go | 4 ++-- pkg/resources/secondary_database.go | 2 +- pkg/resources/shared_database.go | 2 +- pkg/resources/streamlit.go | 7 +++---- pkg/resources/table.go | 2 +- pkg/resources/user.go | 8 ++++---- pkg/resources/view.go | 4 ++-- pkg/resources/warehouse.go | 7 +++---- 28 files changed, 52 insertions(+), 63 deletions(-) diff --git a/pkg/resources/account.go b/pkg/resources/account.go index 2f41737bdd..0fef63ce97 100644 --- a/pkg/resources/account.go +++ b/pkg/resources/account.go @@ -216,7 +216,7 @@ func Account() *schema.Resource { Delete: DeleteAccount, CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChanged(FullyQualifiedNameAttributeName, "name"), + ComputedIfAnyAttributeChanged(accountSchema, FullyQualifiedNameAttributeName, "name"), ), Schema: accountSchema, diff --git a/pkg/resources/account_role.go b/pkg/resources/account_role.go index e7cc6f7fcf..28922572db 100644 --- a/pkg/resources/account_role.go +++ b/pkg/resources/account_role.go @@ -50,9 +50,8 @@ func AccountRole() *schema.Resource { Description: "The resource is used for role management, where roles can be assigned privileges and, in turn, granted to users and other roles. When granted to roles they can create hierarchies of privilege structures. For more details, refer to the [official documentation](https://docs.snowflake.com/en/user-guide/security-access-control-overview).", CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "comment"), - ComputedIfAnyAttributeChangedWithSuppressDiff(ShowOutputAttributeName, suppressIdentifierQuoting, "name"), - ComputedIfAnyAttributeChangedWithSuppressDiff(FullyQualifiedNameAttributeName, suppressIdentifierQuoting, "name"), + ComputedIfAnyAttributeChanged(accountRoleSchema, ShowOutputAttributeName, "comment", "name"), + ComputedIfAnyAttributeChanged(accountRoleSchema, FullyQualifiedNameAttributeName, "name"), ), Importer: &schema.ResourceImporter{ diff --git a/pkg/resources/api_authentication_integration_with_authorization_code_grant.go b/pkg/resources/api_authentication_integration_with_authorization_code_grant.go index a1fb3df121..c2086e4608 100644 --- a/pkg/resources/api_authentication_integration_with_authorization_code_grant.go +++ b/pkg/resources/api_authentication_integration_with_authorization_code_grant.go @@ -46,8 +46,8 @@ func ApiAuthenticationIntegrationWithAuthorizationCodeGrant() *schema.Resource { ForceNewIfChangeToEmptyString("oauth_token_endpoint"), ForceNewIfChangeToEmptyString("oauth_authorization_endpoint"), ForceNewIfChangeToEmptyString("oauth_client_auth_method"), - ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "enabled", "comment"), - ComputedIfAnyAttributeChanged(DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity", + ComputedIfAnyAttributeChanged(apiAuthAuthorizationCodeGrantSchema, ShowOutputAttributeName, "enabled", "comment"), + ComputedIfAnyAttributeChanged(apiAuthAuthorizationCodeGrantSchema, DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity", "oauth_client_id", "oauth_client_auth_method", "oauth_authorization_endpoint", "oauth_token_endpoint", "oauth_allowed_scopes"), ), diff --git a/pkg/resources/api_authentication_integration_with_client_credentials.go b/pkg/resources/api_authentication_integration_with_client_credentials.go index 53231d8cf3..c98039dcfc 100644 --- a/pkg/resources/api_authentication_integration_with_client_credentials.go +++ b/pkg/resources/api_authentication_integration_with_client_credentials.go @@ -41,8 +41,8 @@ func ApiAuthenticationIntegrationWithClientCredentials() *schema.Resource { CustomizeDiff: customdiff.All( ForceNewIfChangeToEmptyString("oauth_token_endpoint"), ForceNewIfChangeToEmptyString("oauth_client_auth_method"), - ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "enabled", "comment"), - ComputedIfAnyAttributeChanged(DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity", + ComputedIfAnyAttributeChanged(apiAuthClientCredentialsSchema, ShowOutputAttributeName, "enabled", "comment"), + ComputedIfAnyAttributeChanged(apiAuthClientCredentialsSchema, DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity", "oauth_client_id", "oauth_client_auth_method", "oauth_token_endpoint", "oauth_allowed_scopes"), ), Importer: &schema.ResourceImporter{ diff --git a/pkg/resources/api_authentication_integration_with_jwt_bearer.go b/pkg/resources/api_authentication_integration_with_jwt_bearer.go index 23c4a68ee5..b3a508cb46 100644 --- a/pkg/resources/api_authentication_integration_with_jwt_bearer.go +++ b/pkg/resources/api_authentication_integration_with_jwt_bearer.go @@ -45,8 +45,8 @@ func ApiAuthenticationIntegrationWithJwtBearer() *schema.Resource { ForceNewIfChangeToEmptyString("oauth_token_endpoint"), ForceNewIfChangeToEmptyString("oauth_authorization_endpoint"), ForceNewIfChangeToEmptyString("oauth_client_auth_method"), - ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "enabled", "comment"), - ComputedIfAnyAttributeChanged(DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity", + ComputedIfAnyAttributeChanged(apiAuthJwtBearerSchema, ShowOutputAttributeName, "enabled", "comment"), + ComputedIfAnyAttributeChanged(apiAuthJwtBearerSchema, DescribeOutputAttributeName, "enabled", "comment", "oauth_access_token_validity", "oauth_refresh_token_validity", "oauth_client_id", "oauth_client_auth_method", "oauth_authorization_endpoint", "oauth_token_endpoint", "oauth_assertion_issuer"), ), diff --git a/pkg/resources/custom_diffs.go b/pkg/resources/custom_diffs.go index b516ef567b..01b30fd31b 100644 --- a/pkg/resources/custom_diffs.go +++ b/pkg/resources/custom_diffs.go @@ -83,21 +83,8 @@ func ForceNewIfChangeToEmptyString(key string) schema.CustomizeDiffFunc { }) } -func ComputedIfAnyAttributeChanged(key string, changedAttributeKeys ...string) schema.CustomizeDiffFunc { - 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) { - old, new := diff.GetChange(changedKey) - log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: changed key: %s old: %s new: %s\n", changedKey, old, new) - } - result = result || diff.HasChange(changedKey) - } - return result - }) -} - -func ComputedIfAnyAttributeChangedCheckingDiffSuppression(resourceSchema map[string]*schema.Schema, key string, changedAttributeKeys ...string) schema.CustomizeDiffFunc { +// TODO: test +func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key string, changedAttributeKeys ...string) schema.CustomizeDiffFunc { return customdiff.ComputedIf(key, func(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) bool { var result bool for _, changedKey := range changedAttributeKeys { @@ -124,6 +111,7 @@ func ComputedIfAnyAttributeChangedCheckingDiffSuppression(resourceSchema map[str }) } +// TODO: remove func ComputedIfAnyAttributeChangedWithSuppressDiff(key string, suppressDiffFunc schema.SchemaDiffSuppressFunc, changedAttributeKeys ...string) schema.CustomizeDiffFunc { return customdiff.ComputedIf(key, func(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) bool { for _, changedKey := range changedAttributeKeys { diff --git a/pkg/resources/database.go b/pkg/resources/database.go index ec89d89db9..80923faaf2 100644 --- a/pkg/resources/database.go +++ b/pkg/resources/database.go @@ -101,7 +101,7 @@ func Database() *schema.Resource { }, CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChangedWithSuppressDiff(FullyQualifiedNameAttributeName, suppressIdentifierQuoting, "name"), + ComputedIfAnyAttributeChanged(databaseSchema, FullyQualifiedNameAttributeName, "name"), databaseParametersCustomDiff, ), diff --git a/pkg/resources/database_role.go b/pkg/resources/database_role.go index ee98900a7b..c070152ea9 100644 --- a/pkg/resources/database_role.go +++ b/pkg/resources/database_role.go @@ -60,8 +60,7 @@ func DatabaseRole() *schema.Resource { }, CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "comment"), - ComputedIfAnyAttributeChangedWithSuppressDiff(ShowOutputAttributeName, suppressIdentifierQuoting, "name"), + ComputedIfAnyAttributeChanged(databaseRoleSchema, ShowOutputAttributeName, "comment", "name"), ), SchemaVersion: 1, diff --git a/pkg/resources/external_oauth_integration.go b/pkg/resources/external_oauth_integration.go index c8b7ffbead..b454e3fa67 100644 --- a/pkg/resources/external_oauth_integration.go +++ b/pkg/resources/external_oauth_integration.go @@ -167,8 +167,8 @@ func ExternalOauthIntegration() *schema.Resource { ForceNewIfChangeToEmptyString("external_oauth_scope_mapping_attribute"), ForceNewIfChangeToEmptySet("external_oauth_jws_keys_url"), ForceNewIfChangeToEmptySet("external_oauth_token_user_mapping_claim"), - ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "enabled", "external_oauth_type", "comment"), - ComputedIfAnyAttributeChanged(DescribeOutputAttributeName, "enabled", "external_oauth_issuer", "external_oauth_jws_keys_url", "external_oauth_any_role_mode", + ComputedIfAnyAttributeChanged(externalOauthIntegrationSchema, ShowOutputAttributeName, "enabled", "external_oauth_type", "comment"), + ComputedIfAnyAttributeChanged(externalOauthIntegrationSchema, DescribeOutputAttributeName, "enabled", "external_oauth_issuer", "external_oauth_jws_keys_url", "external_oauth_any_role_mode", "external_oauth_rsa_public_key", "external_oauth_rsa_public_key_2", "external_oauth_blocked_roles_list", "external_oauth_allowed_roles_list", "external_oauth_audience_list", "external_oauth_token_user_mapping_claim", "external_oauth_snowflake_user_mapping_attribute", "external_oauth_scope_delimiter", "comment"), diff --git a/pkg/resources/file_format.go b/pkg/resources/file_format.go index adc751fca6..35b2a08205 100644 --- a/pkg/resources/file_format.go +++ b/pkg/resources/file_format.go @@ -319,7 +319,7 @@ func FileFormat() *schema.Resource { Delete: DeleteFileFormat, CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChanged(FullyQualifiedNameAttributeName, "name"), + ComputedIfAnyAttributeChanged(fileFormatSchema, FullyQualifiedNameAttributeName, "name"), ), Schema: fileFormatSchema, diff --git a/pkg/resources/function.go b/pkg/resources/function.go index e9f1b43c53..40709a0feb 100644 --- a/pkg/resources/function.go +++ b/pkg/resources/function.go @@ -173,7 +173,7 @@ func Function() *schema.Resource { CustomizeDiff: customdiff.All( // TODO(SNOW-1348103): add `arguments` to ComputedIfAnyAttributeChanged. This can't be done now because this function compares values without diff suppress. - ComputedIfAnyAttributeChanged(FullyQualifiedNameAttributeName, "name"), + ComputedIfAnyAttributeChanged(functionSchema, FullyQualifiedNameAttributeName, "name"), ), Schema: functionSchema, diff --git a/pkg/resources/masking_policy.go b/pkg/resources/masking_policy.go index b962f8cf81..f10a1321c9 100644 --- a/pkg/resources/masking_policy.go +++ b/pkg/resources/masking_policy.go @@ -122,7 +122,7 @@ func MaskingPolicy() *schema.Resource { Delete: DeleteMaskingPolicy, CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChanged(FullyQualifiedNameAttributeName, "name"), + ComputedIfAnyAttributeChanged(maskingPolicySchema, FullyQualifiedNameAttributeName, "name"), ), Schema: maskingPolicySchema, diff --git a/pkg/resources/materialized_view.go b/pkg/resources/materialized_view.go index e280150418..a280aef202 100644 --- a/pkg/resources/materialized_view.go +++ b/pkg/resources/materialized_view.go @@ -76,7 +76,7 @@ func MaterializedView() *schema.Resource { Delete: DeleteMaterializedView, CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChanged(FullyQualifiedNameAttributeName, "name"), + ComputedIfAnyAttributeChanged(materializedViewSchema, FullyQualifiedNameAttributeName, "name"), ), Schema: materializedViewSchema, diff --git a/pkg/resources/network_policy.go b/pkg/resources/network_policy.go index d04b44bf86..8283f22b63 100644 --- a/pkg/resources/network_policy.go +++ b/pkg/resources/network_policy.go @@ -99,6 +99,7 @@ func NetworkPolicy() *schema.Resource { // for complex types like Sets, Lists, and Maps. When every element of the Set is suppressed in custom diff, // it returns true for d.HasChange anyway (it returns false for suppressed changes on primitive types like Number, Bool, String, etc.). ComputedIfAnyAttributeChanged( + networkPolicySchema, ShowOutputAttributeName, // "allowed_network_rule_list", // "blocked_network_rule_list", @@ -107,13 +108,14 @@ func NetworkPolicy() *schema.Resource { "comment", ), ComputedIfAnyAttributeChanged( + networkPolicySchema, DescribeOutputAttributeName, // "allowed_network_rule_list", // "blocked_network_rule_list", "allowed_ip_list", "blocked_ip_list", ), - ComputedIfAnyAttributeChangedWithSuppressDiff(FullyQualifiedNameAttributeName, suppressIdentifierQuoting, "name"), + ComputedIfAnyAttributeChanged(networkPolicySchema, FullyQualifiedNameAttributeName, "name"), ), Importer: &schema.ResourceImporter{ diff --git a/pkg/resources/oauth_integration_for_custom_clients.go b/pkg/resources/oauth_integration_for_custom_clients.go index 7d7828626b..4462651bf2 100644 --- a/pkg/resources/oauth_integration_for_custom_clients.go +++ b/pkg/resources/oauth_integration_for_custom_clients.go @@ -161,11 +161,13 @@ func OauthIntegrationForCustomClients() *schema.Resource { CustomizeDiff: customdiff.All( ComputedIfAnyAttributeChanged( + oauthIntegrationForCustomClientsSchema, ShowOutputAttributeName, "enabled", "comment", ), ComputedIfAnyAttributeChanged( + oauthIntegrationForCustomClientsSchema, DescribeOutputAttributeName, "oauth_client_type", "oauth_redirect_uri", diff --git a/pkg/resources/oauth_integration_for_partner_applications.go b/pkg/resources/oauth_integration_for_partner_applications.go index de031dab6c..d92d9d4dee 100644 --- a/pkg/resources/oauth_integration_for_partner_applications.go +++ b/pkg/resources/oauth_integration_for_partner_applications.go @@ -121,11 +121,13 @@ func OauthIntegrationForPartnerApplications() *schema.Resource { CustomizeDiff: customdiff.All( ComputedIfAnyAttributeChanged( + oauthIntegrationForPartnerApplicationsSchema, ShowOutputAttributeName, "enabled", "comment", ), ComputedIfAnyAttributeChanged( + oauthIntegrationForPartnerApplicationsSchema, DescribeOutputAttributeName, "oauth_client", "enabled", diff --git a/pkg/resources/password_policy.go b/pkg/resources/password_policy.go index 91ebb5b03b..0aa5aaff98 100644 --- a/pkg/resources/password_policy.go +++ b/pkg/resources/password_policy.go @@ -145,7 +145,7 @@ func PasswordPolicy() *schema.Resource { Delete: DeletePasswordPolicy, CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChanged(FullyQualifiedNameAttributeName, "name"), + ComputedIfAnyAttributeChanged(passwordPolicySchema, FullyQualifiedNameAttributeName, "name"), ), Schema: passwordPolicySchema, diff --git a/pkg/resources/procedure.go b/pkg/resources/procedure.go index 295f3a1b67..f1337acd2b 100644 --- a/pkg/resources/procedure.go +++ b/pkg/resources/procedure.go @@ -189,7 +189,7 @@ func Procedure() *schema.Resource { // TODO(SNOW-1348106): add `arguments` to ComputedIfAnyAttributeChanged for FullyQualifiedNameAttributeName. // This can't be done now because this function compares values without diff suppress. CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChanged(FullyQualifiedNameAttributeName, "name"), + ComputedIfAnyAttributeChanged(procedureSchema, FullyQualifiedNameAttributeName, "name"), ), Schema: procedureSchema, diff --git a/pkg/resources/saml2_integration.go b/pkg/resources/saml2_integration.go index 3939d2249a..6712fd23fe 100644 --- a/pkg/resources/saml2_integration.go +++ b/pkg/resources/saml2_integration.go @@ -172,8 +172,8 @@ func SAML2Integration() *schema.Resource { ForceNewIfChangeToEmptyString("saml2_snowflake_issuer_url"), ForceNewIfChangeToEmptyString("saml2_snowflake_acs_url"), ForceNewIfChangeToEmptyString("saml2_sp_initiated_login_page_label"), - ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "enabled", "comment"), - ComputedIfAnyAttributeChanged(DescribeOutputAttributeName, "saml2_issuer", "saml2_sso_url", "saml2_provider", "saml2_x509_cert", + ComputedIfAnyAttributeChanged(saml2IntegrationSchema, ShowOutputAttributeName, "enabled", "comment"), + ComputedIfAnyAttributeChanged(saml2IntegrationSchema, DescribeOutputAttributeName, "saml2_issuer", "saml2_sso_url", "saml2_provider", "saml2_x509_cert", "saml2_sp_initiated_login_page_label", "saml2_enable_sp_initiated", "saml2_sign_request", "saml2_requtedted_nameid_format", "saml2_post_logout_redirect_url", "saml2_force_authn", "saml2_snowflake_issuer_url", "saml2_snowflake_acs_url", "allowed_user_domains", "allowed_email_patterns"), diff --git a/pkg/resources/schema.go b/pkg/resources/schema.go index 55f54d02cf..de3c4e2e48 100644 --- a/pkg/resources/schema.go +++ b/pkg/resources/schema.go @@ -96,11 +96,10 @@ func Schema() *schema.Resource { Description: "Resource used to manage schema objects. For more information, check [schema documentation](https://docs.snowflake.com/en/sql-reference/sql/create-schema).", CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "comment", "with_managed_access", "is_transient"), - ComputedIfAnyAttributeChangedWithSuppressDiff(ShowOutputAttributeName, suppressIdentifierQuoting, "name"), - ComputedIfAnyAttributeChangedWithSuppressDiff(DescribeOutputAttributeName, suppressIdentifierQuoting, "name"), - ComputedIfAnyAttributeChangedWithSuppressDiff(FullyQualifiedNameAttributeName, suppressIdentifierQuoting, "name"), - ComputedIfAnyAttributeChanged(ParametersAttributeName, collections.Map(sdk.AsStringList(sdk.AllSchemaParameters), strings.ToLower)...), + ComputedIfAnyAttributeChanged(schemaSchema, ShowOutputAttributeName, "name", "comment", "with_managed_access", "is_transient"), + ComputedIfAnyAttributeChanged(schemaSchema, DescribeOutputAttributeName, "name"), + ComputedIfAnyAttributeChanged(schemaSchema, FullyQualifiedNameAttributeName, "name"), + ComputedIfAnyAttributeChanged(schemaParametersSchema, ParametersAttributeName, collections.Map(sdk.AsStringList(sdk.AllSchemaParameters), strings.ToLower)...), schemaParametersCustomDiff, ), diff --git a/pkg/resources/scim_integration.go b/pkg/resources/scim_integration.go index fab3d51ae0..8ca1aeedb2 100644 --- a/pkg/resources/scim_integration.go +++ b/pkg/resources/scim_integration.go @@ -109,8 +109,8 @@ func SCIMIntegration() *schema.Resource { }, CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "enabled", "scim_client", "comment"), - ComputedIfAnyAttributeChanged(DescribeOutputAttributeName, "enabled", "comment", "network_policy", "run_as_role", "sync_password"), + ComputedIfAnyAttributeChanged(scimIntegrationSchema, ShowOutputAttributeName, "enabled", "scim_client", "comment"), + ComputedIfAnyAttributeChanged(scimIntegrationSchema, DescribeOutputAttributeName, "enabled", "comment", "network_policy", "run_as_role", "sync_password"), ), SchemaVersion: 2, diff --git a/pkg/resources/secondary_database.go b/pkg/resources/secondary_database.go index 361db4ab48..155699d1c9 100644 --- a/pkg/resources/secondary_database.go +++ b/pkg/resources/secondary_database.go @@ -53,7 +53,7 @@ func SecondaryDatabase() *schema.Resource { CustomizeDiff: customdiff.All( databaseParametersCustomDiff, - ComputedIfAnyAttributeChangedWithSuppressDiff(FullyQualifiedNameAttributeName, suppressIdentifierQuoting, "name"), + ComputedIfAnyAttributeChanged(secondaryDatabaseSchema, FullyQualifiedNameAttributeName, "name"), ), Schema: helpers.MergeMaps(secondaryDatabaseSchema, databaseParametersSchema), Importer: &schema.ResourceImporter{ diff --git a/pkg/resources/shared_database.go b/pkg/resources/shared_database.go index b6da4eaf36..8339148656 100644 --- a/pkg/resources/shared_database.go +++ b/pkg/resources/shared_database.go @@ -53,7 +53,7 @@ func SharedDatabase() *schema.Resource { Description: "A shared database creates a database from a share provided by another Snowflake account. For more information about shares, see [Introduction to Secure Data Sharing](https://docs.snowflake.com/en/user-guide/data-sharing-intro).", CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChangedWithSuppressDiff(FullyQualifiedNameAttributeName, suppressIdentifierQuoting, "name"), + ComputedIfAnyAttributeChanged(sharedDatabaseSchema, FullyQualifiedNameAttributeName, "name"), ), Schema: helpers.MergeMaps(sharedDatabaseSchema, sharedDatabaseParametersSchema), diff --git a/pkg/resources/streamlit.go b/pkg/resources/streamlit.go index e386f10f6c..830e7c0924 100644 --- a/pkg/resources/streamlit.go +++ b/pkg/resources/streamlit.go @@ -118,10 +118,9 @@ func Streamlit() *schema.Resource { }, CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "title", "comment", "query_warehouse"), - ComputedIfAnyAttributeChangedWithSuppressDiff(ShowOutputAttributeName, suppressIdentifierQuoting, "name"), - ComputedIfAnyAttributeChangedWithSuppressDiff(FullyQualifiedNameAttributeName, suppressIdentifierQuoting, "name"), - ComputedIfAnyAttributeChanged(DescribeOutputAttributeName, "title", "comment", "root_location", "main_file", "query_warehouse", "external_access_integrations"), + ComputedIfAnyAttributeChanged(streamlitSchema, ShowOutputAttributeName, "name", "title", "comment", "query_warehouse"), + ComputedIfAnyAttributeChanged(streamlitSchema, FullyQualifiedNameAttributeName, "name"), + ComputedIfAnyAttributeChanged(streamlitSchema, DescribeOutputAttributeName, "title", "comment", "root_location", "main_file", "query_warehouse", "external_access_integrations"), ), SchemaVersion: 1, diff --git a/pkg/resources/table.go b/pkg/resources/table.go index ad10836b74..100d89cffa 100644 --- a/pkg/resources/table.go +++ b/pkg/resources/table.go @@ -212,7 +212,7 @@ func Table() *schema.Resource { Delete: DeleteTable, CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChanged(FullyQualifiedNameAttributeName, "name"), + ComputedIfAnyAttributeChanged(tableSchema, FullyQualifiedNameAttributeName, "name"), ), Schema: tableSchema, diff --git a/pkg/resources/user.go b/pkg/resources/user.go index 05108193e4..58af3bae16 100644 --- a/pkg/resources/user.go +++ b/pkg/resources/user.go @@ -199,10 +199,10 @@ func User() *schema.Resource { }, CustomizeDiff: customdiff.All( - // TODO [SNOW-1629468 - next pr]: handle diff suppression correctly; add "default_role", "default_secondary_roles" - ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "password", "login_name", "display_name", "first_name", "middle_name", "last_name", "email", "must_change_password", "disabled", "days_to_expiry", "mins_to_unlock", "default_warehouse", "default_namespace", "mins_to_bypass_mfa", "rsa_public_key", "rsa_public_key_2", "comment", "disable_mfa"), - ComputedIfAnyAttributeChanged(ParametersAttributeName, collections.Map(sdk.AsStringList(sdk.AllUserParameters), strings.ToLower)...), - ComputedIfAnyAttributeChanged(FullyQualifiedNameAttributeName, "name"), + // TODO [SNOW-1629468 - next pr]: test "default_role", "default_secondary_roles" + ComputedIfAnyAttributeChanged(userSchema, ShowOutputAttributeName, "password", "login_name", "display_name", "first_name", "middle_name", "last_name", "email", "must_change_password", "disabled", "days_to_expiry", "mins_to_unlock", "default_warehouse", "default_namespace", "default_role", "default_secondary_roles", "mins_to_bypass_mfa", "rsa_public_key", "rsa_public_key_2", "comment", "disable_mfa"), + ComputedIfAnyAttributeChanged(userParametersSchema, ParametersAttributeName, collections.Map(sdk.AsStringList(sdk.AllUserParameters), strings.ToLower)...), + ComputedIfAnyAttributeChanged(userSchema, FullyQualifiedNameAttributeName, "name"), userParametersCustomDiff, // TODO [SNOW-1645348]: revisit with service user work func(_ context.Context, diff *schema.ResourceDiff, _ interface{}) error { diff --git a/pkg/resources/view.go b/pkg/resources/view.go index e9e929f2da..5090e2b9ee 100644 --- a/pkg/resources/view.go +++ b/pkg/resources/view.go @@ -280,8 +280,8 @@ func View() *schema.Resource { Description: "Resource used to manage view objects. For more information, check [view documentation](https://docs.snowflake.com/en/sql-reference/sql/create-view).", CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "comment", "change_tracking", "is_secure", "is_temporary", "is_recursive", "statement"), - ComputedIfAnyAttributeChanged(FullyQualifiedNameAttributeName, "name"), + ComputedIfAnyAttributeChanged(viewSchema, ShowOutputAttributeName, "comment", "change_tracking", "is_secure", "is_temporary", "is_recursive", "statement"), + ComputedIfAnyAttributeChanged(viewSchema, FullyQualifiedNameAttributeName, "name"), ), Schema: viewSchema, diff --git a/pkg/resources/warehouse.go b/pkg/resources/warehouse.go index 9623cd947b..6232b0bac1 100644 --- a/pkg/resources/warehouse.go +++ b/pkg/resources/warehouse.go @@ -204,10 +204,9 @@ func Warehouse() *schema.Resource { }, CustomizeDiff: customdiff.All( - ComputedIfAnyAttributeChanged(ShowOutputAttributeName, "warehouse_type", "warehouse_size", "max_cluster_count", "min_cluster_count", "scaling_policy", "auto_suspend", "auto_resume", "comment", "enable_query_acceleration", "query_acceleration_max_scale_factor"), - ComputedIfAnyAttributeChanged(ParametersAttributeName, strings.ToLower(string(sdk.ObjectParameterMaxConcurrencyLevel)), strings.ToLower(string(sdk.ObjectParameterStatementQueuedTimeoutInSeconds)), strings.ToLower(string(sdk.ObjectParameterStatementTimeoutInSeconds))), - ComputedIfAnyAttributeChangedWithSuppressDiff(ShowOutputAttributeName, suppressIdentifierQuoting, "name", "resource_monitor"), - ComputedIfAnyAttributeChangedWithSuppressDiff(FullyQualifiedNameAttributeName, suppressIdentifierQuoting, "name"), + ComputedIfAnyAttributeChanged(warehouseSchema, ShowOutputAttributeName, "name", "warehouse_type", "warehouse_size", "max_cluster_count", "min_cluster_count", "scaling_policy", "auto_suspend", "auto_resume", "resource_monitor", "comment", "enable_query_acceleration", "query_acceleration_max_scale_factor"), + ComputedIfAnyAttributeChanged(warehouseSchema, ParametersAttributeName, strings.ToLower(string(sdk.ObjectParameterMaxConcurrencyLevel)), strings.ToLower(string(sdk.ObjectParameterStatementQueuedTimeoutInSeconds)), strings.ToLower(string(sdk.ObjectParameterStatementTimeoutInSeconds))), + ComputedIfAnyAttributeChanged(warehouseSchema, FullyQualifiedNameAttributeName, "name"), customdiff.ForceNewIfChange("warehouse_size", func(ctx context.Context, old, new, meta any) bool { return old.(string) != "" && new.(string) == "" From 1ae8ab47b8c5619e322720306470b26833d4bfd6 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 2 Sep 2024 18:34:09 +0200 Subject: [PATCH 04/14] Unit test improved computed if any attribute changed --- pkg/resources/custom_diffs.go | 16 ---- pkg/resources/custom_diffs_test.go | 149 ++++++++++++++++++++--------- 2 files changed, 103 insertions(+), 62 deletions(-) diff --git a/pkg/resources/custom_diffs.go b/pkg/resources/custom_diffs.go index 01b30fd31b..d31a012ab8 100644 --- a/pkg/resources/custom_diffs.go +++ b/pkg/resources/custom_diffs.go @@ -111,22 +111,6 @@ func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key }) } -// TODO: remove -func ComputedIfAnyAttributeChangedWithSuppressDiff(key string, suppressDiffFunc schema.SchemaDiffSuppressFunc, changedAttributeKeys ...string) schema.CustomizeDiffFunc { - return customdiff.ComputedIf(key, func(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) bool { - for _, changedKey := range changedAttributeKeys { - if diff.HasChange(changedKey) { - oldValue, newValue := diff.GetChange(changedKey) - if !suppressDiffFunc(key, oldValue.(string), newValue.(string), nil) { - log.Printf("[DEBUG] ComputedIfAnyAttributeChangedWithSuppressDiff: changed key: %s", changedKey) - return true - } - } - } - return false - }) -} - type parameter[T ~string] struct { parameterName T valueType valueType diff --git a/pkg/resources/custom_diffs_test.go b/pkg/resources/custom_diffs_test.go index 5bd28733aa..ca847e492b 100644 --- a/pkg/resources/custom_diffs_test.go +++ b/pkg/resources/custom_diffs_test.go @@ -345,10 +345,40 @@ func TestForceNewIfChangeToEmptySet(t *testing.T) { } } -func TestComputedIfAnyAttributeChangedWithSuppressDiff(t *testing.T) { - suppressFunc := schema.SchemaDiffSuppressFunc(func(k, oldValue, newValue string, d *schema.ResourceData) bool { +func Test_ComputedIfAnyAttributeChanged(t *testing.T) { + testSuppressFunc := schema.SchemaDiffSuppressFunc(func(k, oldValue, newValue string, d *schema.ResourceData) bool { return strings.Trim(oldValue, `"`) == strings.Trim(newValue, `"`) }) + testSchema := map[string]*schema.Schema{ + "value_with_diff_suppress": { + Type: schema.TypeString, + Optional: true, + DiffSuppressFunc: testSuppressFunc, + }, + "value_without_diff_suppress": { + Type: schema.TypeString, + Optional: true, + }, + "computed_value": { + Type: schema.TypeString, + Computed: true, + }, + } + testCustomDiff := resources.ComputedIfAnyAttributeChanged( + testSchema, + "computed_value", + "value_with_diff_suppress", + "value_without_diff_suppress", + ) + testProvider := &schema.Provider{ + ResourcesMap: map[string]*schema.Resource{ + "test": { + Schema: testSchema, + CustomizeDiff: testCustomDiff, + }, + }, + } + tests := []struct { name string stateValue map[string]string @@ -356,89 +386,116 @@ func TestComputedIfAnyAttributeChangedWithSuppressDiff(t *testing.T) { expectDiff bool }{ { - name: "no change", + name: "no change in both values", stateValue: map[string]string{ - "value": "foo", - "computed_value": "foo", + "value_with_diff_suppress": "foo", + "value_without_diff_suppress": "foo", + "computed_value": "foo", }, rawConfigValue: map[string]any{ - "value": "foo", + "value_with_diff_suppress": "foo", + "value_without_diff_suppress": "foo", }, expectDiff: false, }, { - name: "no change - quotes in config", + name: "change on field with diff suppression - suppressed (quotes in config added)", stateValue: map[string]string{ - "value": "foo", - "computed_value": "foo", + "value_with_diff_suppress": "foo", + "value_without_diff_suppress": "foo", + "computed_value": "foo", }, rawConfigValue: map[string]any{ - "value": "\"foo\"", + "value_with_diff_suppress": "\"foo\"", + "value_without_diff_suppress": "foo", }, expectDiff: false, }, { - name: "no change - quotes in state", + name: "change on field with diff suppression - suppressed (quotes in config removed)", stateValue: map[string]string{ - "value": "\"foo\"", - "computed_value": "foo", + "value_with_diff_suppress": "\"foo\"", + "value_without_diff_suppress": "foo", + "computed_value": "foo", }, rawConfigValue: map[string]any{ - "value": "foo", + "value_with_diff_suppress": "foo", + "value_without_diff_suppress": "foo", }, expectDiff: false, }, { - name: "name change", + name: "change on field with diff suppression - not suppressed (value change)", stateValue: map[string]string{ - "value": "foo", - "computed_value": "foo", + "value_with_diff_suppress": "foo", + "value_without_diff_suppress": "foo", + "computed_value": "foo", }, rawConfigValue: map[string]any{ - "value": "bar", + "value_with_diff_suppress": "bar", + "value_without_diff_suppress": "foo", }, expectDiff: true, }, { - name: "name and quoting change", + name: "change on field with diff suppression - not suppressed (value and quotes changed)", stateValue: map[string]string{ - "value": "\"foo\"", - "computed_value": "foo", + "value_with_diff_suppress": "\"foo\"", + "value_without_diff_suppress": "foo", + "computed_value": "foo", }, rawConfigValue: map[string]any{ - "value": "bar", + "value_with_diff_suppress": "bar", + "value_without_diff_suppress": "foo", + }, + expectDiff: true, + }, + { + name: "change on field without diff suppression", + stateValue: map[string]string{ + "value_with_diff_suppress": "foo", + "value_without_diff_suppress": "foo", + "computed_value": "foo", + }, + rawConfigValue: map[string]any{ + "value_with_diff_suppress": "foo", + "value_without_diff_suppress": "bar", + }, + expectDiff: true, + }, + { + name: "change on field without diff suppression, suppressed change on field with diff suppression", + stateValue: map[string]string{ + "value_with_diff_suppress": "foo", + "value_without_diff_suppress": "foo", + "computed_value": "foo", + }, + rawConfigValue: map[string]any{ + "value_with_diff_suppress": "\"foo\"", + "value_without_diff_suppress": "bar", + }, + expectDiff: true, + }, + { + name: "change on field without diff suppression, not suppressed change on field with diff suppression", + stateValue: map[string]string{ + "value_with_diff_suppress": "foo", + "value_without_diff_suppress": "foo", + "computed_value": "foo", + }, + rawConfigValue: map[string]any{ + "value_with_diff_suppress": "\"bar\"", + "value_without_diff_suppress": "bar", }, expectDiff: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - customDiff := resources.ComputedIfAnyAttributeChangedWithSuppressDiff( - "computed_value", - suppressFunc, - "value", - ) - provider := &schema.Provider{ - ResourcesMap: map[string]*schema.Resource{ - "test": { - Schema: map[string]*schema.Schema{ - "value": { - Type: schema.TypeString, - Required: true, - DiffSuppressFunc: suppressFunc, - }, - "computed_value": { - Type: schema.TypeString, - Computed: true, - }, - }, - CustomizeDiff: customDiff, - }, - }, - } + tt := tt diff := calculateDiffFromAttributes( t, - provider, + testProvider, tt.stateValue, tt.rawConfigValue, ) From cddde7d1953df3e65a7c6e3990d5af3b60842571 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 2 Sep 2024 18:40:59 +0200 Subject: [PATCH 05/14] Add negative test --- pkg/resources/custom_diffs.go | 1 - pkg/resources/custom_diffs_test.go | 47 +++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/pkg/resources/custom_diffs.go b/pkg/resources/custom_diffs.go index d31a012ab8..3561b049d9 100644 --- a/pkg/resources/custom_diffs.go +++ b/pkg/resources/custom_diffs.go @@ -83,7 +83,6 @@ func ForceNewIfChangeToEmptyString(key string) schema.CustomizeDiffFunc { }) } -// TODO: test func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key string, changedAttributeKeys ...string) schema.CustomizeDiffFunc { return customdiff.ComputedIf(key, func(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) bool { var result bool diff --git a/pkg/resources/custom_diffs_test.go b/pkg/resources/custom_diffs_test.go index ca847e492b..e27a3af73a 100644 --- a/pkg/resources/custom_diffs_test.go +++ b/pkg/resources/custom_diffs_test.go @@ -386,7 +386,7 @@ func Test_ComputedIfAnyAttributeChanged(t *testing.T) { expectDiff bool }{ { - name: "no change in both values", + name: "no change on both fields", stateValue: map[string]string{ "value_with_diff_suppress": "foo", "value_without_diff_suppress": "foo", @@ -507,4 +507,49 @@ func Test_ComputedIfAnyAttributeChanged(t *testing.T) { } }) } + + t.Run("attributes not found in schema, both fields changed", func(t *testing.T) { + otherTestSchema := map[string]*schema.Schema{ + "value": { + Type: schema.TypeString, + Optional: true, + DiffSuppressFunc: testSuppressFunc, + }, + "computed_value": { + Type: schema.TypeString, + Computed: true, + }, + } + otherTestCustomDiff := resources.ComputedIfAnyAttributeChanged( + otherTestSchema, + "computed_value", + "value_with_diff_suppress", + "value_without_diff_suppress", + ) + otherTestProvider := &schema.Provider{ + ResourcesMap: map[string]*schema.Resource{ + "test": { + Schema: testSchema, + CustomizeDiff: otherTestCustomDiff, + }, + }, + } + + diff := calculateDiffFromAttributes( + t, + otherTestProvider, + map[string]string{ + "value_with_diff_suppress": "foo", + "value_without_diff_suppress": "foo", + "computed_value": "foo", + }, + map[string]any{ + "value_with_diff_suppress": "\"bar\"", + "value_without_diff_suppress": "bar", + }, + ) + + require.NotNil(t, diff) + assert.Nil(t, diff.Attributes["computed_value"]) + }) } From 3c36974fce82abeff7d22113c6bb0cd62056d652 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 2 Sep 2024 18:48:21 +0200 Subject: [PATCH 06/14] Add description --- pkg/resources/custom_diffs.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/resources/custom_diffs.go b/pkg/resources/custom_diffs.go index 3561b049d9..442b4c5180 100644 --- a/pkg/resources/custom_diffs.go +++ b/pkg/resources/custom_diffs.go @@ -83,6 +83,9 @@ func ForceNewIfChangeToEmptyString(key string) schema.CustomizeDiffFunc { }) } +// ComputedIfAnyAttributeChanged marks the given fields as computed if any of the listed fields changes. +// It takes field-level diffSuppress into consideration based on the schema passed. +// If the field is not found in the given schema, it continues without error. func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key string, changedAttributeKeys ...string) schema.CustomizeDiffFunc { return customdiff.ComputedIf(key, func(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) bool { var result bool From 4b90b491463ff98eec78f28e9dbbf62fd382862d Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 2 Sep 2024 18:53:34 +0200 Subject: [PATCH 07/14] Add important TODO --- pkg/resources/custom_diffs.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/resources/custom_diffs.go b/pkg/resources/custom_diffs.go index 442b4c5180..ff6a46ce61 100644 --- a/pkg/resources/custom_diffs.go +++ b/pkg/resources/custom_diffs.go @@ -86,6 +86,7 @@ func ForceNewIfChangeToEmptyString(key string) schema.CustomizeDiffFunc { // ComputedIfAnyAttributeChanged marks the given fields as computed if any of the listed fields changes. // It takes field-level diffSuppress into consideration based on the schema passed. // If the field is not found in the given schema, it continues without error. +// TODO: diffSuppressFunc sometimes use the last param (d) and we can't get it from the resource diff (ResourceDiff != ResourceData) func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key string, changedAttributeKeys ...string) schema.CustomizeDiffFunc { return customdiff.ComputedIf(key, func(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) bool { var result bool From a7cb8cfb4d08b856c25ea84d1b6dc968cea33d5a Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 2 Sep 2024 19:28:46 +0200 Subject: [PATCH 08/14] Hack resource data --- pkg/resources/custom_diffs.go | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/pkg/resources/custom_diffs.go b/pkg/resources/custom_diffs.go index ff6a46ce61..b1f2a66f66 100644 --- a/pkg/resources/custom_diffs.go +++ b/pkg/resources/custom_diffs.go @@ -3,14 +3,16 @@ package resources import ( "context" "log" + "reflect" "strconv" "strings" + "unsafe" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/collections" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" ) func StringParameterValueComputedIf[T ~string](key string, params []*sdk.Parameter, parameterLevel sdk.ParameterType, parameter T) schema.CustomizeDiffFunc { @@ -86,7 +88,6 @@ func ForceNewIfChangeToEmptyString(key string) schema.CustomizeDiffFunc { // ComputedIfAnyAttributeChanged marks the given fields as computed if any of the listed fields changes. // It takes field-level diffSuppress into consideration based on the schema passed. // If the field is not found in the given schema, it continues without error. -// TODO: diffSuppressFunc sometimes use the last param (d) and we can't get it from the resource diff (ResourceDiff != ResourceData) func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key string, changedAttributeKeys ...string) schema.CustomizeDiffFunc { return customdiff.ComputedIf(key, func(ctx context.Context, diff *schema.ResourceDiff, meta interface{}) bool { var result bool @@ -97,7 +98,11 @@ func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key if v, ok := resourceSchema[changedKey]; ok { if diffSuppressFunc := v.DiffSuppressFunc; diffSuppressFunc != nil { - if !diffSuppressFunc(key, oldValue.(string), newValue.(string), nil) { + hackedResourceData, hackedCorrectly := hackResourceData(resourceSchema, diff) + if !hackedCorrectly { + continue + } + if !diffSuppressFunc(key, oldValue.(string), newValue.(string), hackedResourceData) { log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: key %s was changed and the diff is not suppressed", changedKey) result = true } else { @@ -114,6 +119,27 @@ func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key }) } +// TODO: extract hacky stuff outside this directory? +func hackResourceData(resourceSchema schema.InternalMap, diff *schema.ResourceDiff) (*schema.ResourceData, bool) { + unexportedState := reflect.ValueOf(diff).Elem().FieldByName("state") + hackedState := reflect.NewAt(unexportedState.Type(), unsafe.Pointer(unexportedState.UnsafeAddr())).Interface() + unexportedDiff := reflect.ValueOf(diff).Elem().FieldByName("diff") + hackedDiff := reflect.NewAt(unexportedDiff.Type(), unsafe.Pointer(unexportedDiff.UnsafeAddr())).Interface() + castState, ok := hackedState.(*terraform.InstanceState) + if !ok { + return nil, false + } + castDiff, ok := hackedDiff.(*terraform.InstanceDiff) + if !ok { + return nil, false + } + hackedResourceData, err := resourceSchema.Data(castState, castDiff) + if err != nil { + return nil, false + } + return hackedResourceData, true +} + type parameter[T ~string] struct { parameterName T valueType valueType From 50706e51c75942c086ef1a2e15504fcaf3126b66 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 2 Sep 2024 19:46:22 +0200 Subject: [PATCH 09/14] Fix unit test --- pkg/resources/custom_diffs.go | 1 + pkg/resources/custom_diffs_test.go | 1 + 2 files changed, 2 insertions(+) diff --git a/pkg/resources/custom_diffs.go b/pkg/resources/custom_diffs.go index b1f2a66f66..1b14db9139 100644 --- a/pkg/resources/custom_diffs.go +++ b/pkg/resources/custom_diffs.go @@ -100,6 +100,7 @@ func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key if diffSuppressFunc := v.DiffSuppressFunc; diffSuppressFunc != nil { hackedResourceData, hackedCorrectly := hackResourceData(resourceSchema, diff) if !hackedCorrectly { + log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: did not create resource data correctly, skipping\n") continue } if !diffSuppressFunc(key, oldValue.(string), newValue.(string), hackedResourceData) { diff --git a/pkg/resources/custom_diffs_test.go b/pkg/resources/custom_diffs_test.go index e27a3af73a..acecdfee11 100644 --- a/pkg/resources/custom_diffs_test.go +++ b/pkg/resources/custom_diffs_test.go @@ -501,6 +501,7 @@ func Test_ComputedIfAnyAttributeChanged(t *testing.T) { ) if tt.expectDiff { require.NotNil(t, diff) + require.NotNil(t, diff.Attributes["computed_value"]) assert.True(t, diff.Attributes["computed_value"].NewComputed) } else { require.Nil(t, diff) From a9831f7b147ece150c376f04ed3d33ac8b436639 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 2 Sep 2024 19:53:05 +0200 Subject: [PATCH 10/14] Fix hack impl --- pkg/resources/custom_diffs.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/resources/custom_diffs.go b/pkg/resources/custom_diffs.go index 1b14db9139..4f37fb23d9 100644 --- a/pkg/resources/custom_diffs.go +++ b/pkg/resources/custom_diffs.go @@ -123,9 +123,9 @@ func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key // TODO: extract hacky stuff outside this directory? func hackResourceData(resourceSchema schema.InternalMap, diff *schema.ResourceDiff) (*schema.ResourceData, bool) { unexportedState := reflect.ValueOf(diff).Elem().FieldByName("state") - hackedState := reflect.NewAt(unexportedState.Type(), unsafe.Pointer(unexportedState.UnsafeAddr())).Interface() + hackedState := reflect.NewAt(unexportedState.Type(), unsafe.Pointer(unexportedState.UnsafeAddr())).Elem().Interface() unexportedDiff := reflect.ValueOf(diff).Elem().FieldByName("diff") - hackedDiff := reflect.NewAt(unexportedDiff.Type(), unsafe.Pointer(unexportedDiff.UnsafeAddr())).Interface() + hackedDiff := reflect.NewAt(unexportedDiff.Type(), unsafe.Pointer(unexportedDiff.UnsafeAddr())).Elem().Interface() castState, ok := hackedState.(*terraform.InstanceState) if !ok { return nil, false From 85d1e2780cf2c52cf43b8d4cf049feb3bec7b42f Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Mon, 2 Sep 2024 22:37:50 +0200 Subject: [PATCH 11/14] Fix old and new values passed to diff suppress --- pkg/resources/custom_diffs.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/resources/custom_diffs.go b/pkg/resources/custom_diffs.go index 4f37fb23d9..e807127807 100644 --- a/pkg/resources/custom_diffs.go +++ b/pkg/resources/custom_diffs.go @@ -2,6 +2,7 @@ package resources import ( "context" + "fmt" "log" "reflect" "strconv" @@ -103,7 +104,7 @@ func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: did not create resource data correctly, skipping\n") continue } - if !diffSuppressFunc(key, oldValue.(string), newValue.(string), hackedResourceData) { + if !diffSuppressFunc(key, fmt.Sprintf("%v", oldValue), fmt.Sprintf("%v", newValue), hackedResourceData) { log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: key %s was changed and the diff is not suppressed", changedKey) result = true } else { From 82649d4a1b71124b80b85cfdf6c93366a662e292 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Tue, 3 Sep 2024 10:59:11 +0200 Subject: [PATCH 12/14] Fix saml2 tests after changes --- .../saml2_integration_acceptance_test.go | 48 +++++++++++++------ .../non_zero_values/test.tf | 11 +++++ .../non_zero_values/variables.tf | 15 ++++++ 3 files changed, 60 insertions(+), 14 deletions(-) create mode 100644 pkg/resources/testdata/TestAcc_Saml2Integration/non_zero_values/test.tf create mode 100644 pkg/resources/testdata/TestAcc_Saml2Integration/non_zero_values/variables.tf diff --git a/pkg/resources/saml2_integration_acceptance_test.go b/pkg/resources/saml2_integration_acceptance_test.go index d27061762e..352c58c884 100644 --- a/pkg/resources/saml2_integration_acceptance_test.go +++ b/pkg/resources/saml2_integration_acceptance_test.go @@ -416,7 +416,7 @@ func TestAcc_Saml2Integration_forceAuthn(t *testing.T) { }, CheckDestroy: acc.CheckDestroy(t, resources.Saml2SecurityIntegration), Steps: []resource.TestStep{ - // set up with concrete type + // set up with concrete saml2_force_authn { ConfigPlanChecks: resource.ConfigPlanChecks{ PreApply: []plancheck.PlanCheck{ @@ -432,7 +432,7 @@ func TestAcc_Saml2Integration_forceAuthn(t *testing.T) { resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.0.saml2_force_authn.0.value", "true"), ), }, - // import when type in config + // import when saml2_force_authn in config { ResourceName: "snowflake_saml2_integration.test", ImportState: true, @@ -442,7 +442,7 @@ func TestAcc_Saml2Integration_forceAuthn(t *testing.T) { importchecks.TestCheckResourceAttrInstanceState(resourcehelpers.EncodeResourceIdentifier(id), "describe_output.0.saml2_force_authn.0.value", "true"), ), }, - // change type in config + // change saml2_force_authn in config { ConfigPlanChecks: resource.ConfigPlanChecks{ PreApply: []plancheck.PlanCheck{ @@ -462,7 +462,7 @@ func TestAcc_Saml2Integration_forceAuthn(t *testing.T) { { Config: saml2ConfigWithAuthn(id.Name(), issuer, string(sdk.Saml2SecurityIntegrationSaml2ProviderCustom), validUrl, cert, true), }, - // remove non-default type from config + // remove non-default saml2_force_authn from config { Config: saml2Config(id.Name(), issuer, string(sdk.Saml2SecurityIntegrationSaml2ProviderCustom), validUrl, cert), ConfigPlanChecks: resource.ConfigPlanChecks{ @@ -479,23 +479,26 @@ func TestAcc_Saml2Integration_forceAuthn(t *testing.T) { resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.0.saml2_force_authn.0.value", "false"), ), }, - // add config + // add saml2_force_authn to config (false - which is a default in Snowflake) - no changes expected { ConfigPlanChecks: resource.ConfigPlanChecks{ PreApply: []plancheck.PlanCheck{ planchecks.PrintPlanDetails("snowflake_saml2_integration.test", "saml2_force_authn", "describe_output"), - planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String(r.BooleanDefault), sdk.String(r.BooleanDefault)), - planchecks.ExpectComputed("snowflake_saml2_integration.test", "describe_output", true), + plancheck.ExpectEmptyPlan(), }, }, Config: saml2ConfigWithAuthn(id.Name(), issuer, string(sdk.Saml2SecurityIntegrationSaml2ProviderCustom), validUrl, cert, false), Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", "false"), + resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", r.BooleanDefault), resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.#", "1"), resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.0.saml2_force_authn.0.value", "false"), ), }, - // remove type from config but update externally to default (still expecting non-empty plan because we do not know the default) + // change back to non-default + { + Config: saml2ConfigWithAuthn(id.Name(), issuer, string(sdk.Saml2SecurityIntegrationSaml2ProviderCustom), validUrl, cert, true), + }, + // remove saml2_force_authn from config but update externally to default (still expecting non-empty plan because we do not know the default) { PreConfig: func() { acc.TestClient().SecurityIntegration.UpdateSaml2ForceAuthn(t, id, false) @@ -515,7 +518,7 @@ func TestAcc_Saml2Integration_forceAuthn(t *testing.T) { resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.0.saml2_force_authn.0.value", "false"), ), }, - // change the size externally + // change the saml2_force_authn externally { PreConfig: func() { // we change the type to the type different from default, expecting action @@ -536,7 +539,7 @@ func TestAcc_Saml2Integration_forceAuthn(t *testing.T) { resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.0.saml2_force_authn.0.value", "false"), ), }, - // import when no type in config + // import when no saml2_force_authn in config { ResourceName: "snowflake_saml2_integration.test", ImportState: true, @@ -1002,15 +1005,32 @@ func TestAcc_Saml2Integration_DefaultValues(t *testing.T) { resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.0.saml2_force_authn.0.value", "false"), ), }, + // set to "non-zero" values + { + ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Saml2Integration/non_zero_values"), + ConfigVariables: configVariables, + Check: resource.ComposeAggregateTestCheckFunc( + resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "enabled", "true"), + resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_force_authn", "true"), + resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "saml2_post_logout_redirect_url", "http://example.com"), + + resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "show_output.#", "1"), + resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "show_output.0.enabled", "true"), + + resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.#", "1"), + resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.0.saml2_post_logout_redirect_url.0.value", "http://example.com"), + resource.TestCheckResourceAttr("snowflake_saml2_integration.test", "describe_output.0.saml2_force_authn.0.value", "true"), + ), + }, // add valid "zero" values again (to validate if set is run correctly) { ConfigDirectory: acc.ConfigurationDirectory("TestAcc_Saml2Integration/zero_values"), ConfigVariables: configVariables, ConfigPlanChecks: resource.ConfigPlanChecks{ PreApply: []plancheck.PlanCheck{ - planchecks.ExpectChange("snowflake_saml2_integration.test", "enabled", tfjson.ActionUpdate, sdk.String(r.BooleanDefault), sdk.String(r.BooleanDefault)), - planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String(r.BooleanDefault), sdk.String(r.BooleanDefault)), - planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_post_logout_redirect_url", tfjson.ActionUpdate, sdk.String(""), sdk.String("")), + planchecks.ExpectChange("snowflake_saml2_integration.test", "enabled", tfjson.ActionUpdate, sdk.String(r.BooleanTrue), sdk.String(r.BooleanFalse)), + planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_force_authn", tfjson.ActionUpdate, sdk.String(r.BooleanTrue), sdk.String(r.BooleanFalse)), + planchecks.ExpectChange("snowflake_saml2_integration.test", "saml2_post_logout_redirect_url", tfjson.ActionUpdate, sdk.String("http://example.com"), sdk.String("")), planchecks.ExpectComputed("snowflake_saml2_integration.test", "show_output", true), planchecks.ExpectComputed("snowflake_saml2_integration.test", "describe_output", true), }, diff --git a/pkg/resources/testdata/TestAcc_Saml2Integration/non_zero_values/test.tf b/pkg/resources/testdata/TestAcc_Saml2Integration/non_zero_values/test.tf new file mode 100644 index 0000000000..7cc95b2ebe --- /dev/null +++ b/pkg/resources/testdata/TestAcc_Saml2Integration/non_zero_values/test.tf @@ -0,0 +1,11 @@ +resource "snowflake_saml2_integration" "test" { + name = var.name + saml2_issuer = var.saml2_issuer + saml2_sso_url = var.saml2_sso_url + saml2_provider = var.saml2_provider + saml2_x509_cert = var.saml2_x509_cert + + enabled = true + saml2_force_authn = true + saml2_post_logout_redirect_url = "http://example.com" +} diff --git a/pkg/resources/testdata/TestAcc_Saml2Integration/non_zero_values/variables.tf b/pkg/resources/testdata/TestAcc_Saml2Integration/non_zero_values/variables.tf new file mode 100644 index 0000000000..351581f56c --- /dev/null +++ b/pkg/resources/testdata/TestAcc_Saml2Integration/non_zero_values/variables.tf @@ -0,0 +1,15 @@ +variable "name" { + type = string +} +variable "saml2_issuer" { + type = string +} +variable "saml2_provider" { + type = string +} +variable "saml2_sso_url" { + type = string +} +variable "saml2_x509_cert" { + type = string +} From 4b5dc1050ea13657ce1f69eb264ef0de2b8bcc48 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Tue, 3 Sep 2024 12:16:40 +0200 Subject: [PATCH 13/14] Fix warehouse and user --- pkg/resources/user.go | 3 ++- pkg/resources/warehouse_acceptance_test.go | 30 ++-------------------- 2 files changed, 4 insertions(+), 29 deletions(-) diff --git a/pkg/resources/user.go b/pkg/resources/user.go index 58af3bae16..eebb3dbf5e 100644 --- a/pkg/resources/user.go +++ b/pkg/resources/user.go @@ -200,7 +200,8 @@ func User() *schema.Resource { CustomizeDiff: customdiff.All( // TODO [SNOW-1629468 - next pr]: test "default_role", "default_secondary_roles" - ComputedIfAnyAttributeChanged(userSchema, ShowOutputAttributeName, "password", "login_name", "display_name", "first_name", "middle_name", "last_name", "email", "must_change_password", "disabled", "days_to_expiry", "mins_to_unlock", "default_warehouse", "default_namespace", "default_role", "default_secondary_roles", "mins_to_bypass_mfa", "rsa_public_key", "rsa_public_key_2", "comment", "disable_mfa"), + // TODO [SNOW-TODO]: "default_secondary_roles" have to stay commented out because of how the SDKv2 handles diff suppressions and custom diffs for sets + ComputedIfAnyAttributeChanged(userSchema, ShowOutputAttributeName, "password", "login_name", "display_name", "first_name", "middle_name", "last_name", "email", "must_change_password", "disabled", "days_to_expiry", "mins_to_unlock", "default_warehouse", "default_namespace", "default_role", "mins_to_bypass_mfa", "rsa_public_key", "rsa_public_key_2", "comment", "disable_mfa"), ComputedIfAnyAttributeChanged(userParametersSchema, ParametersAttributeName, collections.Map(sdk.AsStringList(sdk.AllUserParameters), strings.ToLower)...), ComputedIfAnyAttributeChanged(userSchema, FullyQualifiedNameAttributeName, "name"), userParametersCustomDiff, diff --git a/pkg/resources/warehouse_acceptance_test.go b/pkg/resources/warehouse_acceptance_test.go index 3e96684a7b..1d737d39cd 100644 --- a/pkg/resources/warehouse_acceptance_test.go +++ b/pkg/resources/warehouse_acceptance_test.go @@ -188,41 +188,15 @@ func TestAcc_Warehouse_BasicFlows(t *testing.T) { resource.TestCheckResourceAttr("snowflake_warehouse.w", "fully_qualified_name", warehouseId2.FullyQualifiedName()), ), }, - // Change config but use defaults for every attribute (but not the parameters) - expect no changes (because these are already SF values) except computed show_output (follow-up why suppress diff is not taken into account in has changes?) + // Change config but use defaults for every attribute (but not the parameters) - expect no changes (because these are already SF values) { ConfigPlanChecks: resource.ConfigPlanChecks{ PreApply: []plancheck.PlanCheck{ planchecks.PrintPlanDetails("snowflake_warehouse.w", "warehouse_type", "warehouse_size", "max_cluster_count", "min_cluster_count", "scaling_policy", "auto_suspend", "auto_resume", "enable_query_acceleration", "query_acceleration_max_scale_factor", "max_concurrency_level", "statement_queued_timeout_in_seconds", "statement_timeout_in_seconds", r.ShowOutputAttributeName), - plancheck.ExpectResourceAction("snowflake_warehouse.w", plancheck.ResourceActionUpdate), - planchecks.ExpectComputed("snowflake_warehouse.w", r.ShowOutputAttributeName, true), + plancheck.ExpectEmptyPlan(), }, }, Config: warehouseFullDefaultWithoutParametersConfig(name2, comment), - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr("snowflake_warehouse.w", "warehouse_type", string(sdk.WarehouseTypeStandard)), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "warehouse_size", string(sdk.WarehouseSizeXSmall)), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "max_cluster_count", "1"), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "min_cluster_count", "1"), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "scaling_policy", string(sdk.ScalingPolicyStandard)), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "auto_suspend", "600"), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "auto_resume", "true"), - resource.TestCheckNoResourceAttr("snowflake_warehouse.w", "initially_suspended"), - resource.TestCheckNoResourceAttr("snowflake_warehouse.w", "resource_monitor"), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "comment", comment), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "enable_query_acceleration", "false"), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "query_acceleration_max_scale_factor", "8"), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "max_concurrency_level", "8"), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "statement_queued_timeout_in_seconds", "0"), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "statement_timeout_in_seconds", "172800"), - - resource.TestCheckResourceAttr("snowflake_warehouse.w", "parameters.#", "1"), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "parameters.0.max_concurrency_level.0.value", "8"), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "parameters.0.max_concurrency_level.0.level", ""), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "parameters.0.statement_queued_timeout_in_seconds.0.value", "0"), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "parameters.0.statement_queued_timeout_in_seconds.0.level", ""), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "parameters.0.statement_timeout_in_seconds.0.value", "172800"), - resource.TestCheckResourceAttr("snowflake_warehouse.w", "parameters.0.statement_timeout_in_seconds.0.level", ""), - ), }, // add parameters - update expected (different level even with same values) { From 0d9c1b09d9bf80de92b8c6001d6ecb704b7bbd71 Mon Sep 17 00:00:00 2001 From: Artur Sawicki Date: Tue, 3 Sep 2024 14:40:40 +0200 Subject: [PATCH 14/14] Fix tests --- .../sdkv2enhancements/resource_data.go | 36 +++++++++++++++++++ pkg/resources/custom_diffs.go | 31 +++------------- pkg/resources/warehouse_acceptance_test.go | 34 ++++++++++++++++++ 3 files changed, 74 insertions(+), 27 deletions(-) create mode 100644 pkg/internal/provider/sdkv2enhancements/resource_data.go diff --git a/pkg/internal/provider/sdkv2enhancements/resource_data.go b/pkg/internal/provider/sdkv2enhancements/resource_data.go new file mode 100644 index 0000000000..c2f450c908 --- /dev/null +++ b/pkg/internal/provider/sdkv2enhancements/resource_data.go @@ -0,0 +1,36 @@ +package sdkv2enhancements + +import ( + "reflect" + "unsafe" + + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" +) + +// CreateResourceDataFromResourceDiff allows to create schema.ResourceData out of schema.ResourceDiff. +// Unfortunately, schema.resourceDiffer is an unexported interface, and functions like schema.SchemaDiffSuppressFunc do not use the interface but a concrete implementation. +// One use case in which we needed to have schema.ResourceData from schema.ResourceDiff was to run schema.SchemaDiffSuppressFunc from inside schema.CustomizeDiffFunc. +// This implementation uses: +// - schema.InternalMap that exposes hidden schema.schemaMap (a wrapper over map[string]*schema.Schema) +// - (m schemaMap) Data method allowing to create schema.ResourceData from terraform.InstanceState and terraform.InstanceDiff +// - terraform.InstanceState and terraform.InstanceDiff are unexported in schema.ResourceDiff, so we get them using reflection +func CreateResourceDataFromResourceDiff(resourceSchema schema.InternalMap, diff *schema.ResourceDiff) (*schema.ResourceData, bool) { + unexportedState := reflect.ValueOf(diff).Elem().FieldByName("state") + stateFromResourceDiff := reflect.NewAt(unexportedState.Type(), unsafe.Pointer(unexportedState.UnsafeAddr())).Elem().Interface() + unexportedDiff := reflect.ValueOf(diff).Elem().FieldByName("diff") + diffFroResourceDif := reflect.NewAt(unexportedDiff.Type(), unsafe.Pointer(unexportedDiff.UnsafeAddr())).Elem().Interface() + castState, ok := stateFromResourceDiff.(*terraform.InstanceState) + if !ok { + return nil, false + } + castDiff, ok := diffFroResourceDif.(*terraform.InstanceDiff) + if !ok { + return nil, false + } + resourceData, err := resourceSchema.Data(castState, castDiff) + if err != nil { + return nil, false + } + return resourceData, true +} diff --git a/pkg/resources/custom_diffs.go b/pkg/resources/custom_diffs.go index e807127807..a10c78d4d6 100644 --- a/pkg/resources/custom_diffs.go +++ b/pkg/resources/custom_diffs.go @@ -4,16 +4,14 @@ import ( "context" "fmt" "log" - "reflect" "strconv" "strings" - "unsafe" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/collections" + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/internal/provider/sdkv2enhancements" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" - "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" ) func StringParameterValueComputedIf[T ~string](key string, params []*sdk.Parameter, parameterLevel sdk.ParameterType, parameter T) schema.CustomizeDiffFunc { @@ -99,12 +97,12 @@ func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key if v, ok := resourceSchema[changedKey]; ok { if diffSuppressFunc := v.DiffSuppressFunc; diffSuppressFunc != nil { - hackedResourceData, hackedCorrectly := hackResourceData(resourceSchema, diff) - if !hackedCorrectly { + resourceData, resourceDataOk := sdkv2enhancements.CreateResourceDataFromResourceDiff(resourceSchema, diff) + if !resourceDataOk { log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: did not create resource data correctly, skipping\n") continue } - if !diffSuppressFunc(key, fmt.Sprintf("%v", oldValue), fmt.Sprintf("%v", newValue), hackedResourceData) { + if !diffSuppressFunc(key, fmt.Sprintf("%v", oldValue), fmt.Sprintf("%v", newValue), resourceData) { log.Printf("[DEBUG] ComputedIfAnyAttributeChanged: key %s was changed and the diff is not suppressed", changedKey) result = true } else { @@ -121,27 +119,6 @@ func ComputedIfAnyAttributeChanged(resourceSchema map[string]*schema.Schema, key }) } -// TODO: extract hacky stuff outside this directory? -func hackResourceData(resourceSchema schema.InternalMap, diff *schema.ResourceDiff) (*schema.ResourceData, bool) { - unexportedState := reflect.ValueOf(diff).Elem().FieldByName("state") - hackedState := reflect.NewAt(unexportedState.Type(), unsafe.Pointer(unexportedState.UnsafeAddr())).Elem().Interface() - unexportedDiff := reflect.ValueOf(diff).Elem().FieldByName("diff") - hackedDiff := reflect.NewAt(unexportedDiff.Type(), unsafe.Pointer(unexportedDiff.UnsafeAddr())).Elem().Interface() - castState, ok := hackedState.(*terraform.InstanceState) - if !ok { - return nil, false - } - castDiff, ok := hackedDiff.(*terraform.InstanceDiff) - if !ok { - return nil, false - } - hackedResourceData, err := resourceSchema.Data(castState, castDiff) - if err != nil { - return nil, false - } - return hackedResourceData, true -} - type parameter[T ~string] struct { parameterName T valueType valueType diff --git a/pkg/resources/warehouse_acceptance_test.go b/pkg/resources/warehouse_acceptance_test.go index 1d737d39cd..2d49146eed 100644 --- a/pkg/resources/warehouse_acceptance_test.go +++ b/pkg/resources/warehouse_acceptance_test.go @@ -3,6 +3,7 @@ package resources_test import ( "fmt" "regexp" + "strconv" "strings" "testing" @@ -290,6 +291,15 @@ func TestAcc_Warehouse_BasicFlows(t *testing.T) { resource.TestCheckResourceAttr("snowflake_warehouse.w", "parameters.0.statement_timeout_in_seconds.0.level", string(sdk.ParameterTypeWarehouse)), ), }, + // change resource monitor - wrap in quotes (no change expected) + { + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectEmptyPlan(), + }, + }, + Config: warehouseFullConfigNoDefaultsStringId(name2, newComment, strconv.Quote(resourceMonitorId.FullyQualifiedName())), + }, // CHANGE max_concurrency_level EXTERNALLY (proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2318) { PreConfig: func() { acc.TestClient().Warehouse.UpdateMaxConcurrencyLevel(t, warehouseId2, 10) }, @@ -2110,6 +2120,30 @@ resource "snowflake_warehouse" "w" { `, name, comment, id.Name()) } +func warehouseFullConfigNoDefaultsStringId(name string, comment string, id string) string { + return fmt.Sprintf(` +resource "snowflake_warehouse" "w" { + name = "%[1]s" + warehouse_type = "SNOWPARK-OPTIMIZED" + warehouse_size = "MEDIUM" + max_cluster_count = 4 + min_cluster_count = 2 + scaling_policy = "ECONOMY" + auto_suspend = 1200 + auto_resume = false + initially_suspended = false + resource_monitor = %[3]s + comment = "%[2]s" + enable_query_acceleration = true + query_acceleration_max_scale_factor = 4 + + max_concurrency_level = 4 + statement_queued_timeout_in_seconds = 5 + statement_timeout_in_seconds = 86400 +} +`, name, comment, id) +} + func warehouseWithSizeConfig(name string, size string) string { return fmt.Sprintf(` resource "snowflake_warehouse" "w" {