Skip to content

Commit

Permalink
chore: Metric level fix with object renaming guide (#3376)
Browse files Browse the repository at this point in the history
## Changes
- Add missing support for metric level account parameter #3375
- Add object renaming guide
  • Loading branch information
sfc-gh-jcieslak authored Feb 4, 2025
1 parent f23e8cb commit 629ff92
Show file tree
Hide file tree
Showing 9 changed files with 196 additions and 14 deletions.
3 changes: 3 additions & 0 deletions MIGRATION_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ across different versions.
## v1.0.2 ➞ v1.0.3

### Fixed METRIC_LEVEL parameter
METRIC_LEVEL account parameter did not work correctly before ([#3375](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/3375)), but was fixed in this version.

### Fixed ENFORCE_NETWORK_RULES_FOR_INTERNAL_STAGES parameter
ENFORCE_NETWORK_RULES_FOR_INTERNAL_STAGES account parameter did not work correctly before ([#3344]). This parameter was of incorrect type, and the constructed queries did not provide the parameter's value during altering accounts. It has been fixed in this version.

Expand Down
2 changes: 1 addition & 1 deletion docs/guides/identifiers_rework_design_decisions.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ The diff suppression should make those rules irrelevant whenever identifiers in
### New computed fully qualified name field in resources
With the combination of quotes, old parsing methods, and other factors, it was a struggle to specify the fully qualified name of an object needed (e.g. [#2164](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2164), [#2754](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2754)).
Now, with v0.95.0, every resource that represents an object in Snowflake (e.g. user, role), and not an association (e.g. grants) will have a new computed field named `fully_qualified_name`.
With the new computed field, it will be much easier to use resources requiring fully qualified names, for examples of usage head over to the [documentation for granting privileges to account role](../docs/resources/grant_privileges_to_account_role).
With the new computed field, it will be much easier to use resources requiring fully qualified names, for examples of usage head over to the [documentation for granting privileges to account role](../resources/grant_privileges_to_account_role).

For example, instead of writing

Expand Down
75 changes: 75 additions & 0 deletions docs/guides/object_renaming_guide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
---
page_title: "Object Renaming Guide"
subcategory: ""
description: |-
---

# Object Renaming Guide

Recently, we conducted research on object renaming and published a document summarizing the results.
To leverage the knowledge we gained from this research, we wanted to provide a follow-up document that would help you understand the current best practices for tackling object renaming-related topics.
In this document, we propose recommendations and solutions for the issues identified through our research, as well as those previously reported in our GitHub repository.

## Renaming objects in the hierarchy

This problem relates to renaming objects that are higher in the object hierarchy (e.g. database or schema) and how this affects the lower hierarchy objects created on them (e.g. schema or table) while they are present in the Terraform configuration.
In our research, we tested different sets of configurations described [here](./object_renaming_research_summary#renaming-higher-hierarchy-objects).

### Recommendations

For now, the only recommendation that shows real improvements is to keep your object in correct relations. Use the following order:
- [Implicit dependency](https://developer.hashicorp.com/terraform/tutorials/configuration-language/dependencies#manage-implicit-dependencies)
- [Explicit dependency (depends_on)](https://developer.hashicorp.com/terraform/tutorials/configuration-language/dependencies#manage-explicit-dependencies)
- No dependency

Currently, we do not support object renaming within hierarchies.
However, we are planning to make a follow-up research that would enable it.
If the research confirms that we will be able to implement it, and we decide to do so, maintaining the correct resource structure will not only be advisable but essential.
It will be crucial for accurately determining the appropriate actions a resource should take when a high-level object is renamed.

If you really need to perform, for example, a database rename with other resources referencing its name, you can first remove the dependent objects from the state.
Then, perform the actual rename, and after that, you can import the dependent objects back to the state, but with a different database.
This is very time-consuming, so only consider this when the number of objects dependent on the object you want to rename is low.
To see more or less how this could be implemented, take a look at the [migration guide](./resource_migration) we already described which has similar steps of execution.

### Future plans

In addition to the plans described in the [research summary](./object_renaming_research_summary#renaming-higher-hierarchy-objects), we would like to research what resources will be needed to handle high-level object renames in the future.
The problem right now is that lower-level objects have fields that reference higher-level objects with the ForceNew option.
A solution would be to remove this parameter and handle certain situations differently.
The new solution should provide an easier way to conclude high-level object renames with our provider.

## Issues with lists and sets

Currently, we have limited capabilities when it comes to certain operations on lists and sets.
An example of such a limitation could be detecting whether a collection item was updated or one item was removed and the new one was put in its place.
This is mainly due to how the Terraform SDKv2 handles changes for collections.
So far, the most challenging case was columns on tables, as Snowflake has its own limitations preventing us from reaching the correct state.
Here are some of the issues pointing to the limitations we are talking about:
- [terraform-plugin-sdk#133](https://github.com/hashicorp/terraform-plugin-sdk/issues/133)
- [terraform-plugin-sdk#196](https://github.com/hashicorp/terraform-plugin-sdk/issues/196) (this is regarding the testing framework, but the issue persists on the provider-level code as well)
- [terraform-plugin-sdk#447](https://github.com/hashicorp/terraform-plugin-sdk/issues/447)
- [terraform-plugin-sdk#1103](https://github.com/hashicorp/terraform-plugin-sdk/issues/1103)

There is more, but the real issue is that those problems overlap, making it really difficult to provide any custom functionality that wasn’t considered when designing the Terraform SDKv2.

### Recommendations

It's important to align your needs with the capabilities of the provider's resources and choose the appropriate tool for the task.
This is particularly crucial for lower-level objects like tables, which are subject to frequent changes and may pose challenges when being provisioned in Terraform.
Tables are unique as they are infrastructure objects that contain data, so modifications need to be considered carefully.
Due to current limitations, it might be impractical to provision tables with the provider, as some table parameter changes require dropping and recreating the table, resulting in data loss.
In Terraform, this approach is common to ultimately achieve the desired infrastructure state with the specified objects.
After the research, we have some upcoming improvements in handling changes in lists and sets, but they won’t resolve all the issues, and the above remains.

### Future plans

As mentioned in the [research summary](./object_renaming_research_summary#ignoring-list-order-after-creation--updating-list-items-mostly-related-to-table-columns), we plan to improve the table resource with all the findings, which will mostly affect the list of columns and how we detect/plan changes for them.
Once implemented, all the details will be available in the documentation for the table resource and in the [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md).

## Summary

We hope that the additional recommendations derived from our research will assist you in making informed decisions regarding the use of our provider.
If you have any questions or need further clarification, we encourage you to create issues in our [GitHub repository](https://github.com/Snowflake-Labs/terraform-provider-snowflake).
Your feedback is invaluable and will contribute to further improving our documentation.
2 changes: 1 addition & 1 deletion pkg/internal/tracking/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (

const (
CurrentSchemaVersion string = "1"
ProviderVersion string = "v1.0.2" // TODO(SNOW-1814934): Currently hardcoded, make it computed
ProviderVersion string = "v1.0.3" // TODO(SNOW-1814934): Currently hardcoded, make it computed
MetadataPrefix string = "terraform_provider_usage_tracking"
)

Expand Down
22 changes: 22 additions & 0 deletions pkg/resources/account_parameter_acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,4 +173,26 @@ func TestAcc_AccountParameter_INITIAL_REPLICATION_SIZE_LIMIT_IN_TB(t *testing.T)
})
}

// Proves https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/3375 is fixed
func TestAcc_AccountParameter_METRIC_LEVEL(t *testing.T) {
model := model.AccountParameter("test", string(sdk.AccountParameterMetricLevel), string(sdk.MetricLevelAll))
resource.Test(t, resource.TestCase{
ProtoV6ProviderFactories: acc.TestAccProtoV6ProviderFactories,
PreCheck: func() { acc.TestAccPreCheck(t) },
TerraformVersionChecks: []tfversion.TerraformVersionCheck{
tfversion.RequireAbove(tfversion.Version1_5_0),
},
CheckDestroy: acc.CheckAccountParameterUnset(t, sdk.AccountParameterMetricLevel),
Steps: []resource.TestStep{
{
Config: config.FromModels(t, model),
Check: assert.AssertThat(t, resourceassert.AccountParameterResource(t, model.ResourceReference()).
HasKeyString(string(sdk.AccountParameterMetricLevel)).
HasValueString(string(sdk.MetricLevelAll)),
),
},
},
})
}

// TODO(SNOW-1866453): add more acc tests for the remaining parameters
28 changes: 17 additions & 11 deletions pkg/sdk/parameters.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ func (parameters *parameters) SetAccountParameter(ctx context.Context, parameter
return fmt.Errorf("MIN_DATA_RETENTION_TIME_IN_DAYS session parameter is an integer, got %v", value)
}
opts.Set.Parameters.AccountParameters.MinDataRetentionTimeInDays = Pointer(v)
case AccountParameterMetricLevel:
opts.Set.Parameters.AccountParameters.MetricLevel = Pointer(MetricLevel(value))
case AccountParameterNetworkPolicy:
opts.Set.Parameters.AccountParameters.NetworkPolicy = &value
case AccountParameterOAuthAddPrivilegedRolesToBlockedList:
Expand Down Expand Up @@ -233,6 +235,8 @@ func (parameters *parameters) UnsetAccountParameter(ctx context.Context, paramet
opts.Unset.Parameters.AccountParameters.InitialReplicationSizeLimitInTB = Pointer(true)
case AccountParameterMinDataRetentionTimeInDays:
opts.Unset.Parameters.AccountParameters.MinDataRetentionTimeInDays = Pointer(true)
case AccountParameterMetricLevel:
opts.Unset.Parameters.AccountParameters.MetricLevel = Pointer(true)
case AccountParameterNetworkPolicy:
opts.Unset.Parameters.AccountParameters.NetworkPolicy = Pointer(true)
case AccountParameterOAuthAddPrivilegedRolesToBlockedList:
Expand Down Expand Up @@ -1191,17 +1195,18 @@ type AccountParameters struct {
ExternalOAuthAddPrivilegedRolesToBlockedList *bool `ddl:"parameter" sql:"EXTERNAL_OAUTH_ADD_PRIVILEGED_ROLES_TO_BLOCKED_LIST"`
// InitialReplicationSizeLimitInTB is a string because values like 3.0 get rounded to 3, resulting in an error in Snowflake.
// This is still validated below.
InitialReplicationSizeLimitInTB *string `ddl:"parameter" sql:"INITIAL_REPLICATION_SIZE_LIMIT_IN_TB"`
MinDataRetentionTimeInDays *int `ddl:"parameter" sql:"MIN_DATA_RETENTION_TIME_IN_DAYS"`
NetworkPolicy *string `ddl:"parameter,single_quotes" sql:"NETWORK_POLICY"`
OAuthAddPrivilegedRolesToBlockedList *bool `ddl:"parameter" sql:"OAUTH_ADD_PRIVILEGED_ROLES_TO_BLOCKED_LIST"`
PeriodicDataRekeying *bool `ddl:"parameter" sql:"PERIODIC_DATA_REKEYING"`
PreventLoadFromInlineURL *bool `ddl:"parameter" sql:"PREVENT_LOAD_FROM_INLINE_URL"`
PreventUnloadToInlineURL *bool `ddl:"parameter" sql:"PREVENT_UNLOAD_TO_INLINE_URL"`
PreventUnloadToInternalStages *bool `ddl:"parameter" sql:"PREVENT_UNLOAD_TO_INTERNAL_STAGES"`
RequireStorageIntegrationForStageCreation *bool `ddl:"parameter" sql:"REQUIRE_STORAGE_INTEGRATION_FOR_STAGE_CREATION"`
RequireStorageIntegrationForStageOperation *bool `ddl:"parameter" sql:"REQUIRE_STORAGE_INTEGRATION_FOR_STAGE_OPERATION"`
SSOLoginPage *bool `ddl:"parameter" sql:"SSO_LOGIN_PAGE"`
InitialReplicationSizeLimitInTB *string `ddl:"parameter" sql:"INITIAL_REPLICATION_SIZE_LIMIT_IN_TB"`
MetricLevel *MetricLevel `ddl:"parameter" sql:"METRIC_LEVEL"`
MinDataRetentionTimeInDays *int `ddl:"parameter" sql:"MIN_DATA_RETENTION_TIME_IN_DAYS"`
NetworkPolicy *string `ddl:"parameter,single_quotes" sql:"NETWORK_POLICY"`
OAuthAddPrivilegedRolesToBlockedList *bool `ddl:"parameter" sql:"OAUTH_ADD_PRIVILEGED_ROLES_TO_BLOCKED_LIST"`
PeriodicDataRekeying *bool `ddl:"parameter" sql:"PERIODIC_DATA_REKEYING"`
PreventLoadFromInlineURL *bool `ddl:"parameter" sql:"PREVENT_LOAD_FROM_INLINE_URL"`
PreventUnloadToInlineURL *bool `ddl:"parameter" sql:"PREVENT_UNLOAD_TO_INLINE_URL"`
PreventUnloadToInternalStages *bool `ddl:"parameter" sql:"PREVENT_UNLOAD_TO_INTERNAL_STAGES"`
RequireStorageIntegrationForStageCreation *bool `ddl:"parameter" sql:"REQUIRE_STORAGE_INTEGRATION_FOR_STAGE_CREATION"`
RequireStorageIntegrationForStageOperation *bool `ddl:"parameter" sql:"REQUIRE_STORAGE_INTEGRATION_FOR_STAGE_OPERATION"`
SSOLoginPage *bool `ddl:"parameter" sql:"SSO_LOGIN_PAGE"`
}

func (v *AccountParameters) validate() error {
Expand Down Expand Up @@ -1242,6 +1247,7 @@ type AccountParametersUnset struct {
ExternalOAuthAddPrivilegedRolesToBlockedList *bool `ddl:"keyword" sql:"EXTERNAL_OAUTH_ADD_PRIVILEGED_ROLES_TO_BLOCKED_LIST"`
InitialReplicationSizeLimitInTB *bool `ddl:"keyword" sql:"INITIAL_REPLICATION_SIZE_LIMIT_IN_TB"`
MinDataRetentionTimeInDays *bool `ddl:"keyword" sql:"MIN_DATA_RETENTION_TIME_IN_DAYS"`
MetricLevel *bool `ddl:"keyword" sql:"METRIC_LEVEL"`
NetworkPolicy *bool `ddl:"keyword" sql:"NETWORK_POLICY"`
OAuthAddPrivilegedRolesToBlockedList *bool `ddl:"keyword" sql:"OAUTH_ADD_PRIVILEGED_ROLES_TO_BLOCKED_LIST"`
PeriodicDataRekeying *bool `ddl:"keyword" sql:"PERIODIC_DATA_REKEYING"`
Expand Down
1 change: 1 addition & 0 deletions pkg/sdk/parameters_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ func TestToAccountParameter(t *testing.T) {
{input: "EXTERNAL_OAUTH_ADD_PRIVILEGED_ROLES_TO_BLOCKED_LIST", want: AccountParameterExternalOAuthAddPrivilegedRolesToBlockedList},
{input: "INITIAL_REPLICATION_SIZE_LIMIT_IN_TB", want: AccountParameterInitialReplicationSizeLimitInTB},
{input: "MIN_DATA_RETENTION_TIME_IN_DAYS", want: AccountParameterMinDataRetentionTimeInDays},
{input: "METRIC_LEVEL", want: AccountParameterMetricLevel},
{input: "NETWORK_POLICY", want: AccountParameterNetworkPolicy},
{input: "OAUTH_ADD_PRIVILEGED_ROLES_TO_BLOCKED_LIST", want: AccountParameterOAuthAddPrivilegedRolesToBlockedList},
{input: "PERIODIC_DATA_REKEYING", want: AccountParameterPeriodicDataRekeying},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ The diff suppression should make those rules irrelevant whenever identifiers in
### New computed fully qualified name field in resources
With the combination of quotes, old parsing methods, and other factors, it was a struggle to specify the fully qualified name of an object needed (e.g. [#2164](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2164), [#2754](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2754)).
Now, with v0.95.0, every resource that represents an object in Snowflake (e.g. user, role), and not an association (e.g. grants) will have a new computed field named `fully_qualified_name`.
With the new computed field, it will be much easier to use resources requiring fully qualified names, for examples of usage head over to the [documentation for granting privileges to account role](../docs/resources/grant_privileges_to_account_role).
With the new computed field, it will be much easier to use resources requiring fully qualified names, for examples of usage head over to the [documentation for granting privileges to account role](../resources/grant_privileges_to_account_role).

For example, instead of writing

Expand Down
Loading

0 comments on commit 629ff92

Please sign in to comment.