-
Notifications
You must be signed in to change notification settings - Fork 420
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: grant privileges to database role resource #2306
feat: grant privileges to database role resource #2306
Conversation
ea01954
to
5dc79aa
Compare
Integration tests failure for ea0195400fbb65487bc01647ceef34f7c5fba360 |
Integration tests failure for 5dc79aa845bdf766bd68335b11cf22fa8ccd5e47 |
Integration tests failure for 631db5a0051627e8377d7552f5d70920fcb1ccf0 |
Integration tests failure for 6767e9d066b25d56e55dc46c8523ef6fd966eaad |
Integration tests failure for 778b734a1d5527f0e077a9c0506e3efda5cb2197 |
6641d4f
to
8e15592
Compare
Integration tests failure for 6641d4f8d38902d4303920f95c0f501557cb05a2 |
Integration tests failure for 8e155921a1ef5e0526c3e6e83566249364e1b0ea |
Integration tests failure for 9d0ab46419ddda2018a9da44e294b24732402906 |
pkg/resources/testdata/TestAcc_GrantPrivilegesToDatabaseRole_AlwaysApply/test.tf
Outdated
Show resolved
Hide resolved
Integration tests failure for ee9de174531145edc55bb43226f3af3c96628906 |
ee9de17
to
f935612
Compare
Integration tests failure for f935612de69e2ef300d2c104a1b6b423e2fd94c0 |
Integration tests failure for c1633d9b596d9891e0161b4fb12486d836e6aa9a |
c1633d9
to
b3f933a
Compare
Integration tests failure for b3f933a1a248afdd63d68956f4a26d421ca81d38 |
pkg/resources/grant_privileges_to_database_role_acceptance_test.go
Outdated
Show resolved
Hide resolved
pkg/resources/grant_privileges_to_database_role_acceptance_test.go
Outdated
Show resolved
Hide resolved
1f8ebb6
to
eacccb0
Compare
Integration tests failure for eacccb0f09af6eb2b2d608d064e82c8eadc49354 |
Integration tests failure for 1f8ebb65bc553b29a68da64d141fd588435a0cdf |
1a551af
to
b3ae822
Compare
b3ae822
to
7657134
Compare
Integration tests failure for 1a551aff5b610109a554913572735103329903fb |
Integration tests failure for b3ae8223f2b26384858300dd5a23cc1049f81ec0 |
Integration tests failure for 7657134a02cd000866ea877e5bc7d38168fae612 |
Description: "If true, the resource will always produce a “plan” and on “apply” it will re-grant defined privileges. It is supposed to be used only in “grant privileges on all X’s in database / schema Y” or “grant all privileges to X” scenarios to make sure that every new object in a given database / schema is granted by the account role and every new privilege is granted to the database role. Important note: this flag is not compliant with the Terraform assumptions of the config being eventually convergent (producing an empty plan).", | ||
}, | ||
"always_apply_trigger": { | ||
Type: schema.TypeString, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it make more sense for this to be a boolean flag instead of a string? also I think it would be circumsantially usefully to set manually. for example, you could check the current time and only apply once per day, or once per week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first, it was a boolean, but I thought it could be acting wrongly in some scenarios. For example, the only place where this flag could be switched is in the Read operation (because it is invoked on every tf action), but then we could end up in a situation where sometimes it triggers and sometimes doesn't (mostly because Update operation calls Read at the end). With random strings, we don't have to think about it and wonder if it will be switched correctly because it will be always a different value.
pkg/resources/testdata/TestAcc_GrantPrivilegesToDatabaseRole_OnAllSchemasInDatabase/test.tf
Outdated
Show resolved
Hide resolved
8a13e19
to
cc39e2c
Compare
Integration tests failure for 8a13e1951fb3afae98beb56be4d519f367906587 |
Integration tests failure for cc39e2c096990fb6a2b8fe0ed4e4971ef13d7d28 |
🤖 I have created a release *beep* *boop* --- ## [0.83.0](v0.82.0...v0.83.0) (2024-01-11) ### 🎉 **What's new:** * Add create streamlit privilege to the SDK ([#2303](#2303)) ([be01d5f](be01d5f)) * grant privileges to database role resource ([#2306](#2306)) ([0311cf8](0311cf8)) ### 🐛 **Bug fixes:** * Add secondary account and fix tests ([#2324](#2324)) ([da6ca73](da6ca73)) * external tables issues ([#2334](#2334)) ([ae41691](ae41691)) * Fix test because of the date ([#2312](#2312)) ([9a9ea33](9a9ea33)) * Fix warehouse read and resource monitor empty set ([#2319](#2319)) ([05f96c6](05f96c6)), closes [#2318](#2318) [#2316](#2316) * goreleaser for mfa token caching ([#2320](#2320)) ([4fef709](4fef709)) --- 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>
Add new snowflake_grant_privileges_to_database_role resource.
TODO: