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: Upgrade tag SDK #3126

Merged
merged 10 commits into from
Oct 21, 2024
Merged

feat: Upgrade tag SDK #3126

merged 10 commits into from
Oct 21, 2024

Conversation

sfc-gh-jmichalak
Copy link
Collaborator

@sfc-gh-jmichalak sfc-gh-jmichalak commented Oct 10, 2024

Test Plan

  • integration tests
  • unit tests

References

https://docs.snowflake.com/en/sql-reference/sql/create-tag
https://docs.snowflake.com/en/user-guide/object-tagging
https://docs.snowflake.com/en/sql-reference/functions/system_get_tag

TODO

  • change GetTag to not fail on null values
  • move tag assignments integration tests to a common place
  • restrict the list of object types we want to support, instead of all of them
  • rework tag, tag_association resource
  • remove tag_masking_policy_association_resource (tag_association will be used instead)

Copy link

Integration tests failure for 424b47af067b90540fd2c61a1d0ae3c058ca4fea

Copy link

Integration tests cancelled for 651c4e36cd88317fcbd19030f713ef50522c4ad0

Copy link

Integration tests cancelled for acb0981ae15561392c73f553800c16f1a293794b

@sfc-gh-jmichalak sfc-gh-jmichalak marked this pull request as ready for review October 11, 2024 14:11
Copy link

Integration tests failure for 47b9ac5dec645cea701af0885eb5b790ea5a0117

pkg/resources/tag.go Outdated Show resolved Hide resolved
ObjectTypeAlert,
ObjectTypeBudget,
ObjectTypeClassification,
ObjectTypeExternalFunction,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could create a test that would go through all the object types to see which ones are valid ones, I just checked that, e.g. dynamic tables can also be used (with table data type ofc).

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for such a test

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's related to #3126 (comment) - I can do this in the next PR.

pkg/sdk/object_types.go Show resolved Hide resolved
// SQL compilation error: Invalid value VIEW for argument OBJECT_TYPE. Please use object type TABLE for all kinds of table-like objects.
// TODO [SNOW-1022645]: discuss how we handle situation like this in the SDK
func normalizeGetTagObjectType(objectType ObjectType) ObjectType {
if slices.Contains([]ObjectType{ObjectTypeView, ObjectTypeMaterializedView, ObjectTypeExternalTable}, objectType) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about dynamic table?

Copy link
Collaborator Author

@sfc-gh-jmichalak sfc-gh-jmichalak Oct 15, 2024

Choose a reason for hiding this comment

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

Initially, I missed dynamic tables because they are missing from https://docs.snowflake.com/en/user-guide/object-tagging. Managing dynamic tables is not implemented in the SDK. I just checked it manually, and it turns out you can use GetTag with dynamic table as an argument.

Thinking in reverse: here we can narrow the list of accepted object types, like we do in the grants, wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea, next PR/follow-up issue?

pkg/sdk/testint/applications_integration_test.go Outdated Show resolved Hide resolved
pkg/sdk/testint/databases_integration_test.go Show resolved Hide resolved
@@ -194,6 +195,33 @@ func TestInt_ExternalFunctions(t *testing.T) {
assertExternalFunction(t, externalFunction.ID(), true)
})

t.Run("alter external function: set and unset tags", func(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe instead adding such a func to each object, we could extract one integration test with parametrized setup and run it for all objects instead? (I know that currently we had setting/unsetting tags in each object but with the rework, we can pick more centralized approach)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can do it in the 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.

Oky (keeping it open then)

@@ -326,7 +326,7 @@ func TestInt_MaterializedViews(t *testing.T) {
err := client.Views.Alter(ctx, alterRequestSetTags)
require.NoError(t, err)

returnedTagValue, err := client.SystemFunctions.GetTag(ctx, tag.ID(), id, sdk.ObjectTypeTable)
returnedTagValue, err := client.SystemFunctions.GetTag(ctx, tag.ID(), id, sdk.ObjectTypeMaterializedView)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why in some tests we check for err nil and in some we also validate the tag's value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's because when a tag is not set, GetTag returns an error. I'd like to change this, so that nil is returned as a tag value in the 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.

Oky (keeping it open then)

pkg/sdk/testint/schemas_integration_test.go Show resolved Hide resolved
Copy link

Integration tests failure for 6be59db0f614df9aabfad26cf726cd8aaa12b114

Copy link

Integration tests cancelled for ca92d70faf456812972fca68a8e0029be707de8c

Copy link

Integration tests failure for bf2af836a4744222fb783b4191d1a1aaf6d56b23

@sfc-gh-jmichalak sfc-gh-jmichalak merged commit 893b288 into main Oct 21, 2024
8 of 9 checks passed
@sfc-gh-jmichalak sfc-gh-jmichalak deleted the tag-v1 branch October 21, 2024 08:51
sfc-gh-jmichalak pushed a commit that referenced this pull request Nov 8, 2024
##
[0.98.0](v0.97.0...v0.98.0)
(2024-11-08)

Feature scope readiness for V1:
[link](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/v1-preparations/ESSENTIAL_GA_OBJECTS.MD)
([Roadmap
reference](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/ROADMAP.md#wrap-up-the-functional-scope)).
:exclamation: Migration guide: [v0.97.0 ->
v0.98.0](https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/MIGRATION_GUIDE.md#v0970--v0980)

### 🎉 What's new
- New resources:
- authentication_policy
([#3098](#3098)),
references
[#2880](#2880)
- external_volume
([#3106](#3106)),
partially references
[#2980](#2980)
- stream_on_directory_table
([#3129](#3129))
- stream_on_view
([#3150](#3150))
- primary_connection, secondary_connection
([#3162](#3162))
- secret_with_basic_authentication, secret_with_generic_string,
secret_with_oauth_authorization_code_grant,
secret_with_oauth_client_credentials
([#3110](#3110)),
([#3141](#3141))
- New data sources:
- connections
([#3155](#3155)),
([#3173](#3173))
- secrets
([#3131](#3131))
- Reworked:
- provider configuration hierarchy
([#3166](#3166)),
references
[#1881](#1881),
[#2145](#2145),
[#2925](#2925),
[#2983](#2983),
[#3104](#3104)
- provider configuration fields
([#3152](#3152))
streams data source
([#3151](#3151))
- SDK upgrades:
- Upgrade tag SDK
([#3126](#3126))
- Recreate streams when they are stale
([#3129](#3129))
### 🔧  Misc
- Add object renaming research summary
([#3172](#3172))
- Test support for object renaming
([#3130](#3130)),
([#3147](#3147)),
([#3154](#3154))
- Add tests to issue
[#3117](#3117)
([#3133](#3133))
- New roadmap entry
([#3158](#3158))
- Test more authentication methods
([#3178](#3178))
- Minor fixes
([#3174](#3174))
### 🐛  Bug fixes
- Apply various fixes
([#3176](#3176)),
this addresses BCR 2024_08, references
[#2717](#2717),
[#3005](#3005),
[#3125](#3125),
[#3127](#3127),
[#3153](#3153)
- Connection and secret data sources tests
([#3177](#3177))
- Fix grant import docs
([#3183](#3183)),
resolves
[#3179](https://github.com/Snowflake-Labs/terraform-provider-snowflake/discussions/3179)
- Fix user resource import
([#3181](#3181))
- Handle external type changes in stream resources
([#3164](#3164))
- Do not use OR REPLACE on initial creation in resources with
copy_grants
([#3129](#3129))
- Address issue
[#2201](#2201)
by introducing new stream resources

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