feat: Tag association v1 readiness #3210

sfc-gh-jmichalak · 2024-11-20T14:39:21Z

rework tag_association resource
return nil from GetTag instead of failing
add more tests regarding tag/masking policy: assert that ALTER MASKING POLICY SET TAG differs from ALTER TAG SET MASKING POLICY
support tagging account for identifiers with org name
support IF EXISTS for unsetting tags
add notes about manually unassigning policies from objects, add a todo with an issue number
fix a wrong issue number in essential objects list

Test Plan

acceptance tests

…

References

https://docs.snowflake.com/en/user-guide/object-tagging
https://docs.snowflake.com/en/sql-reference/functions/system_get_tag
#3145
#1910
#2943
#3235

TODO

use generated config and asserts, remove old test tf files

Ideas

extract a separate resource for tagging accounts?

github-actions · 2024-11-20T17:02:34Z

Integration tests failure for 8aa6ea667b3781751030a662948cb08d39c1c647

github-actions · 2024-11-20T18:06:45Z

Integration tests failure for f97b0e8b9fdee53a6442fa8b6e76eaf351a5ba06

github-actions · 2024-11-21T13:07:38Z

Integration tests success for 818d00b33e87a83140565a1320a695221e9e66c1

sfc-gh-jcieslak · 2024-11-21T16:00:50Z

MIGRATION_GUIDE.md

+}
+```
+
+The state is migrated automatically. There is no need to adjust configuration files, unless you use resource id like `snowflake_tag_association.example.id`.


I don't understand this part: There is no need to adjust configuration files, unless you use resource id like snowflake_tag_association.example.id., could you elaborate?

pkg/acceptance/helpers/context_client.go

pkg/resources/tag_association.go

sfc-gh-jcieslak · 2024-11-22T09:28:24Z

pkg/resources/tag_association_acceptance_test.go

-				ConfigVariables: m(),
-				Check: resource.ComposeTestCheckFunc(
-					resource.TestCheckResourceAttr(resourceName, "object_type", "DATABASE"),
+				ConfigVariables: m(tagId, tagValue),


Why didn't we use model + custom asserts approach here?

As stated in the TODO section of the PR description, I'll do this in the next PR.

Used the generated builders for the new tests.

pkg/sdk/system_functions.go

sfc-gh-asawicki · 2024-11-22T08:58:46Z

MIGRATION_GUIDE.md

+}
+```
+
+The state is migrated automatically. There is no need to adjust configuration files, unless you use resource id like `snowflake_tag_association.example.id`.


nit: unless you use ... as reference in other resources

sfc-gh-asawicki · 2024-11-22T09:01:53Z

MIGRATION_GUIDE.md

@@ -9,6 +9,61 @@ across different versions.

 ## v0.98.0 ➞ v0.99.0

+### 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 in one resource.


so it means that if I want to add the same tag, same value to the same object type, then I have to put all of them in one resource, right? (here it would mean that changing silver to gold in the second resource is not permitted because both these resources would have the same id)

If I understand this correctly, then I think it would be good to add this info, directly to the resource docs, and here too.

Yes, added more explanation.

sfc-gh-asawicki · 2024-11-22T09:05:21Z

docs/resources/authentication_policy.md

@@ -5,6 +5,8 @@ description: |-
  Resource used to manage authentication policy objects. For more information, check authentication policy documentation https://docs.snowflake.com/en/sql-reference/sql/create-authentication-policy.
 ---

+-> **Note** According to Snowflake [docs](https://docs.snowflake.com/en/sql-reference/sql/drop-authentication-policy#usage-notes), an authentication policy cannot be dropped successfully if it is currently assigned to another object. Currently, the provider does not unassign such objects automatically. Before dropping the resource, list the assigned objects with `SELECT * from table(information_schema.policy_references(policy_name=>'<string>'));` and unassign them manually with `ALTER ...` or with updated Terraform configuration, if possible.


nit: maybe even warning for this

It was done in #3226.

sfc-gh-asawicki · 2024-11-22T09:08:16Z

docs/resources/authentication_policy.md

@@ -5,6 +5,8 @@ description: |-
  Resource used to manage authentication policy objects. For more information, check authentication policy documentation https://docs.snowflake.com/en/sql-reference/sql/create-authentication-policy.
 ---

+-> **Note** According to Snowflake [docs](https://docs.snowflake.com/en/sql-reference/sql/drop-authentication-policy#usage-notes), an authentication policy cannot be dropped successfully if it is currently assigned to another object. Currently, the provider does not unassign such objects automatically. Before dropping the resource, list the assigned objects with `SELECT * from table(information_schema.policy_references(policy_name=>'<string>'));` and unassign them manually with `ALTER ...` or with updated Terraform configuration, if possible.


Let's suggest going through terraform config changes in two steps and manual SQL as an alternative. Also, let's add example to guides/ and reference it here.

In guide let's show:

the example config creating policy assignment

error that will be a result of just a removal

step 1 - unnasigning policy (by changing config and running tf plan_apply)

step 2 - successful removal

This change was merged in #3226. I'll add such a guide in a next PR.

sfc-gh-asawicki · 2024-11-22T09:17:04Z

pkg/acceptance/check_destroy.go

+				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


nit: t.Logf at least in such a case

sfc-gh-asawicki · 2024-11-22T09:50:37Z

pkg/resources/tag_association_state_upgraders.go

+		if objectType == string(sdk.ObjectTypeAccount) {
+			id = sdk.NewAccountIdentifierFromFullyQualifiedName(obj["name"].(string))
+		} else {
+			database := obj["database"].(string)


nit: there is getTagObjectIdentifier function already; maybe we could extend and use it (or at least use it for the else part)

sfc-gh-asawicki · 2024-11-22T09:56:14Z

pkg/resources/tag_association.go

+			return diag.FromErr(err)
+		}
+
+		addedids, removedids := ListDiff(oldIds, newIds)


nit: addedIds and removedIds

sfc-gh-asawicki · 2024-11-22T10:00:57Z

pkg/resources/tag_association.go

+	if err != nil {
+		return diag.FromErr(err)
+	}
+	if d.HasChange("tag_value") {


in this two subsequent ifs we do exactly the same "setup" logic:

we get the tag value

we get the lists of ids (old, new, common)

Let's merge this logic:

If object is removed, unset the tag, no matter the value

If the object is added, set the tag, no matter if the value changed (we just use the new one in all cases)

If object is common, set only if the value changed

It covers all the cases (I guess) but is more straightforward

sfc-gh-asawicki · 2024-11-22T10:03:47Z

pkg/resources/tag_association_acceptance_test.go

+					resource.TestCheckResourceAttr(resourceName, "tag_value", tagValue),
+				),
+			},
+			// external change


external change can be either unset or setting to different value which are a bit different, please add also this one.

sfc-gh-asawicki · 2024-11-22T10:07:40Z

pkg/resources/tag_association_acceptance_test.go

 	"github.com/hashicorp/terraform-plugin-testing/terraform"
 	"github.com/hashicorp/terraform-plugin-testing/tfversion"
 )

 func TestAcc_TagAssociation(t *testing.T) {
+	_ = testenvs.GetOrSkipTest(t, testenvs.EnableAcceptance)


Can we have two more tests, please:

Test with more objects, so that we verify the logic behind added, removed, common (i.e. we have 3 objects, we remove 1 and add 1 and verify all)

Test for object type change (maybe I missed this one) - let's verify forceNew and proper removal of all previous yags

#3215)  - remove deprecated data sources and resources - some of the tag related fields are not removed - will be done after merging #3210 - some resources (procedures, functions, unsafe_execute) are not removed - can be done after reworking - remove deprecated fields in the provider config - deprecated fields in the resources/data sources that we haven't reworked are still there - remove some unused code from `snowflake` package - remove deprecated `JWT` value from `authenticator` - improve doc generator, so that the headers are added only in case of non-empty lists - I have left some comments in the templates because the generated files must not be blank  ## Test Plan  * [ ] acceptance tests  * [ ] … ## TODO - remove objects related with tags, procedures, functions, unsafe_execute

Changes included (they are really small or fix some wrong changes done during tag rework that were not released yet): - smaller fixes extracted from https://github.com/Snowflake-Labs/terraform-provider-snowflake/pull/3210/files - support tagging account for identifiers with org name - add notes about manually unassigning policies from objects, add a todo with an issue number - fix a wrong issue number in essential objects list Changes NOT included - rework tag_association resource - return nil from GetTag instead of failing - add more tests regarding tag/masking policy: assert that ALTER MASKING POLICY SET TAG differs from ALTER TAG SET MASKING POLICY - support IF EXISTS for unsetting tags  ## Test Plan  * [ ] acceptance tests  * [ ] … ## References  #3210

github-actions · 2024-11-29T13:57:39Z

Integration tests failure for b53d7c5c60883896a65720d0a81b8fb529229552

github-actions · 2024-11-29T18:03:04Z

Integration tests failure for c856aa87d6273a8cebd47d5826f6432795eb93df

sfc-gh-jcieslak · 2024-12-02T11:55:15Z

templates/resources/row_access_policy.md.tmpl

@@ -11,7 +11,8 @@ description: |-

 !> **V1 release candidate** This resource was reworked and is a release candidate for the V1. We do not expect significant changes in it before the V1. We will welcome any feedback and adjust the resource if needed. Any errors reported will be resolved with a higher priority. We encourage checking this resource out before the V1 release. Please follow the [migration guide](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#v0950--v0960) to use it.

-> **Note** According to Snowflake [docs](https://docs.snowflake.com/en/sql-reference/sql/drop-row-access-policy#usage-notes), a row access policy cannot be dropped successfully if it is currently assigned to another object. Currently, the provider does not unassign such objects automatically. Before dropping the resource, list the assigned objects with `SELECT * from table(information_schema.policy_references(policy_name=>'<string>'));` and unassign them manually with `ALTER ...` or with updated Terraform configuration, if possible.
+> [!WARNING]
+> According to Snowflake [docs](https://docs.snowflake.com/en/sql-reference/sql/drop-row-access-policy#usage-notes), a row access policy cannot be dropped successfully if it is currently assigned to another object. Currently, the provider does not unassign such objects automatically. Before dropping the resource, list the assigned objects with `SELECT * from table(information_schema.policy_references(policy_name=>'<string>'));` and unassign them manually with `ALTER ...` or with updated Terraform configuration, if possible.


why this format? Warning has different one, it won't show properly in the docs as warning section.

sfc-gh-jmichalak added 5 commits November 18, 2024 17:49

wip

6f06e25

rework tag association

624a07d

Merge remote-tracking branch 'origin/main' into tag-association-v1

a547fac

Cleanup

8aa6ea6

Cleanup

f97b0e8

Cleanup and fix

818d00b

sfc-gh-jmichalak requested review from sfc-gh-asawicki and sfc-gh-jcieslak November 21, 2024 13:50

sfc-gh-jmichalak marked this pull request as ready for review November 21, 2024 13:50

sfc-gh-jmichalak mentioned this pull request Nov 21, 2024

chore: Remove deprecated resources, data sources, and fields in config #3215

Merged

2 tasks

sfc-gh-jcieslak reviewed Nov 22, 2024

View reviewed changes

sfc-gh-asawicki requested changes Nov 22, 2024

View reviewed changes

sfc-gh-jmichalak mentioned this pull request Nov 26, 2024

fix: Small fixes and adjustments #3226

Merged

2 tasks

sfc-gh-jmichalak added 3 commits November 28, 2024 15:07

Merge remote-tracking branch 'origin/main' into tag-association-v1

7751436

Review comments

4e5d39a

Merge remote-tracking branch 'origin/main' into tag-association-v1

b53d7c5

Fix tests

c856aa8

sfc-gh-jcieslak reviewed Dec 2, 2024

View reviewed changes

sfc-gh-jcieslak approved these changes Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Tag association v1 readiness #3210

feat: Tag association v1 readiness #3210

sfc-gh-jmichalak commented Nov 20, 2024 •

edited

Loading

github-actions bot commented Nov 20, 2024

github-actions bot commented Nov 20, 2024

github-actions bot commented Nov 21, 2024

sfc-gh-jcieslak Nov 21, 2024

sfc-gh-jmichalak Dec 2, 2024

sfc-gh-jcieslak Nov 22, 2024

sfc-gh-jmichalak Nov 28, 2024

sfc-gh-jmichalak Dec 2, 2024

sfc-gh-asawicki Nov 22, 2024

sfc-gh-jmichalak Dec 2, 2024

sfc-gh-asawicki Nov 22, 2024

sfc-gh-jmichalak Dec 2, 2024

sfc-gh-asawicki Nov 22, 2024

sfc-gh-jmichalak Nov 28, 2024

sfc-gh-asawicki Nov 22, 2024

sfc-gh-jmichalak Dec 2, 2024

sfc-gh-asawicki Nov 22, 2024

sfc-gh-jmichalak Dec 2, 2024

sfc-gh-asawicki Nov 22, 2024

sfc-gh-jmichalak Dec 2, 2024

sfc-gh-asawicki Nov 22, 2024

sfc-gh-jmichalak Dec 2, 2024

sfc-gh-asawicki Nov 22, 2024

sfc-gh-jmichalak Dec 2, 2024

sfc-gh-asawicki Nov 22, 2024

sfc-gh-jmichalak Dec 2, 2024

sfc-gh-asawicki Nov 22, 2024

sfc-gh-jmichalak Dec 2, 2024

github-actions bot commented Nov 29, 2024

github-actions bot commented Nov 29, 2024

sfc-gh-jcieslak Dec 2, 2024

feat: Tag association v1 readiness #3210

Are you sure you want to change the base?

feat: Tag association v1 readiness #3210

Conversation

sfc-gh-jmichalak commented Nov 20, 2024 • edited Loading

Test Plan

References

TODO

Ideas

github-actions bot commented Nov 20, 2024

github-actions bot commented Nov 20, 2024

github-actions bot commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 29, 2024

github-actions bot commented Nov 29, 2024

Choose a reason for hiding this comment

sfc-gh-jmichalak commented Nov 20, 2024 •

edited

Loading