From f97b0e8b9fdee53a6442fa8b6e76eaf351a5ba06 Mon Sep 17 00:00:00 2001 From: Jakub Michalak Date: Wed, 20 Nov 2024 16:34:12 +0100 Subject: [PATCH] Cleanup --- MIGRATION_GUIDE.md | 21 +++++++++- pkg/acceptance/check_destroy.go | 2 +- pkg/acceptance/helpers/context_client.go | 9 +++++ .../helpers/system_functions_client.go | 30 ++++++++++++++ pkg/acceptance/helpers/tag_client.go | 8 ---- pkg/acceptance/helpers/test_client.go | 2 + pkg/helpers/helpers.go | 33 --------------- pkg/resources/helper_expansion.go | 31 ++++++++++++-- .../tag_association_acceptance_test.go | 4 +- .../tag_association_state_upgraders.go | 25 +++++++----- pkg/resources/tag_association_test.go | 16 ++++++++ .../TestAcc_TagAssociation/issue1202/main.tf | 5 --- .../TestAcc_TagAssociation/issue1909/test.tf | 18 ++------- .../TestAcc_TagAssociation/issue1926/test.tf | 2 + pkg/sdk/tags_impl.go | 2 + pkg/sdk/tags_test.go | 40 +++++++++++++++---- pkg/sdk/testint/tags_integration_test.go | 5 +-- 17 files changed, 161 insertions(+), 92 deletions(-) create mode 100644 pkg/acceptance/helpers/system_functions_client.go diff --git a/MIGRATION_GUIDE.md b/MIGRATION_GUIDE.md index 6b697b91d5..3a58240b90 100644 --- a/MIGRATION_GUIDE.md +++ b/MIGRATION_GUIDE.md @@ -11,13 +11,30 @@ across different versions. ### snowflake_tag_association resource changes #### *(behavior change)* new id format -In order to provide more functionality for tagging objects, we have changed the resource id from `"TAG_DATABASE"."TAG_SCHEMA"."TAG_NAME"` to `"TAG_DATABASE"."TAG_SCHEMA"."TAG_NAME"|TAG_VALUE|OBJECT_TYPE`. This allows to group tags associations per tag ID, tag value and object type like the following: +In order to provide more functionality for tagging objects, we have changed the resource id from `"TAG_DATABASE"."TAG_SCHEMA"."TAG_NAME"` to `"TAG_DATABASE"."TAG_SCHEMA"."TAG_NAME"|TAG_VALUE|OBJECT_TYPE`. This allows to group tags associations per tag ID, tag value and object type in one resource. ``` +resource "snowflake_tag_association" "gold_warehouses" { + object_identifiers = [snowflake_warehouse.w1.fully_qualified_name, snowflake_warehouse.w2.fully_qualified_name] + object_type = "WAREHOUSE" + tag_id = snowflake_tag.tier.fully_qualified_name + tag_value = "gold" +} +resource "snowflake_tag_association" "silver_warehouses" { + object_identifiers = [snowflake_warehouse.w3.fully_qualified_name] + object_type = "WAREHOUSE" + tag_id = snowflake_tag.tier.fully_qualified_name + tag_value = "silver" +} +resource "snowflake_tag_association" "silver_databases" { + object_identifiers = [snowflake_database.d1.fully_qualified_name] + object_type = "DATABASE" + tag_id = snowflake_tag.tier.fully_qualified_name + tag_value = "silver" +} ``` The state is migrated automatically. There is no need to adjust configuration files, unless you use resource id like `snowflake_tag_association.example.id`. - #### *(behavior change)* changed fields Behavior of some fields was changed: - `object_identifier` was renamed to `object_identifiers` and it is now a set of fully qualified names. Change your configurations from diff --git a/pkg/acceptance/check_destroy.go b/pkg/acceptance/check_destroy.go index 306a77d66e..3b13ae4f3c 100644 --- a/pkg/acceptance/check_destroy.go +++ b/pkg/acceptance/check_destroy.go @@ -523,7 +523,7 @@ func CheckTagValueEmpty(t *testing.T) func(*terraform.State) error { return fmt.Errorf("invalid object id: %w", err) } } - tag, err := TestClient().Tag.GetTag(t, tagId, id, objectType) + tag, err := TestClient().SystemFunctions.GetTag(t, tagId, id, objectType) if err != nil { if strings.Contains(err.Error(), "does not exist or not authorized") { // Note: this can happen if the object has been deleted as well; in this case, ignore the error diff --git a/pkg/acceptance/helpers/context_client.go b/pkg/acceptance/helpers/context_client.go index a4dc768089..f6078cd86e 100644 --- a/pkg/acceptance/helpers/context_client.go +++ b/pkg/acceptance/helpers/context_client.go @@ -53,6 +53,15 @@ func (c *ContextClient) CurrentOrganizationName(t *testing.T) string { return orgName } +func (c *ContextClient) CurrentAccountIdentifier(t *testing.T) sdk.AccountIdentifier { + t.Helper() + + orgName := c.CurrentOrganizationName(t) + accountName := c.CurrentAccountName(t) + + return sdk.NewAccountIdentifier(orgName, accountName) +} + func (c *ContextClient) CurrentRole(t *testing.T) sdk.AccountObjectIdentifier { t.Helper() ctx := context.Background() diff --git a/pkg/acceptance/helpers/system_functions_client.go b/pkg/acceptance/helpers/system_functions_client.go new file mode 100644 index 0000000000..50bcc6f275 --- /dev/null +++ b/pkg/acceptance/helpers/system_functions_client.go @@ -0,0 +1,30 @@ +package helpers + +import ( + "context" + "testing" + + "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" +) + +type SystemFunctionsClient struct { + context *TestClientContext +} + +func NewSystemFunctionsClient(context *TestClientContext) *SystemFunctionsClient { + return &SystemFunctionsClient{ + context: context, + } +} + +func (c *SystemFunctionsClient) client() sdk.SystemFunctions { + return c.context.client.SystemFunctions +} + +func (c *SystemFunctionsClient) GetTag(t *testing.T, tagId sdk.SchemaObjectIdentifier, objectId sdk.ObjectIdentifier, objectType sdk.ObjectType) (*string, error) { + t.Helper() + ctx := context.Background() + client := c.context.client.SystemFunctions + + return client.GetTag(ctx, tagId, objectId, objectType) +} diff --git a/pkg/acceptance/helpers/tag_client.go b/pkg/acceptance/helpers/tag_client.go index b0d3c4327b..1a4641efa1 100644 --- a/pkg/acceptance/helpers/tag_client.go +++ b/pkg/acceptance/helpers/tag_client.go @@ -60,14 +60,6 @@ func (c *TagClient) Unset(t *testing.T, objectType sdk.ObjectType, id sdk.Object require.NoError(t, err) } -func (c *TagClient) GetTag(t *testing.T, tagId sdk.SchemaObjectIdentifier, objectId sdk.ObjectIdentifier, objectType sdk.ObjectType) (*string, error) { - t.Helper() - ctx := context.Background() - client := c.context.client.SystemFunctions - - return client.GetTag(ctx, tagId, objectId, objectType) -} - func (c *TagClient) Alter(t *testing.T, req *sdk.AlterTagRequest) { t.Helper() ctx := context.Background() diff --git a/pkg/acceptance/helpers/test_client.go b/pkg/acceptance/helpers/test_client.go index 53a9b6cb2d..8aad7aa58d 100644 --- a/pkg/acceptance/helpers/test_client.go +++ b/pkg/acceptance/helpers/test_client.go @@ -59,6 +59,7 @@ type TestClient struct { StorageIntegration *StorageIntegrationClient Stream *StreamClient Streamlit *StreamlitClient + SystemFunctions *SystemFunctionsClient Table *TableClient Tag *TagClient Task *TaskClient @@ -131,6 +132,7 @@ func NewTestClient(c *sdk.Client, database string, schema string, warehouse stri StorageIntegration: NewStorageIntegrationClient(context, idsGenerator), Stream: NewStreamClient(context, idsGenerator), Streamlit: NewStreamlitClient(context, idsGenerator), + SystemFunctions: NewSystemFunctionsClient(context), Table: NewTableClient(context, idsGenerator), Tag: NewTagClient(context, idsGenerator), Task: NewTaskClient(context, idsGenerator), diff --git a/pkg/helpers/helpers.go b/pkg/helpers/helpers.go index 24c1ae8eba..6b3e39d4cf 100644 --- a/pkg/helpers/helpers.go +++ b/pkg/helpers/helpers.go @@ -290,36 +290,3 @@ func ContainsIdentifierIgnoringQuotes(ids []string, id string) bool { return false } - -func ExpandStringList(configured []interface{}) []string { - vs := make([]string, 0, len(configured)) - for _, v := range configured { - val, ok := v.(string) - if ok && val != "" { - vs = append(vs, val) - } - } - return vs -} - -func ExpandObjectIdentifierSet(configured []any, objectType sdk.ObjectType) ([]sdk.ObjectIdentifier, error) { - vs := ExpandStringList(configured) - ids := make([]sdk.ObjectIdentifier, len(vs)) - for i, idRaw := range vs { - var id sdk.ObjectIdentifier - var err error - if objectType == sdk.ObjectTypeAccount { - id, err = sdk.ParseAccountIdentifier(idRaw) - if err != nil { - return nil, fmt.Errorf("invalid account id: %w", err) - } - } else { - id, err = sdk.ParseObjectIdentifierString(idRaw) - if err != nil { - return nil, fmt.Errorf("invalid object id: %w", err) - } - } - ids[i] = id - } - return ids, nil -} diff --git a/pkg/resources/helper_expansion.go b/pkg/resources/helper_expansion.go index 73eeb5eb80..399c9eb813 100644 --- a/pkg/resources/helper_expansion.go +++ b/pkg/resources/helper_expansion.go @@ -1,9 +1,9 @@ package resources import ( + "fmt" "slices" - "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/helpers" "github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk" ) @@ -20,11 +20,36 @@ func expandIntList(configured []interface{}) []int { } func expandStringList(configured []interface{}) []string { - return helpers.ExpandStringList(configured) + vs := make([]string, 0, len(configured)) + for _, v := range configured { + val, ok := v.(string) + if ok && val != "" { + vs = append(vs, val) + } + } + return vs } func ExpandObjectIdentifierSet(configured []any, objectType sdk.ObjectType) ([]sdk.ObjectIdentifier, error) { - return helpers.ExpandObjectIdentifierSet(configured, objectType) + vs := expandStringList(configured) + ids := make([]sdk.ObjectIdentifier, len(vs)) + for i, idRaw := range vs { + var id sdk.ObjectIdentifier + var err error + if objectType == sdk.ObjectTypeAccount { + id, err = sdk.ParseAccountIdentifier(idRaw) + if err != nil { + return nil, fmt.Errorf("invalid account id: %w", err) + } + } else { + id, err = sdk.ParseObjectIdentifierString(idRaw) + if err != nil { + return nil, fmt.Errorf("invalid object id: %w", err) + } + } + ids[i] = id + } + return ids, nil } func expandStringListAllowEmpty(configured []interface{}) []string { diff --git a/pkg/resources/tag_association_acceptance_test.go b/pkg/resources/tag_association_acceptance_test.go index 2cacd97a63..5a04256a91 100644 --- a/pkg/resources/tag_association_acceptance_test.go +++ b/pkg/resources/tag_association_acceptance_test.go @@ -313,9 +313,7 @@ func TestAcc_TagAssociationAccountIssues1910(t *testing.T) { _ = testenvs.GetOrSkipTest(t, testenvs.EnableAcceptance) tagId := acc.TestClient().Ids.RandomSchemaObjectIdentifier() - accountName := acc.TestClient().Context.CurrentAccountName(t) - orgName := acc.TestClient().Context.CurrentOrganizationName(t) - accountId := sdk.NewAccountIdentifier(orgName, accountName) + accountId := acc.TestClient().Context.CurrentAccountIdentifier(t) resourceName := "snowflake_tag_association.test" m := func() map[string]config.Variable { return map[string]config.Variable{ diff --git a/pkg/resources/tag_association_state_upgraders.go b/pkg/resources/tag_association_state_upgraders.go index c3a6e574ca..60e37f751f 100644 --- a/pkg/resources/tag_association_state_upgraders.go +++ b/pkg/resources/tag_association_state_upgraders.go @@ -25,18 +25,21 @@ func v0_98_0_TagAssociationStateUpgrader(ctx context.Context, rawState map[strin for _, objectIdentifierOld := range objectIdentifiersOld { obj := objectIdentifierOld.(map[string]any) var id sdk.ObjectIdentifier - database := obj["database"].(string) - schema := obj["schema"].(string) - name := obj["name"].(string) - switch { - case schema != "": - id = sdk.NewSchemaObjectIdentifier(database, schema, name) - case database != "": - id = sdk.NewDatabaseObjectIdentifier(database, name) - default: - id = sdk.NewAccountObjectIdentifier(name) + if objectType == string(sdk.ObjectTypeAccount) { + id = sdk.NewAccountIdentifierFromFullyQualifiedName(obj["name"].(string)) + } else { + database := obj["database"].(string) + schema := obj["schema"].(string) + name := obj["name"].(string) + switch { + case schema != "": + id = sdk.NewSchemaObjectIdentifier(database, schema, name) + case database != "": + id = sdk.NewDatabaseObjectIdentifier(database, name) + default: + id = sdk.NewAccountObjectIdentifier(name) + } } - objectIdentifiers = append(objectIdentifiers, id.FullyQualifiedName()) } rawState["object_identifiers"] = objectIdentifiers diff --git a/pkg/resources/tag_association_test.go b/pkg/resources/tag_association_test.go index c0f6f2b530..62015b1c7e 100644 --- a/pkg/resources/tag_association_test.go +++ b/pkg/resources/tag_association_test.go @@ -11,6 +11,22 @@ import ( ) func TestTagIdentifierAndObjectIdentifier(t *testing.T) { + t.Run("account identifier", func(t *testing.T) { + in := map[string]any{ + "tag_id": "\"test_db\".\"test_schema\".\"test_tag\"", + "object_type": "ACCOUNT", + "object_identifier": []any{ + "orgname.accountname", + }, + } + d := schema.TestResourceDataRaw(t, resources.TagAssociation().Schema, in) + tid, identifiers, objectType, err := resources.TagIdentifierAndObjectIdentifier(d) + require.NoError(t, err) + assert.Equal(t, sdk.NewSchemaObjectIdentifier("test_db", "test_schema", "test_tag"), tid) + assert.Len(t, identifiers, 1) + assert.Equal(t, `"orgname"."accountname"`, identifiers[0].FullyQualifiedName()) + assert.Equal(t, sdk.ObjectTypeAccount, objectType) + }) t.Run("account object identifier", func(t *testing.T) { in := map[string]any{ "tag_id": "\"test_db\".\"test_schema\".\"test_tag\"", diff --git a/pkg/resources/testdata/TestAcc_TagAssociation/issue1202/main.tf b/pkg/resources/testdata/TestAcc_TagAssociation/issue1202/main.tf index 52a093e268..dd81b42a82 100644 --- a/pkg/resources/testdata/TestAcc_TagAssociation/issue1202/main.tf +++ b/pkg/resources/testdata/TestAcc_TagAssociation/issue1202/main.tf @@ -15,11 +15,6 @@ resource "snowflake_table" "test" { } resource "snowflake_tag_association" "test" { - // we need to set the object_identifier to avoid the following error: - // provider_test.go:17: err: resource snowflake_tag_association: object_identifier: Optional or Required must be set, not both - // we should consider deprecating object_identifier in favor of object_name - // https://github.com/Snowflake-Labs/terraform-provider-snowflake/pull/2534#discussion_r1507570740 - // object_name = "\"${var.database}\".\"${var.schema}\".\"${var.table_name}\"" object_identifiers = [snowflake_table.test.fully_qualified_name] object_type = "TABLE" tag_id = snowflake_tag.test.fully_qualified_name diff --git a/pkg/resources/testdata/TestAcc_TagAssociation/issue1909/test.tf b/pkg/resources/testdata/TestAcc_TagAssociation/issue1909/test.tf index 361ee85266..519df8f05e 100644 --- a/pkg/resources/testdata/TestAcc_TagAssociation/issue1909/test.tf +++ b/pkg/resources/testdata/TestAcc_TagAssociation/issue1909/test.tf @@ -25,18 +25,8 @@ resource "snowflake_table" "test2" { resource "snowflake_tag_association" "test" { object_identifiers = [var.column_fully_qualified_name, var.column2_fully_qualified_name] - # object_identifier { - # database = var.database - # schema = var.schema - # name = "${snowflake_table.test.name}.${snowflake_table.test.column[0].name}" - # } - # object_identifier { - # database = var.database - # schema = var.schema - # name = "${snowflake_table.test2.name}.${snowflake_table.test2.column[0].name}" - # } - object_type = "COLUMN" - tag_id = snowflake_tag.test.fully_qualified_name - tag_value = "v1" - depends_on = [snowflake_table.test, snowflake_table.test2] + object_type = "COLUMN" + tag_id = snowflake_tag.test.fully_qualified_name + tag_value = "v1" + depends_on = [snowflake_table.test, snowflake_table.test2] } diff --git a/pkg/resources/testdata/TestAcc_TagAssociation/issue1926/test.tf b/pkg/resources/testdata/TestAcc_TagAssociation/issue1926/test.tf index 2f2d42947c..8401d09080 100644 --- a/pkg/resources/testdata/TestAcc_TagAssociation/issue1926/test.tf +++ b/pkg/resources/testdata/TestAcc_TagAssociation/issue1926/test.tf @@ -7,6 +7,8 @@ resource "snowflake_table" "test" { name = var.table_name database = var.database schema = var.schema + // TODO(SNOW-1348114): use only one column, if possible. + // We need a dummy column here because a table must have at least one column, and when we rename the second one in the config, it gets dropped for a moment. column { name = "DUMMY" type = "VARIANT" diff --git a/pkg/sdk/tags_impl.go b/pkg/sdk/tags_impl.go index 9d1907923f..aa194a9147 100644 --- a/pkg/sdk/tags_impl.go +++ b/pkg/sdk/tags_impl.go @@ -149,6 +149,7 @@ func (s *SetTagRequest) toOpts() *setTagOptions { o.column = String(id.Name()) } } + // TODO(SNOW-1818976): remove this hack if o.objectType == ObjectTypeAccount { id, ok := o.objectName.(AccountIdentifier) if ok { @@ -174,6 +175,7 @@ func (s *UnsetTagRequest) toOpts() *unsetTagOptions { o.column = String(id.Name()) } } + // TODO(SNOW-1818976): remove this hack if o.objectType == ObjectTypeAccount { id, ok := o.objectName.(AccountIdentifier) if ok { diff --git a/pkg/sdk/tags_test.go b/pkg/sdk/tags_test.go index 0979537b27..38eb3edcb7 100644 --- a/pkg/sdk/tags_test.go +++ b/pkg/sdk/tags_test.go @@ -394,6 +394,21 @@ func TestTagSet(t *testing.T) { assertOptsValidAndSQLEquals(t, opts, `ALTER %s %s SET TAG "tag1" = 'value1'`, opts.objectType, id.FullyQualifiedName()) }) + t.Run("set on account", func(t *testing.T) { + accountId := randomAccountIdentifier() + opts := &setTagOptions{ + objectType: ObjectTypeStage, + objectName: accountId, + SetTags: []TagAssociation{ + { + Name: NewAccountObjectIdentifier("tag1"), + Value: "value1", + }, + }, + } + assertOptsValidAndSQLEquals(t, opts, `ALTER %s %s SET TAG "tag1", "tag2"`, opts.objectType, accountId.FullyQualifiedName()) + }) + t.Run("set with column", func(t *testing.T) { objectId := randomTableColumnIdentifierInSchemaObject(id) tagId := randomSchemaObjectIdentifier() @@ -434,19 +449,27 @@ func TestTagUnset(t *testing.T) { assertOptsInvalidJoinedErrors(t, opts, errors.New("tagging for object type SEQUENCE is not supported")) }) - t.Run("validation: unsupported account", func(t *testing.T) { - opts := defaultOpts() - opts.objectType = ObjectTypeAccount - assertOptsInvalidJoinedErrors(t, opts, errors.New("tagging for object type ACCOUNT is not supported - use Tags.UnsetOnCurrentAccount instead")) - }) - t.Run("unset with all optional", func(t *testing.T) { opts := defaultOpts() opts.UnsetTags = []ObjectIdentifier{ NewAccountObjectIdentifier("tag1"), NewAccountObjectIdentifier("tag2"), } - assertOptsValidAndSQLEquals(t, opts, `ALTER %s %s UNSET TAG "tag1", "tag2"`, opts.objectType, id.FullyQualifiedName()) + opts.IfExists = Pointer(true) + assertOptsValidAndSQLEquals(t, opts, `ALTER %s IF EXISTS %s UNSET TAG "tag1", "tag2"`, opts.objectType, id.FullyQualifiedName()) + }) + + t.Run("unset on account", func(t *testing.T) { + accountId := randomAccountIdentifier() + opts := &unsetTagOptions{ + objectType: ObjectTypeStage, + objectName: accountId, + UnsetTags: []ObjectIdentifier{ + NewAccountObjectIdentifier("tag1"), + NewAccountObjectIdentifier("tag2"), + }, + } + assertOptsValidAndSQLEquals(t, opts, `ALTER %s %s UNSET TAG "tag1", "tag2"`, opts.objectType, accountId.FullyQualifiedName()) }) t.Run("unset with column", func(t *testing.T) { @@ -460,8 +483,9 @@ func TestTagUnset(t *testing.T) { tagId1, tagId2, }, + IfExists: Pointer(true), } opts := request.toOpts() - assertOptsValidAndSQLEquals(t, opts, `ALTER %s %s MODIFY COLUMN "%s" UNSET TAG %s, %s`, opts.objectType, id.FullyQualifiedName(), objectId.Name(), tagId1.FullyQualifiedName(), tagId2.FullyQualifiedName()) + assertOptsValidAndSQLEquals(t, opts, `ALTER %s IF EXISTS %s MODIFY COLUMN "%s" UNSET TAG %s, %s`, opts.objectType, id.FullyQualifiedName(), objectId.Name(), tagId1.FullyQualifiedName(), tagId2.FullyQualifiedName()) }) } diff --git a/pkg/sdk/testint/tags_integration_test.go b/pkg/sdk/testint/tags_integration_test.go index e87b79ccb1..84d5d817b2 100644 --- a/pkg/sdk/testint/tags_integration_test.go +++ b/pkg/sdk/testint/tags_integration_test.go @@ -373,10 +373,7 @@ func TestInt_TagsAssociations(t *testing.T) { }) t.Run("TestInt_TagAssociationForAccount", func(t *testing.T) { - accountName := testClientHelper().Context.CurrentAccountName(t) - organizationName := testClientHelper().Context.CurrentOrganizationName(t) - id := sdk.NewAccountIdentifier(organizationName, accountName) - + id := testClientHelper().Context.CurrentAccountIdentifier(t) // test tag sdk method err := client.Tags.Set(ctx, sdk.NewSetTagRequest(sdk.ObjectTypeAccount, id).WithSetTags(tags)) require.NoError(t, err)