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 resource #1187

Merged
merged 38 commits into from
Aug 24, 2022
Merged

feat: tag association resource #1187

merged 38 commits into from
Aug 24, 2022

Conversation

sfc-gh-swinkler
Copy link
Collaborator

@sfc-gh-swinkler sfc-gh-swinkler commented Aug 23, 2022

This PR changes the way that tags work. Instead of using tagschema on each resource, it creates a new resource specifically for associating tags with resources. this fixes a lot of the weird dependency problems people had using the old method. the tagschema is still supported but marked as deprecated and will be removed in a future release.

also there was a lot of weird problems with integration tests failing that are fixed that probably should have been done in a separate PR,

Test Plan

  • acceptance tests

References

@sfc-gh-swinkler sfc-gh-swinkler changed the title Tag reference feat: tag association resource Aug 23, 2022
pkg/resources/helpers_test.go Outdated Show resolved Hide resolved
pkg/resources/tag_test.go Outdated Show resolved Hide resolved
pkg/resources/user_test.go Outdated Show resolved Hide resolved
@github-actions
Copy link

Integration tests failure for a27dcaf11a2ae39e14543cdc95dc09e659edae12

@github-actions
Copy link

Integration tests failure for 99e526e559e56939b2851945fb2aaf8f0debffdc

@github-actions
Copy link

Integration tests failure for f78b32cfb9aed514daf0ec4df6edea94900480da

@github-actions
Copy link

Integration tests failure for 979bcd1e58e0cc317fcdb0fe26da76d493df824c

@github-actions
Copy link

Integration tests failure for bcac47f24489bd09377673c2b8ddead5632a9eef

@github-actions
Copy link

Integration tests failure for 2cac339e00d1715bcc33455f4dc6d6f2d7272c98

@github-actions
Copy link

Integration tests failure for 53fe3a189541acb3ce3e8f6b9579eab952944489

@github-actions
Copy link

Integration tests failure for e25740768c6671f564082083452481cf3b493229

@github-actions
Copy link

Integration tests failure for 834aa32d1329bb9c6f241c8712cf7384071ef47c

@github-actions
Copy link

Integration tests failure for 27bd0f9f4ed5e5c66d5ed5bd4ff8b280e4110420

@github-actions
Copy link

Integration tests failure for bbfb1e367411d75afd522953a8cccc5c0e439b3f

@github-actions
Copy link

Integration tests failure for 4cff2e58e4c834e3faefb1879b60170cf162f97c

@github-actions
Copy link

Integration tests failure for 7fa001eee0ff6702f0ca65c89b13394c76643dab

@github-actions
Copy link

Integration tests failure for c6da4f4badba028d2557265a57477e1e8be61e80

@github-actions
Copy link

Integration tests success for 43250b511fd363e0957bbbc7cc60997062716a54

@github-actions
Copy link

Integration tests success for e0cbb90fd9538d786f6e84ada213869cfc9291c5

}

func ListTagAssociations(tb *TagAssociationBuilder, db *sql.DB) ([]tagAssociation, error) {
stmt := fmt.Sprintf(`SELECT * FROM SNOWFLAKE.ACCOUNT_USAGE.TAG_REFERENCES WHERE TAG_NAME = '%v' AND DOMAIN = '%v' AND TAG_VALUE = '%v'`, tb.tagName, tb.objectType, tb.tagValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

@sfc-gh-swinkler Would you be so kind to provide a reasoning why the tags are read this way?

I see multiple potential issues here:

  • minor nitpick: tags on deleted objects are not filtered (OBJECT_DELETED IS NULL)
  • the query filters only on tag name instead of the fully qualified identifier (missing TAG_DATABASE and TAG_SCHEMA) - if I have two tags with the same name in different schemas, this would also select the other one
  • why via the ACCOUNT_USAGE view? It would be possible to query the tags without any latency using SYSTEM$GET_TAG and compare the value afterwards with the state, if this is needed.
  • Also, currently the query doesn't include the object name, so we are just querying if there is a tag on any object of the same domain with the given tag value set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@christophkreutzer thank you for pointing these out. i must say that i am not an expert at the Snowflake product, so I did not know about this SYSTEM$GET_TAG function. These would be good changes to make. If you can make a request with your account manager, then i can prioritize getting this done.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sfc-gh-swinkler thanks for the fast response! I created a first draft in PR #1261, and I'll let our account team know of the issues if that helps you taking a look at it.

christophkreutzer added a commit to christophkreutzer/terraform-provider-snowflake that referenced this pull request Oct 7, 2022
As discussed in Snowflake-Labs#1187 (comment),
there are some potential issues in the current implementation:
* minor nitpick: tags on deleted objects are not filtered (OBJECT_DELETED IS NULL)
* the query filters only on tag name instead of the fully qualified identifier (missing TAG_DATABASE and TAG_SCHEMA) - if I have two tags with the same name in different schemas, this would also select the other one
* why via the ACCOUNT_USAGE view? It would be possible to query the tags without any latency using SYSTEM$GET_TAG and compare the value afterwards with the state, if this is needed.
* Also, currently the query doesn't include the object name, so we are just querying if there is a tag on any object of the same domain with the given tag value set?

This is only a first draft for discussion.
sfc-gh-swinkler pushed a commit that referenced this pull request Oct 20, 2022
* fix/WIP: clean up tag association read

As discussed in #1187 (comment),
there are some potential issues in the current implementation:
* minor nitpick: tags on deleted objects are not filtered (OBJECT_DELETED IS NULL)
* the query filters only on tag name instead of the fully qualified identifier (missing TAG_DATABASE and TAG_SCHEMA) - if I have two tags with the same name in different schemas, this would also select the other one
* why via the ACCOUNT_USAGE view? It would be possible to query the tags without any latency using SYSTEM$GET_TAG and compare the value afterwards with the state, if this is needed.
* Also, currently the query doesn't include the object name, so we are just querying if there is a tag on any object of the same domain with the given tag value set?

This is only a first draft for discussion.

* deprecate skip_validation on tag associations

* implement read and update on tag associations

similar to how it is done on tags

* fix docs/gofmt
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.

4 participants