-
Notifications
You must be signed in to change notification settings - Fork 427
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 based masking policy #1143
feat: tag based masking policy #1143
Conversation
This reverts commit 584f011.
sorry for the long delay in responding to this PR, my wife died in July and I was on grievance leave. I have been catching up over the past couple weeks. This is a complicated PR so i have been thinking about it for a few days now. We are actually in the process of changing how tags work in the provider. It won't break backwards compatibility, but it does affect how new resources that involved tagging should be implemented. What I would like to do is wait until we get the change implemented, which should be later this week, and then address this masking policy attachment after that. I do like this PR, but there will need to be a few changes to get it to align with the new model. If you are not able to make the change, then i can take over and make sure this gets done in the next sprint. |
I'm really sorry to hear that, sending you my condolences. Sounds good to me. Feel free to keep me posted with what changes need to be made once the implementation change is complete. I'll let you know if I'm unable to work on them in a timely manner. Thank you for taking a look at this. |
basically how it will work is that there is a new resource called "snowflake_tag_attachment". it will replace the current tag schema on each resource. below is sample configuration
as you can see we are using a tag_id rather than database, schema and name. the tag_id is a string which has the same infomration, and is flexible in the way that the data is passed in, for example: are all allowed. We should be getting this PR done either tomorrow or Monday. |
looking good, looking real good. as long as this passes integration tests i will merge this |
/ok-to-test sha=32e493e |
Integration tests failure for 32e493e |
@sfc-gh-swinkler I've updated the CI here to include |
@berosen that environment variable is already set |
/ok-to-test sha=227a37f |
Integration tests failure for 227a37f |
1 similar comment
Integration tests failure for 227a37f |
/ok-to-test sha=227a37f |
Integration tests failure for 227a37f |
not sure what is going on here. the Its also not clear to me why this even needs to be set for the masking policy association. Nowhere in the documentation does it state that this needs to be set: https://docs.snowflake.com/en/user-guide/tag-based-masking-policies.html. As far as i know this only needs to be set for Python UDF. Are you setting this locally when you test the resource? If so, what exact line is causing this to fail if the environment variable is not set? Maybe can add some debugging there to figure out what the problem is. Because default warehouse gets set in the provider from environment variable here: |
Integration tests failure for 227a37f |
hmm that's odd. Yes I'm setting that variable locally. The line that needs the warehouse is this one because it uses the |
going to merge this and try to figure out what the problem is on my own. i think you are right, that its probably not picking up the changes. |
thank you for your contribution |
Thanks @sfc-gh-swinkler , look like that was the issue and ended up passing here. Appreciate your help on this. |
closes #1116
Note
SNOWFLAKE_WAREHOUSE
needs to be provided for this resource to query the POLICY_REFERENCES table function.Test Plan
Tested with
TF_ACC=1 go test -v ./... -run TestAcc_TagMaskingPolicyAssociation
References