From f5d36236543b74a2b47d5714de4982292946efe1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Cie=C5=9Blak?= Date: Wed, 21 Aug 2024 14:08:08 +0200 Subject: [PATCH 1/5] Add identifiers rework summary and address TODOs --- CREATING_ISSUES.md | 7 ++ .../identifiers_rework_design_decisions.md | 70 +++++++++++++++++++ .../security_integrations_acceptance_test.go | 4 +- pkg/helpers/helpers.go | 2 +- pkg/resources/account_role.go | 7 +- pkg/resources/common.go | 1 - .../deprecated_identifier_helpers.go | 4 +- v1-preparations/CHANGES_BEFORE_V1.md | 4 ++ 8 files changed, 89 insertions(+), 10 deletions(-) create mode 100644 docs/technical-documentation/identifiers_rework_design_decisions.md diff --git a/CREATING_ISSUES.md b/CREATING_ISSUES.md index 5e4b468f44..a9af1ef6eb 100644 --- a/CREATING_ISSUES.md +++ b/CREATING_ISSUES.md @@ -12,6 +12,8 @@ * [What are the current/future plans for the provider?](#what-are-the-currentfuture-plans-for-the-provider) * [How can I contribute?](#how-can-i-contribute) * [How can I debug the issue myself?](#how-can-i-debug-the-issue-myself) + * [How can I import already existing Snowflake infrastructure into Terraform?](#how-can-i-import-already-existing-snowflake-infrastructure-into-terraform) + * [What identifiers are valid inside the provider and how to reference one resource inside the other one?](#what-identifiers-are-valid-inside-the-provider-and-how-to-reference-one-resource-inside-the-other-one) * [Commonly known issues](#commonly-known-issues) * [Old Terraform CLI version](#old-terraform-cli-version) * [Errors with connection to Snowflake](#errors-with-connection-to-snowflake) @@ -96,6 +98,11 @@ as it describes different approaches of importing the existing Snowflake infrast One thing worth noting is that some approaches can be automated by scripts interacting with Snowflake and generating needed configuration blocks, which is highly recommended for large-scale migrations. +### What identifiers are valid inside the provider and how to reference one resource inside the other one? +Please refer to [this document](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/identifiers_rework_design_decisions.md) +- For the recommended identifier format, take a look at the "Known limitations and identifier recommendations" section. +- For a new way of referencing object identifiers in resources, take a look at the "New computed fully qualified name field in resources" section. + ## Commonly known issues ### Old Terraform CLI version **Problem:** Sometimes you can get errors similar to: diff --git a/docs/technical-documentation/identifiers_rework_design_decisions.md b/docs/technical-documentation/identifiers_rework_design_decisions.md new file mode 100644 index 0000000000..af655f7fc7 --- /dev/null +++ b/docs/technical-documentation/identifiers_rework_design_decisions.md @@ -0,0 +1,70 @@ + +# Identifiers rework + +This document summarises work done in the [identifiers rework](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#identifiers-rework) and future plans for further identifier improvements. +But before we dive into results and design decisions, here’s the list of reasons why we decided to rework the identifiers in the first place: +- Common issues with identifiers with arguments (identifiers for functions, procedures, and external functions). +- Meaningless error messages whenever an invalid identifier is specified. +- Inconsistencies in quotes causing differences in Terraform plans. +- The inconvenience of specifying fully qualified names in certain resource fields (e.g. object name in privilege-granting resources). +- Mixed usage of account identifier formats across resources. +Now, knowing the issues we wanted to solve, we would like to present the changes and design decisions we made. + +## Topics + +### New identifier parser +To resolve many of our underlying problems with parsing identifiers, we decided to go with the new one that will be able to correctly parse fully qualified names of objects. +In addition to a better parsing function, we made sure it will return user-friendly error messages that will be able to find the root cause of a problem when specifying invalid identifiers. +Previously, the error looked like [this](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2091). + +### Using the recommended format for account identifiers +Previously, the use of account identifiers was mixed across the resources, in many cases causing confusion ([commonly known issues reference](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CREATING_ISSUES.md#incorrect-account-identifier-snowflake_databasefrom_share)). +Some of them required an account locator format (that was not fully supported and is currently deprecated), and some of the new recommended ones. +We decided to unify them and use the new account identifier format everywhere. + +### Better handling for identifiers with arguments +Previously, the handling of identifiers with arguments was not done fully correctly, causing many issues and confusion on how to use them ([commonly known issues reference](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CREATING_ISSUES.md#granting-on-functions-or-procedures)). +The main pain point was using them with privilege-granting resources. To address this we had to make two steps. +The first one was adding a dedicated representation of an identifier containing arguments and using it in our SDK. +The second one was additional parsing for the output of SHOW GRANTS in our SDK which was only necessary for functions, +procedures, and external functions that returned non-valid identifier formats. + +### Quoting differences +There are many reported issues on identifier quoting and how it is inconsistent across resources and causes plan diffs to enforce certain format (e.g. [#2982](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2982), [#2236](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2236)). +To address that, we decided to add diff suppress on identifier fields that ignore changes related to differences in quotes. +The main root cause of such differences was that Snowflake has specific rules when a given identifier (or part of an identifier) is quoted and when it’s not. +The diff suppression should make those rules irrelevant whenever identifiers in your Terraform configuration contain quotes or not. + +### 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](https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs/resources/grant_privileges_to_account_role). + +### New resource identifier format +This will be a small shift in the identifier representation for resources. The general rule will be now that: +- If a resource can only be described with the Snowflake identifier, the fully qualified name will be put into the resource identifier. Previously, it was almost the same, except it was separated by pipes, and it was not a valid identifier. +- If a resource cannot be described only by a single Snowflake identifier, then the resource identifier will be a pipe-separated text of all parts needed to identify a given resource ([example](https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs/resources/grant_privileges_to_account_role#import)). Mind that this approach is not compliant with identifiers containing pipes, but this approach is a middle ground between an easy-to-specify separator and a character that shouldn’t be that common in the identifier (it was previously used for all identifiers). + +Known limitations and identifier recommendations +The main limitations around identifiers are strictly connected to what characters are used. Here’s a list of recommendations on which characters should be generally avoided when specifying identifiers: +Avoid dots ‘.’ inside identifiers. It’s the main separator between identifier parts and although we are handling dots inside identifiers, there may be cases where it’s impossible to parse the identifier correctly. +Avoid pipes ‘|’ inside identifiers. It’s the separator for our more complex resource identifiers that could make our parser split the resource identifier into the wrong parts. +Avoid parentheses ‘(’ and ‘)’ when specifying identifiers for functions, procedures, external functions. Parentheses as part of their identifiers could potentially make our parser split the identifier into wrong parts causing issues. +As a general recommendation, please lean toward simple names without any special characters, and if word separation is needed, use underscores. This also applies to other “identifiers” like column names in tables or argument names in functions. If you are currently using complex identifiers, we recommend considering migration to simpler identifiers for a more straightforward and less error-prone experience. + +### New identifier conventions +Although, we are closing the identifiers rework, some resources won’t have the mentioned improvements. +They were mostly applied to the objects that were already prepared for v1 ([essential objects](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/v1-preparations/ESSENTIAL_GA_OBJECTS.MD)). +The remaining resources (and newly created ones) will receive these improvements [during v1 preparation](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#preparing-essential-ga-objects-for-the-provider-v1) following our internal guidelines that contain those new rules regarding identifiers. +No matter if the resource has been refactored or not, the same recommendations mentioned above apply. + +## Next steps +While we have completed the identifiers rework for now, we plan to revisit these topics in the future to ensure continued improvements. +In the upcoming phases, we will focus on addressing the following key areas: +- Implementing better validations for identifiers. +- Providing support for new identifier formats in our resources (e.g. [instance roles](https://docs.snowflake.com/en/sql-reference/snowflake-db-classes#instance-roles)). + +## Conclusions +We have concluded the identifiers rework, implementing significant improvements to address common issues and inconsistencies in identifier handling. +Moving forward, we aim to continue enhancing our identifier functionalities to provide a smoother experience. +We value your feedback on the recent changes made to the identifiers. Please share your thoughts and suggestions to help us refine our identifier management further. diff --git a/pkg/datasources/security_integrations_acceptance_test.go b/pkg/datasources/security_integrations_acceptance_test.go index 8294157454..8bf82e5682 100644 --- a/pkg/datasources/security_integrations_acceptance_test.go +++ b/pkg/datasources/security_integrations_acceptance_test.go @@ -371,7 +371,7 @@ func TestAcc_SecurityIntegrations_OauthForCustomClients(t *testing.T) { // resource.TestCheckResourceAttr("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.0.blocked_roles_list.0.value"), resource.TestCheckResourceAttr("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.0.oauth_issue_refresh_tokens.0.value", "true"), resource.TestCheckResourceAttr("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.0.oauth_refresh_token_validity.0.value", "86400"), - resource.TestCheckResourceAttr("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.0.network_policy.0.value", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + resource.TestCheckResourceAttr("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.0.network_policy.0.value", networkPolicy.ID().Name()), resource.TestCheckResourceAttrSet("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.0.oauth_client_rsa_public_key_fp.0.value"), resource.TestCheckResourceAttrSet("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.0.oauth_client_rsa_public_key_2_fp.0.value"), resource.TestCheckResourceAttr("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.0.comment.0.value", comment), @@ -613,7 +613,7 @@ func TestAcc_SecurityIntegrations_Scim(t *testing.T) { resource.TestCheckResourceAttr("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.#", "1"), resource.TestCheckResourceAttr("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.0.enabled.0.value", "false"), - resource.TestCheckResourceAttr("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.0.network_policy.0.value", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + resource.TestCheckResourceAttr("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.0.network_policy.0.value", networkPolicy.ID().Name()), resource.TestCheckResourceAttr("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.0.run_as_role.0.value", "GENERIC_SCIM_PROVISIONER"), resource.TestCheckResourceAttr("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.0.sync_password.0.value", "true"), resource.TestCheckResourceAttr("data.snowflake_security_integrations.test", "security_integrations.0.describe_output.0.comment.0.value", comment), diff --git a/pkg/helpers/helpers.go b/pkg/helpers/helpers.go index a3e0f04737..5b9cfcd290 100644 --- a/pkg/helpers/helpers.go +++ b/pkg/helpers/helpers.go @@ -163,7 +163,7 @@ func ConcatSlices[T any](slices ...[]T) []T { return tmp } -// TODO(SNOW-999049): address during identifiers rework +// TODO(SNOW-1569530): address during identifiers rework func ParseRootLocation(location string) (sdk.SchemaObjectIdentifier, string, error) { location = strings.TrimPrefix(location, "@") parts, err := sdk.ParseIdentifierStringWithOpts(location, func(r *csv.Reader) { diff --git a/pkg/resources/account_role.go b/pkg/resources/account_role.go index 0626082501..ae076dec98 100644 --- a/pkg/resources/account_role.go +++ b/pkg/resources/account_role.go @@ -17,10 +17,9 @@ import ( var accountRoleSchema = map[string]*schema.Schema{ "name": { - Type: schema.TypeString, - Required: true, - // TODO(SNOW-999049): Uncomment once better identifier validation will be implemented - // ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), + Type: schema.TypeString, + Required: true, + ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), }, "comment": { Type: schema.TypeString, diff --git a/pkg/resources/common.go b/pkg/resources/common.go index d9335e2a46..0ac40fe575 100644 --- a/pkg/resources/common.go +++ b/pkg/resources/common.go @@ -28,7 +28,6 @@ func normalizeQuery(str string) string { return strings.TrimSpace(space.ReplaceAllString(str, " ")) } -// TODO [SNOW-999049]: address during identifiers rework func suppressIdentifierQuoting(_, oldValue, newValue string, _ *schema.ResourceData) bool { if oldValue == "" || newValue == "" { return false diff --git a/pkg/resources/deprecated_identifier_helpers.go b/pkg/resources/deprecated_identifier_helpers.go index c8b61236b1..5903225b3d 100644 --- a/pkg/resources/deprecated_identifier_helpers.go +++ b/pkg/resources/deprecated_identifier_helpers.go @@ -5,7 +5,7 @@ import ( "strings" ) -// TODO [SNOW-999049]: replace during identifiers rework +// TODO [SNOW-1634872]: replace during identifiers rework func FormatFullyQualifiedObjectID(dbName, schemaName, objectName string) string { var n strings.Builder @@ -40,7 +40,7 @@ func FormatFullyQualifiedObjectID(dbName, schemaName, objectName string) string return n.String() } -// TODO [SNOW-999049]: replace during identifiers rework +// TODO [SNOW-1634872]: replace during identifiers rework func ParseFullyQualifiedObjectID(s string) (dbName, schemaName, objectName string) { parsedString := strings.ReplaceAll(s, "\"", "") diff --git a/v1-preparations/CHANGES_BEFORE_V1.md b/v1-preparations/CHANGES_BEFORE_V1.md index 2f952f7344..a4ba18a9e2 100644 --- a/v1-preparations/CHANGES_BEFORE_V1.md +++ b/v1-preparations/CHANGES_BEFORE_V1.md @@ -65,3 +65,7 @@ Because of that, we would like to shelve the idea of introducing cloning to the object cloning is one of the topics we would like to take a closer look at. Right now, the cloning can be done manually and imported into normal resources, but in case there is any divergence between the normal and cloned object, the resources may act in an unexpected way. An alternative solution is to use plain SQL with [unsafe execute resources](https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs/resources/unsafe_execute) for now. + +## Identifier design decisions +The summary of design decisions taken during the [identifiers rework](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#identifiers-rework) +was put into a separate document ([here](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/identifiers_rework_design_decisions.md)). \ No newline at end of file From 136fdd336257136a4078e7fa1950a6da68d5479e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Cie=C5=9Blak?= Date: Thu, 22 Aug 2024 12:51:33 +0200 Subject: [PATCH 2/5] wip --- .../identifiers_rework_design_decisions.md | 21 ++++++++++---- ...tion_for_custom_clients_acceptance_test.go | 16 +++++------ .../scim_integration_acceptance_test.go | 10 +++---- pkg/resources/table_constraint.go | 2 +- pkg/sdk/failover_groups.go | 2 +- pkg/sdk/identifier_helpers.go | 28 ------------------- pkg/sdk/identifier_parsers.go | 1 - pkg/sdk/policy_references_test.go | 2 +- pkg/sdk/tag_association_validations.go | 2 +- ...cortex_search_services_integration_test.go | 4 +-- .../testint/streamlits_integration_test.go | 2 +- .../testint/streams_gen_integration_test.go | 8 +++--- pkg/sdk/testint/tables_integration_test.go | 2 +- pkg/sdk/testint/views_gen_integration_test.go | 2 +- 14 files changed, 42 insertions(+), 60 deletions(-) diff --git a/docs/technical-documentation/identifiers_rework_design_decisions.md b/docs/technical-documentation/identifiers_rework_design_decisions.md index af655f7fc7..37d52bc605 100644 --- a/docs/technical-documentation/identifiers_rework_design_decisions.md +++ b/docs/technical-documentation/identifiers_rework_design_decisions.md @@ -8,6 +8,7 @@ But before we dive into results and design decisions, here’s the list of reaso - Inconsistencies in quotes causing differences in Terraform plans. - The inconvenience of specifying fully qualified names in certain resource fields (e.g. object name in privilege-granting resources). - Mixed usage of account identifier formats across resources. + Now, knowing the issues we wanted to solve, we would like to present the changes and design decisions we made. ## Topics @@ -45,12 +46,22 @@ This will be a small shift in the identifier representation for resources. The g - If a resource can only be described with the Snowflake identifier, the fully qualified name will be put into the resource identifier. Previously, it was almost the same, except it was separated by pipes, and it was not a valid identifier. - If a resource cannot be described only by a single Snowflake identifier, then the resource identifier will be a pipe-separated text of all parts needed to identify a given resource ([example](https://registry.terraform.io/providers/Snowflake-Labs/snowflake/latest/docs/resources/grant_privileges_to_account_role#import)). Mind that this approach is not compliant with identifiers containing pipes, but this approach is a middle ground between an easy-to-specify separator and a character that shouldn’t be that common in the identifier (it was previously used for all identifiers). -Known limitations and identifier recommendations +### Known limitations and identifier recommendations The main limitations around identifiers are strictly connected to what characters are used. Here’s a list of recommendations on which characters should be generally avoided when specifying identifiers: -Avoid dots ‘.’ inside identifiers. It’s the main separator between identifier parts and although we are handling dots inside identifiers, there may be cases where it’s impossible to parse the identifier correctly. -Avoid pipes ‘|’ inside identifiers. It’s the separator for our more complex resource identifiers that could make our parser split the resource identifier into the wrong parts. -Avoid parentheses ‘(’ and ‘)’ when specifying identifiers for functions, procedures, external functions. Parentheses as part of their identifiers could potentially make our parser split the identifier into wrong parts causing issues. -As a general recommendation, please lean toward simple names without any special characters, and if word separation is needed, use underscores. This also applies to other “identifiers” like column names in tables or argument names in functions. If you are currently using complex identifiers, we recommend considering migration to simpler identifiers for a more straightforward and less error-prone experience. +- Avoid dots ‘.’ inside identifiers. It’s the main separator between identifier parts and although we are handling dots inside identifiers, there may be cases where it’s impossible to parse the identifier correctly. +- Avoid pipes ‘|’ inside identifiers. It’s the separator for our more complex resource identifiers that could make our parser split the resource identifier into the wrong parts. +- Avoid parentheses ‘(’ and ‘)’ when specifying identifiers for functions, procedures, external functions. Parentheses as part of their identifiers could potentially make our parser split the identifier into wrong parts causing issues. + +As a general recommendation, please lean toward simple names without any special characters, and if word separation is needed, use underscores. +This also applies to other “identifiers” like column names in tables or argument names in functions. +If you are currently using complex identifiers, we recommend considering migration to simpler identifiers for a more straightforward and less error-prone experience. +Also, we want to make it clear that every field specifying identifier (or its part, e.g. `name`, `database`, `schema`) are always case-sensitive. By specifying +identifiers with lowercase characters in Terraform, you also have to refer to them with lowercase names in quotes in Snowflake. +For example, by specifying an account role with `name = "test"` to check privileges granted to the role in Snowflake, you have to call: +```sql +show grants to role "test"; +show grants to role test; -- this won't work, because unquoted identifiers are converted to uppercase according to https://docs.snowflake.com/en/sql-reference/identifiers-syntax#label-identifier-casing +``` ### New identifier conventions Although, we are closing the identifiers rework, some resources won’t have the mentioned improvements. diff --git a/pkg/resources/oauth_integration_for_custom_clients_acceptance_test.go b/pkg/resources/oauth_integration_for_custom_clients_acceptance_test.go index 4eccc8625c..167370ef28 100644 --- a/pkg/resources/oauth_integration_for_custom_clients_acceptance_test.go +++ b/pkg/resources/oauth_integration_for_custom_clients_acceptance_test.go @@ -164,7 +164,7 @@ func TestAcc_OauthIntegrationForCustomClients_Basic(t *testing.T) { resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "blocked_roles_list.#", "3"), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "oauth_issue_refresh_tokens", "true"), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "oauth_refresh_token_validity", "86400"), - resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "network_policy", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "network_policy", networkPolicy.ID().Name()), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "oauth_client_rsa_public_key", key), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "oauth_client_rsa_public_key_2", key), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "comment", comment), @@ -189,7 +189,7 @@ func TestAcc_OauthIntegrationForCustomClients_Basic(t *testing.T) { // resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.blocked_roles_list.0.value"), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.oauth_issue_refresh_tokens.0.value", "true"), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.oauth_refresh_token_validity.0.value", "86400"), - resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.network_policy.0.value", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.network_policy.0.value", networkPolicy.ID().Name()), resource.TestCheckResourceAttrSet("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.oauth_client_rsa_public_key_fp.0.value"), resource.TestCheckResourceAttrSet("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.oauth_client_rsa_public_key_2_fp.0.value"), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.comment.0.value", comment), @@ -218,7 +218,7 @@ func TestAcc_OauthIntegrationForCustomClients_Basic(t *testing.T) { importchecks.TestCheckResourceAttrInstanceState(id.Name(), "blocked_roles_list.#", "3"), importchecks.TestCheckResourceAttrInstanceState(id.Name(), "oauth_issue_refresh_tokens", "true"), importchecks.TestCheckResourceAttrInstanceState(id.Name(), "oauth_refresh_token_validity", "86400"), - importchecks.TestCheckResourceAttrInstanceState(id.Name(), "network_policy", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + importchecks.TestCheckResourceAttrInstanceState(id.Name(), "network_policy", networkPolicy.ID().Name()), importchecks.TestCheckResourceAttrNotInInstanceState(id.Name(), "oauth_client_rsa_public_key"), importchecks.TestCheckResourceAttrNotInInstanceState(id.Name(), "oauth_client_rsa_public_key_2"), importchecks.TestCheckResourceAttrInstanceState(id.Name(), "comment", comment), @@ -268,7 +268,7 @@ func TestAcc_OauthIntegrationForCustomClients_Basic(t *testing.T) { resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "blocked_roles_list.#", "3"), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "oauth_issue_refresh_tokens", "true"), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "oauth_refresh_token_validity", "86400"), - resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "network_policy", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "network_policy", networkPolicy.ID().Name()), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "oauth_client_rsa_public_key", key), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "oauth_client_rsa_public_key_2", key), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "comment", comment), @@ -293,7 +293,7 @@ func TestAcc_OauthIntegrationForCustomClients_Basic(t *testing.T) { // resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.blocked_roles_list.0.value"), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.oauth_issue_refresh_tokens.0.value", "true"), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.oauth_refresh_token_validity.0.value", "86400"), - resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.network_policy.0.value", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.network_policy.0.value", networkPolicy.ID().Name()), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.oauth_client_rsa_public_key_fp.0.value", ""), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.oauth_client_rsa_public_key_2_fp.0.value", ""), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.comment.0.value", comment), @@ -425,7 +425,7 @@ func TestAcc_OauthIntegrationForCustomClients_Complete(t *testing.T) { resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "blocked_roles_list.#", "3"), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "oauth_issue_refresh_tokens", "true"), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "oauth_refresh_token_validity", "86400"), - resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "network_policy", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "network_policy", networkPolicy.ID().Name()), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "oauth_client_rsa_public_key", key), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "oauth_client_rsa_public_key_2", key), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "comment", comment), @@ -450,7 +450,7 @@ func TestAcc_OauthIntegrationForCustomClients_Complete(t *testing.T) { // resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.blocked_roles_list.0.value"), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.oauth_issue_refresh_tokens.0.value", "true"), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.oauth_refresh_token_validity.0.value", "86400"), - resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.network_policy.0.value", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.network_policy.0.value", networkPolicy.ID().Name()), resource.TestCheckResourceAttrSet("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.oauth_client_rsa_public_key_fp.0.value"), resource.TestCheckResourceAttrSet("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.oauth_client_rsa_public_key_2_fp.0.value"), resource.TestCheckResourceAttr("snowflake_oauth_integration_for_custom_clients.test", "describe_output.0.comment.0.value", comment), @@ -479,7 +479,7 @@ func TestAcc_OauthIntegrationForCustomClients_Complete(t *testing.T) { importchecks.TestCheckResourceAttrInstanceState(id.Name(), "blocked_roles_list.#", "3"), importchecks.TestCheckResourceAttrInstanceState(id.Name(), "oauth_issue_refresh_tokens", "true"), importchecks.TestCheckResourceAttrInstanceState(id.Name(), "oauth_refresh_token_validity", "86400"), - importchecks.TestCheckResourceAttrInstanceState(id.Name(), "network_policy", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + importchecks.TestCheckResourceAttrInstanceState(id.Name(), "network_policy", networkPolicy.ID().Name()), importchecks.TestCheckResourceAttrNotInInstanceState(id.Name(), "oauth_client_rsa_public_key"), importchecks.TestCheckResourceAttrNotInInstanceState(id.Name(), "oauth_client_rsa_public_key_2"), importchecks.TestCheckResourceAttrInstanceState(id.Name(), "comment", comment), diff --git a/pkg/resources/scim_integration_acceptance_test.go b/pkg/resources/scim_integration_acceptance_test.go index 1ae06e9413..4408602aa3 100644 --- a/pkg/resources/scim_integration_acceptance_test.go +++ b/pkg/resources/scim_integration_acceptance_test.go @@ -106,7 +106,7 @@ func TestAcc_ScimIntegration_basic(t *testing.T) { resource.TestCheckResourceAttr("snowflake_scim_integration.test", "enabled", "true"), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "scim_client", "OKTA"), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "run_as_role", role2.Name()), - resource.TestCheckResourceAttr("snowflake_scim_integration.test", "network_policy", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + resource.TestCheckResourceAttr("snowflake_scim_integration.test", "network_policy", networkPolicy.ID().Name()), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "sync_password", "false"), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "comment", "foo"), @@ -120,7 +120,7 @@ func TestAcc_ScimIntegration_basic(t *testing.T) { resource.TestCheckResourceAttr("snowflake_scim_integration.test", "describe_output.#", "1"), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "describe_output.0.enabled.0.value", "true"), - resource.TestCheckResourceAttr("snowflake_scim_integration.test", "describe_output.0.network_policy.0.value", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + resource.TestCheckResourceAttr("snowflake_scim_integration.test", "describe_output.0.network_policy.0.value", networkPolicy.ID().Name()), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "describe_output.0.run_as_role.0.value", role2.Name()), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "describe_output.0.sync_password.0.value", "false"), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "describe_output.0.comment.0.value", "foo"), @@ -138,7 +138,7 @@ func TestAcc_ScimIntegration_basic(t *testing.T) { importchecks.TestCheckResourceAttrInstanceState(id.Name(), "enabled", "true"), importchecks.TestCheckResourceAttrInstanceState(id.Name(), "scim_client", "OKTA"), importchecks.TestCheckResourceAttrInstanceState(id.Name(), "run_as_role", role2.Name()), - importchecks.TestCheckResourceAttrInstanceState(id.Name(), "network_policy", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + importchecks.TestCheckResourceAttrInstanceState(id.Name(), "network_policy", networkPolicy.ID().Name()), importchecks.TestCheckResourceAttrInstanceState(id.Name(), "sync_password", "false"), importchecks.TestCheckResourceAttrInstanceState(id.Name(), "comment", "foo"), ), @@ -194,7 +194,7 @@ func TestAcc_ScimIntegration_complete(t *testing.T) { resource.TestCheckResourceAttr("snowflake_scim_integration.test", "enabled", "false"), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "scim_client", "GENERIC"), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "run_as_role", role.Name()), - resource.TestCheckResourceAttr("snowflake_scim_integration.test", "network_policy", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + resource.TestCheckResourceAttr("snowflake_scim_integration.test", "network_policy", networkPolicy.ID().Name()), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "sync_password", "false"), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "comment", "foo"), ), @@ -241,7 +241,7 @@ func TestAcc_ScimIntegration_completeAzure(t *testing.T) { resource.TestCheckResourceAttr("snowflake_scim_integration.test", "enabled", "false"), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "scim_client", string(sdk.ScimSecurityIntegrationScimClientAzure)), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "run_as_role", role.Name()), - resource.TestCheckResourceAttr("snowflake_scim_integration.test", "network_policy", sdk.NewAccountObjectIdentifier(networkPolicy.Name).Name()), // TODO(SNOW-999049): Fix during identifiers rework + resource.TestCheckResourceAttr("snowflake_scim_integration.test", "network_policy", networkPolicy.ID().Name()), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "sync_password", r.BooleanDefault), resource.TestCheckResourceAttr("snowflake_scim_integration.test", "comment", "foo"), ), diff --git a/pkg/resources/table_constraint.go b/pkg/resources/table_constraint.go index f929dabcf6..415322c9fd 100644 --- a/pkg/resources/table_constraint.go +++ b/pkg/resources/table_constraint.go @@ -211,7 +211,7 @@ func (v *tableConstraintID) parse(s string) { func getTableIdentifier(s string) (*sdk.SchemaObjectIdentifier, error) { var objectIdentifier sdk.ObjectIdentifier var err error - // TODO [SNOW-999049]: Fallback for old implementations using table.id instead of table.fully_qualified_name - probably will be removed later. + // TODO [SNOW-1348114]: Address during table rework; Fallback for old implementations using table.id instead of table.fully_qualified_name - probably will be removed later. if strings.Contains(s, "|") { objectIdentifier = helpers.DecodeSnowflakeID(s) } else { diff --git a/pkg/sdk/failover_groups.go b/pkg/sdk/failover_groups.go index 9ff342b2c8..bf88d84c54 100644 --- a/pkg/sdk/failover_groups.go +++ b/pkg/sdk/failover_groups.go @@ -602,7 +602,7 @@ func (v *failoverGroups) ShowShares(ctx context.Context, id AccountObjectIdentif } resultList := make([]AccountObjectIdentifier, len(dest)) for i, r := range dest { - // TODO [SNOW-999049]: this was not working correctly with identifiers containing `.` character + // TODO [SNOW-1348343]: change during failover groups rework; this was not working correctly with identifiers containing `.` character resultList[i] = NewExternalObjectIdentifier(NewAccountIdentifierFromFullyQualifiedName(r.OwnerAccount), NewAccountObjectIdentifier(r.Name)).objectIdentifier.(AccountObjectIdentifier) } return resultList, nil diff --git a/pkg/sdk/identifier_helpers.go b/pkg/sdk/identifier_helpers.go index 58665d26d0..95ea8e894f 100644 --- a/pkg/sdk/identifier_helpers.go +++ b/pkg/sdk/identifier_helpers.go @@ -1,7 +1,6 @@ package sdk import ( - "encoding/csv" "fmt" "log" "strings" @@ -16,33 +15,6 @@ type ObjectIdentifier interface { FullyQualifiedName() string } -// TODO(SNOW-999049): This function will be tested/improved/used more wiedely during the identifiers rework. -// Right now, the implementation is just a copy of DecodeSnowflakeParameterID used in resources. -func ParseObjectIdentifier(identifier string) (ObjectIdentifier, error) { - reader := csv.NewReader(strings.NewReader(identifier)) - reader.Comma = '.' - lines, err := reader.ReadAll() - if err != nil { - return nil, fmt.Errorf("unable to read identifier: %s, err = %w", identifier, err) - } - if len(lines) != 1 { - return nil, fmt.Errorf("incompatible identifier: %s", identifier) - } - parts := lines[0] - switch len(parts) { - case 1: - return NewAccountObjectIdentifier(parts[0]), nil - case 2: - return NewDatabaseObjectIdentifier(parts[0], parts[1]), nil - case 3: - return NewSchemaObjectIdentifier(parts[0], parts[1], parts[2]), nil - case 4: - return NewTableColumnIdentifier(parts[0], parts[1], parts[2], parts[3]), nil - default: - return nil, fmt.Errorf("unable to classify identifier: %s", identifier) - } -} - func NewObjectIdentifierFromFullyQualifiedName(fullyQualifiedName string) ObjectIdentifier { parts := strings.Split(fullyQualifiedName, ".") switch len(parts) { diff --git a/pkg/sdk/identifier_parsers.go b/pkg/sdk/identifier_parsers.go index dc47009c6a..a1b7bc13fb 100644 --- a/pkg/sdk/identifier_parsers.go +++ b/pkg/sdk/identifier_parsers.go @@ -69,7 +69,6 @@ func ParseAccountObjectIdentifier(identifier string) (AccountObjectIdentifier, e ) } -// TODO(SNOW-1495053): Replace ParseObjectIdentifier // ParseObjectIdentifierString tries to guess the identifier by the number of parts it contains. // Because of the overlapping, in some cases, the output ObjectIdentifier can be one of the following implementations: // - AccountObjectIdentifier for one part diff --git a/pkg/sdk/policy_references_test.go b/pkg/sdk/policy_references_test.go index d67aac1eea..5e00285e97 100644 --- a/pkg/sdk/policy_references_test.go +++ b/pkg/sdk/policy_references_test.go @@ -116,7 +116,7 @@ func TestPolicyReferencesGetForEntity(t *testing.T) { }) } -// TODO [SNOW-999049]: check during the identifiers rework +// TODO [SNOW-1569516]: make nicer during the identifiers rework follow up func temporaryReplace(id SchemaObjectIdentifier) string { return strings.ReplaceAll(id.FullyQualifiedName(), `"`, `\"`) } diff --git a/pkg/sdk/tag_association_validations.go b/pkg/sdk/tag_association_validations.go index 19d9bca9ff..f4a2d1e33f 100644 --- a/pkg/sdk/tag_association_validations.go +++ b/pkg/sdk/tag_association_validations.go @@ -35,7 +35,7 @@ var ( ObjectTypeColumn, ObjectTypeEventTable, } - // TODO(SNOW-999049): Object types should be able tell their id structure and tagAssociationAllowedObjectTypes should be used to filter correct object types. + // TODO(SNOW-1229218): Object types should be able tell their id structure and tagAssociationAllowedObjectTypes should be used to filter correct object types. TagAssociationTagObjectTypeIsSchemaObjectType = []ObjectType{ ObjectTypeAlert, ObjectTypeExternalFunction, diff --git a/pkg/sdk/testint/cortex_search_services_integration_test.go b/pkg/sdk/testint/cortex_search_services_integration_test.go index 6397a86865..52910aaf08 100644 --- a/pkg/sdk/testint/cortex_search_services_integration_test.go +++ b/pkg/sdk/testint/cortex_search_services_integration_test.go @@ -79,8 +79,8 @@ func TestInt_CortexSearchServices(t *testing.T) { assert.NotEmpty(t, cortexSearchServiceDetails.CreatedOn) assert.Equal(t, cortexSearchService.Name, cortexSearchServiceDetails.Name) // Yes, the names are exchanged on purpose, because now it works like this - assert.Equal(t, cortexSearchService.DatabaseName, cortexSearchServiceDetails.SchemaName) - assert.Equal(t, cortexSearchService.SchemaName, cortexSearchServiceDetails.DatabaseName) + assert.Equal(t, cortexSearchService.DatabaseName, cortexSearchServiceDetails.DatabaseName) + assert.Equal(t, cortexSearchService.SchemaName, cortexSearchServiceDetails.SchemaName) assert.Equal(t, targetLag, cortexSearchServiceDetails.TargetLag) assert.NotEmpty(t, cortexSearchServiceDetails.Warehouse) assert.Equal(t, strings.ToUpper(on), *cortexSearchServiceDetails.SearchColumn) diff --git a/pkg/sdk/testint/streamlits_integration_test.go b/pkg/sdk/testint/streamlits_integration_test.go index 5f9e9435ac..ac4a4ac70d 100644 --- a/pkg/sdk/testint/streamlits_integration_test.go +++ b/pkg/sdk/testint/streamlits_integration_test.go @@ -329,7 +329,7 @@ func TestInt_Streamlits(t *testing.T) { require.Equal(t, e.Name, detail.Name) require.Equal(t, e.UrlId, detail.UrlId) require.Equal(t, mainFile, detail.MainFile) - // TODO [SNOW-999049]: make nicer during the identifiers rework + // TODO [SNOW-1569516]: make nicer during the identifiers rework follow up require.Equal(t, stage.ID().FullyQualifiedName(), sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(detail.RootLocation[1:]).FullyQualifiedName()) require.Empty(t, detail.Title) require.Empty(t, detail.QueryWarehouse) diff --git a/pkg/sdk/testint/streams_gen_integration_test.go b/pkg/sdk/testint/streams_gen_integration_test.go index 6b5548c0ba..618f0c5a62 100644 --- a/pkg/sdk/testint/streams_gen_integration_test.go +++ b/pkg/sdk/testint/streams_gen_integration_test.go @@ -46,7 +46,7 @@ func TestInt_Streams(t *testing.T) { s, err := client.Streams.ShowByID(ctx, id) require.NoError(t, err) - // TODO [SNOW-999049]: make nicer during the identifiers rework + // TODO [SNOW-1348112]: make nicer during the stream rework assert.Equal(t, table.ID().FullyQualifiedName(), sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(*s.TableName).FullyQualifiedName()) assertStream(t, s, id, "Table", "DEFAULT") }) @@ -77,7 +77,7 @@ func TestInt_Streams(t *testing.T) { s, err := client.Streams.ShowByID(ctx, id) require.NoError(t, err) - // TODO [SNOW-999049]: make nicer during the identifiers rework + // TODO [SNOW-1348112]: make nicer during the stream rework assert.Equal(t, externalTableId.FullyQualifiedName(), sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(*s.TableName).FullyQualifiedName()) assertStream(t, s, id, "External Table", "INSERT_ONLY") }) @@ -148,7 +148,7 @@ func TestInt_Streams(t *testing.T) { require.NoError(t, err) assertStream(t, s, cloneId, "Table", "DEFAULT") - // TODO [SNOW-999049]: make nicer during the identifiers rework + // TODO [SNOW-1348112]: make nicer during the stream rework assert.Equal(t, table.ID().FullyQualifiedName(), sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(*s.TableName).FullyQualifiedName()) }) @@ -414,7 +414,7 @@ func TestInt_Streams(t *testing.T) { assert.Equal(t, schema.Name, s.SchemaName) assert.Nil(t, s.TableOn) assert.Equal(t, "some comment", *s.Comment) - // TODO [SNOW-999049]: make nicer during the identifiers rework + // TODO [SNOW-1348112]: make nicer during the stream rework assert.Equal(t, table.ID().FullyQualifiedName(), sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(*s.TableName).FullyQualifiedName()) assert.Equal(t, "Table", *s.SourceType) assert.Equal(t, "DEFAULT", *s.Mode) diff --git a/pkg/sdk/testint/tables_integration_test.go b/pkg/sdk/testint/tables_integration_test.go index f6a6f08ce9..4a3dcf8ee4 100644 --- a/pkg/sdk/testint/tables_integration_test.go +++ b/pkg/sdk/testint/tables_integration_test.go @@ -525,7 +525,7 @@ func TestInt_Table(t *testing.T) { require.NoError(t, err) require.Equal(t, 2, len(tableDetails)) - // TODO [SNOW-999049]: make nicer during the identifiers rework + // TODO [SNOW-1348114]: make nicer during the table rework assert.Equal(t, maskingPolicy.ID().FullyQualifiedName(), sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(*tableDetails[0].PolicyName).FullyQualifiedName()) alterRequest := sdk.NewAlterTableRequest(id). diff --git a/pkg/sdk/testint/views_gen_integration_test.go b/pkg/sdk/testint/views_gen_integration_test.go index 8d4510cdae..25e2e796ab 100644 --- a/pkg/sdk/testint/views_gen_integration_test.go +++ b/pkg/sdk/testint/views_gen_integration_test.go @@ -409,7 +409,7 @@ func TestInt_Views(t *testing.T) { require.NoError(t, err) assert.Equal(t, 1, len(alteredViewDetails)) - // TODO [SNOW-999049]: make nicer during the identifiers rework + // TODO [SNOW-1348118]: make nicer during the view rework assert.Equal(t, maskingPolicy.ID().FullyQualifiedName(), sdk.NewSchemaObjectIdentifierFromFullyQualifiedName(*alteredViewDetails[0].PolicyName).FullyQualifiedName()) alterRequest = sdk.NewAlterViewRequest(id).WithUnsetMaskingPolicyOnColumn( From c3524770bd0ef46dcc3b9a8ad5a88479e2a901e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Cie=C5=9Blak?= Date: Thu, 22 Aug 2024 16:45:34 +0200 Subject: [PATCH 3/5] Changes after review --- pkg/resources/grant_application_role.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/resources/grant_application_role.go b/pkg/resources/grant_application_role.go index 98f0668de1..2a1432e1e8 100644 --- a/pkg/resources/grant_application_role.go +++ b/pkg/resources/grant_application_role.go @@ -175,6 +175,7 @@ func ReadContextGrantApplicationRole(ctx context.Context, d *schema.ResourceData break } } else { + // TODO(SNOW-1569535): fix when we'll have data types associated with the correct identifier parser /* note that grantee_name is not saved as a valid identifier in the SHOW GRANTS OF APPLICATION ROLE command From a214ec2b680b91b90ae265d7270b3b74bded03f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Cie=C5=9Blak?= Date: Fri, 23 Aug 2024 08:39:38 +0200 Subject: [PATCH 4/5] Changes after review --- CREATING_ISSUES.md | 4 +-- README.md | 25 ++++++++++--------- .../identifiers_rework_design_decisions.md | 17 +++++++++++++ pkg/helpers/helpers.go | 2 +- pkg/resources/account_role.go | 2 +- .../deprecated_identifier_helpers.go | 4 +-- 6 files changed, 36 insertions(+), 18 deletions(-) diff --git a/CREATING_ISSUES.md b/CREATING_ISSUES.md index a9af1ef6eb..8d9802db01 100644 --- a/CREATING_ISSUES.md +++ b/CREATING_ISSUES.md @@ -100,8 +100,8 @@ which is highly recommended for large-scale migrations. ### What identifiers are valid inside the provider and how to reference one resource inside the other one? Please refer to [this document](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/identifiers_rework_design_decisions.md) -- For the recommended identifier format, take a look at the "Known limitations and identifier recommendations" section. -- For a new way of referencing object identifiers in resources, take a look at the "New computed fully qualified name field in resources" section. +- For the recommended identifier format, take a look at the ["Known limitations and identifier recommendations"](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/identifiers_rework_design_decisions.md#known-limitations-and-identifier-recommendations) section. +- For a new way of referencing object identifiers in resources, take a look at the ["New computed fully qualified name field in resources" ](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/docs/technical-documentation/identifiers_rework_design_decisions.md#new-computed-fully-qualified-name-field-in-resources) section. ## Commonly known issues ### Old Terraform CLI version diff --git a/README.md b/README.md index c88a8a9624..391118f254 100644 --- a/README.md +++ b/README.md @@ -15,18 +15,19 @@ This is a terraform provider for managing [Snowflake](https://www.snowflake.com/) resources. ## Table of contents -- [Snowflake Terraform Provider](#snowflake-terraform-provider) - - [Table of contents](#table-of-contents) - - [Getting started](#getting-started) - - [Migration guide](#migration-guide) - - [Roadmap](#roadmap) - - [Getting Help](#getting-help) - - [Would you like to create an issue?](#would-you-like-to-create-an-issue) - - [Additional debug logs for `snowflake_grant_privileges_to_role` resource](#additional-debug-logs-for-snowflake_grant_privileges_to_role-resource) - - [Additional SQL Client configuration](#additional-sql-client-configuration) - - [Contributing](#contributing) - - [Releases](#releases) - + +* [Snowflake Terraform Provider](#snowflake-terraform-provider) + * [Table of contents](#table-of-contents) + * [Getting started](#getting-started) + * [Migration guide](#migration-guide) + * [Roadmap](#roadmap) + * [Getting Help](#getting-help) + * [Would you like to create an issue?](#would-you-like-to-create-an-issue) + * [Additional debug logs for `snowflake_grant_privileges_to_role` resource](#additional-debug-logs-for-snowflake_grant_privileges_to_role-resource) + * [Additional SQL Client configuration](#additional-sql-client-configuration) + * [Contributing](#contributing) + * [Releases](#releases) + ## Getting started diff --git a/docs/technical-documentation/identifiers_rework_design_decisions.md b/docs/technical-documentation/identifiers_rework_design_decisions.md index 37d52bc605..65ccdb467a 100644 --- a/docs/technical-documentation/identifiers_rework_design_decisions.md +++ b/docs/technical-documentation/identifiers_rework_design_decisions.md @@ -1,6 +1,23 @@ # Identifiers rework +## Table of contents + +* [Identifiers rework](#identifiers-rework) + * [Table of contents](#table-of-contents) + * [Topics](#topics) + * [New identifier parser](#new-identifier-parser) + * [Using the recommended format for account identifiers](#using-the-recommended-format-for-account-identifiers) + * [Better handling for identifiers with arguments](#better-handling-for-identifiers-with-arguments) + * [Quoting differences](#quoting-differences) + * [New computed fully qualified name field in resources](#new-computed-fully-qualified-name-field-in-resources) + * [New resource identifier format](#new-resource-identifier-format) + * [Known limitations and identifier recommendations](#known-limitations-and-identifier-recommendations) + * [New identifier conventions](#new-identifier-conventions) + * [Next steps](#next-steps) + * [Conclusions](#conclusions) + + This document summarises work done in the [identifiers rework](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#identifiers-rework) and future plans for further identifier improvements. But before we dive into results and design decisions, here’s the list of reasons why we decided to rework the identifiers in the first place: - Common issues with identifiers with arguments (identifiers for functions, procedures, and external functions). diff --git a/pkg/helpers/helpers.go b/pkg/helpers/helpers.go index 5b9cfcd290..0c94f05585 100644 --- a/pkg/helpers/helpers.go +++ b/pkg/helpers/helpers.go @@ -163,7 +163,7 @@ func ConcatSlices[T any](slices ...[]T) []T { return tmp } -// TODO(SNOW-1569530): address during identifiers rework +// TODO(SNOW-1569530): address during identifiers rework follow-up func ParseRootLocation(location string) (sdk.SchemaObjectIdentifier, string, error) { location = strings.TrimPrefix(location, "@") parts, err := sdk.ParseIdentifierStringWithOpts(location, func(r *csv.Reader) { diff --git a/pkg/resources/account_role.go b/pkg/resources/account_role.go index a656c294f1..0a9095ca5c 100644 --- a/pkg/resources/account_role.go +++ b/pkg/resources/account_role.go @@ -20,7 +20,7 @@ var accountRoleSchema = map[string]*schema.Schema{ Type: schema.TypeString, Required: true, DiffSuppressFunc: suppressIdentifierQuoting, - // TODO(SNOW-999049): Uncomment once better identifier validation will be implemented + // TODO(SNOW-1495079): Uncomment once better identifier validation will be implemented // ValidateDiagFunc: IsValidIdentifier[sdk.AccountObjectIdentifier](), }, "comment": { diff --git a/pkg/resources/deprecated_identifier_helpers.go b/pkg/resources/deprecated_identifier_helpers.go index 5903225b3d..9e5555f0fb 100644 --- a/pkg/resources/deprecated_identifier_helpers.go +++ b/pkg/resources/deprecated_identifier_helpers.go @@ -5,7 +5,7 @@ import ( "strings" ) -// TODO [SNOW-1634872]: replace during identifiers rework +// TODO [SNOW-1634872]: replace during identifiers rework follow-up func FormatFullyQualifiedObjectID(dbName, schemaName, objectName string) string { var n strings.Builder @@ -40,7 +40,7 @@ func FormatFullyQualifiedObjectID(dbName, schemaName, objectName string) string return n.String() } -// TODO [SNOW-1634872]: replace during identifiers rework +// TODO [SNOW-1634872]: replace during identifiers rework follow-up func ParseFullyQualifiedObjectID(s string) (dbName, schemaName, objectName string) { parsedString := strings.ReplaceAll(s, "\"", "") From 04f8679c549947db0155b6dfe4de523337ed7c9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Cie=C5=9Blak?= Date: Fri, 23 Aug 2024 10:02:33 +0200 Subject: [PATCH 5/5] =?UTF-8?q?Add=20forbidden=20character=20=F0=9F=92=80?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../identifiers_rework_design_decisions.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/technical-documentation/identifiers_rework_design_decisions.md b/docs/technical-documentation/identifiers_rework_design_decisions.md index 65ccdb467a..b60aad2183 100644 --- a/docs/technical-documentation/identifiers_rework_design_decisions.md +++ b/docs/technical-documentation/identifiers_rework_design_decisions.md @@ -68,12 +68,13 @@ The main limitations around identifiers are strictly connected to what character - Avoid dots ‘.’ inside identifiers. It’s the main separator between identifier parts and although we are handling dots inside identifiers, there may be cases where it’s impossible to parse the identifier correctly. - Avoid pipes ‘|’ inside identifiers. It’s the separator for our more complex resource identifiers that could make our parser split the resource identifier into the wrong parts. - Avoid parentheses ‘(’ and ‘)’ when specifying identifiers for functions, procedures, external functions. Parentheses as part of their identifiers could potentially make our parser split the identifier into wrong parts causing issues. +- Do not use double quotes as part of identifiers (in Snowflake you can have double quotes inside identifiers by escaping them with the second double quote, e.g. `create database “test””identifier”` will create a database with name `test"identifier`). As a general recommendation, please lean toward simple names without any special characters, and if word separation is needed, use underscores. This also applies to other “identifiers” like column names in tables or argument names in functions. If you are currently using complex identifiers, we recommend considering migration to simpler identifiers for a more straightforward and less error-prone experience. -Also, we want to make it clear that every field specifying identifier (or its part, e.g. `name`, `database`, `schema`) are always case-sensitive. By specifying -identifiers with lowercase characters in Terraform, you also have to refer to them with lowercase names in quotes in Snowflake. +Also, we want to make it clear that every field specifying an identifier (or its part, e.g. `name`, `database`, `schema`) is always case-sensitive. By specifying +an identifier with lowercase characters in Terraform, you also have to refer to them with lowercase names in quotes in Snowflake. For example, by specifying an account role with `name = "test"` to check privileges granted to the role in Snowflake, you have to call: ```sql show grants to role "test";