Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Tag association v1 readiness #3210

Merged
merged 12 commits into from
Dec 5, 2024
Merged

Conversation

sfc-gh-jmichalak
Copy link
Collaborator

@sfc-gh-jmichalak sfc-gh-jmichalak commented Nov 20, 2024

  • 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?

Copy link

Integration tests failure for 8aa6ea667b3781751030a662948cb08d39c1c647

Copy link

Integration tests failure for f97b0e8b9fdee53a6442fa8b6e76eaf351a5ba06

Copy link

Integration tests success for 818d00b33e87a83140565a1320a695221e9e66c1

MIGRATION_GUIDE.md Outdated Show resolved Hide resolved
pkg/acceptance/helpers/context_client.go Outdated Show resolved Hide resolved
pkg/resources/tag_association.go Show resolved Hide resolved
ConfigVariables: m(),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "object_type", "DATABASE"),
ConfigVariables: m(tagId, tagValue),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used the generated builders for the new tests.

pkg/sdk/system_functions.go Show resolved Hide resolved
MIGRATION_GUIDE.md Outdated Show resolved Hide resolved
MIGRATION_GUIDE.md Show resolved Hide resolved
docs/resources/authentication_policy.md Outdated Show resolved Hide resolved
@@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will keep it open then.

pkg/acceptance/check_destroy.go Outdated Show resolved Hide resolved
pkg/resources/tag_association_state_upgraders.go Outdated Show resolved Hide resolved
pkg/resources/tag_association.go Outdated Show resolved Hide resolved
pkg/resources/tag_association.go Outdated Show resolved Hide resolved
pkg/resources/tag_association_acceptance_test.go Outdated Show resolved Hide resolved
sfc-gh-jmichalak added a commit that referenced this pull request Nov 25, 2024
#3215)

<!-- Feel free to delete comments as you fill this in -->
- 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
<!-- summary of changes -->

## Test Plan
<!-- detail ways in which this PR has been tested or needs to be tested
-->
* [ ] acceptance tests
<!-- add more below if you think they are relevant -->
* [ ] …

## TODO
- remove objects related with tags, procedures, functions,
unsafe_execute
sfc-gh-jmichalak added a commit that referenced this pull request Nov 26, 2024
<!-- Feel free to delete comments as you fill this in -->
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

<!-- summary of changes -->

## Test Plan
<!-- detail ways in which this PR has been tested or needs to be tested
-->
* [ ] acceptance tests
<!-- add more below if you think they are relevant -->
* [ ] …

## References
<!-- issues documentation links, etc  -->
#3210
Copy link

Integration tests failure for b53d7c5c60883896a65720d0a81b8fb529229552

Copy link

Integration tests failure for c856aa87d6273a8cebd47d5826f6432795eb93df

@@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This format is valid in GH, but not in TF registry. Reverted in #3230.

sfc-gh-jcieslak
sfc-gh-jcieslak previously approved these changes Dec 2, 2024
@sfc-gh-asawicki sfc-gh-asawicki self-requested a review December 3, 2024 14:20
sfc-gh-asawicki
sfc-gh-asawicki previously approved these changes Dec 3, 2024
Copy link

github-actions bot commented Dec 3, 2024

Integration tests cancelled for c856aa87d6273a8cebd47d5826f6432795eb93df

Copy link

github-actions bot commented Dec 3, 2024

Integration tests failure for 71cb0e526e00872b470a2589701fe9af2165f9f3

Copy link

github-actions bot commented Dec 4, 2024

Integration tests failure for 4130e00b0a6b0d49fb7f0a22856dcce7358c2b19

@sfc-gh-asawicki sfc-gh-asawicki self-requested a review December 5, 2024 09:38
@sfc-gh-jmichalak sfc-gh-jmichalak merged commit 04f6d54 into main Dec 5, 2024
8 of 9 checks passed
@sfc-gh-jmichalak sfc-gh-jmichalak deleted the tag-association-v1 branch December 5, 2024 09:46
sfc-gh-jcieslak pushed a commit that referenced this pull request Dec 12, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.100.0](v0.99.0...v0.100.0)
(2024-12-12)


### 🎉 **What's new:**

* Account v1 readiness
([#3236](#3236))
([5df33a8](5df33a8))
* Account v1 readiness generated files
([#3242](#3242))
([3df59dd](3df59dd))
* Account v1 readiness resource
([#3252](#3252))
([8f5698d](8f5698d))
* Add a new account roles data source
([#3257](#3257))
([b3d6b9e](b3d6b9e))
* Add account data source
([#3261](#3261))
([6087fc9](6087fc9))
* Add all other functions and procedures implementations
([#3275](#3275))
([7a6f68d](7a6f68d))
* Basic functions implementation
([#3269](#3269))
([6d4a103](6d4a103))
* Basic procedures implementation
([#3271](#3271))
([933335f](933335f))
* Docs, test, and missing parameter
([#3280](#3280))
([10517f3](10517f3))
* Functions and procedures schemas and generated sources
([#3262](#3262))
([9b70f87](9b70f87))
* Functions sdk update
([#3254](#3254))
([fc1eace](fc1eace))
* Handle missing fields in function and procedure
([#3273](#3273))
([53e7a0a](53e7a0a))
* Procedures schemas and generated sources
([#3263](#3263))
([211ad46](211ad46))
* Procedures sdk update
([#3255](#3255))
([682606a](682606a))
* Rework account parameter resource
([#3264](#3264))
([15aa9c2](15aa9c2))
* Rework data types
([#3244](#3244))
([05ada91](05ada91))
* support table data type
([#3274](#3274))
([13401d5](13401d5))
* Tag association v1 readiness
([#3210](#3210))
([04f6d54](04f6d54))
* Test imports and small fixes
([#3276](#3276))
([a712195](a712195))
* Unsafe execute v1 readiness
([#3266](#3266))
([c4f1e8f](c4f1e8f))
* Use new data types in sql builder for functions and procedures
([#3247](#3247))
([69f677a](69f677a))


### 🔧 **Misc**

* Add ShowByID filtering generation
([#3227](#3227))
([548ec42](548ec42))
* Adress small task-related todos
([#3243](#3243))
([40de9ae](40de9ae))
* Apply masking
([#3234](#3234))
([c209a8a](c209a8a))
* fix missing references in toOpts and changes with newlines
([#3240](#3240))
([246547f](246547f))
* function tests
([#3279](#3279))
([5af6efb](5af6efb))
* Improve config builders
([#3207](#3207))
([425787c](425787c))
* Revert to proper env
([#3238](#3238))
([5d4ed3b](5d4ed3b))
* Use service user for ci
([#3228](#3228))
([2fb50d7](2fb50d7))


### 🐛 **Bug fixes:**

* Make blocked_roles_field optional in OAuth security integrations
([#3267](#3267))
([7197b57](7197b57))
* Minor fixes
([#3231](#3231))
([1863bf6](1863bf6))
* Minor fixes 2
([#3230](#3230))
([73b7e74](73b7e74))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants