-
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: create a workaround for granting privileges on all pipes #2477
Conversation
Integration tests failure for 45533d4c4eecf851868cd8921cf9fc31b78d25f9 |
0acd3e3
to
42be4e4
Compare
Integration tests failure for 42be4e47ab6d7ffd27e55b33a89d9e3bf7d5f27f |
on.SchemaObject.All.InDatabase, | ||
on.SchemaObject.All.InSchema, | ||
func(pipe Pipe) error { | ||
return v.client.Grants.RevokePrivilegesFromAccountRole( |
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.
it seems like a few things could be extracted here: condition, all parameters are the same, the only diff is GrantPrivilegesToAccountRole
vs RevokePrivilegesFromAccountRole
, right?
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.
And also the following ones
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.
I tried to extract it, but in condition on
is used and it's different between account and database roles, InDatabase and InSchema I was able to extract and just pass it to the method (same as previous on
is different) and the SDK call had to be exposed, because pass-through parameters are also different between functions (like privileges, role and opts).
@@ -205,6 +206,83 @@ func TestInt_GrantAndRevokePrivilegesToAccountRole(t *testing.T) { | |||
require.NoError(t, err) | |||
assert.Equal(t, 0, len(grants)) | |||
}) | |||
|
|||
t.Run("grant and revoke on all pipes", func(t *testing.T) { |
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.
can we have a negative test that will show that multiple errors are returned?
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.
Done.
Integration tests failure for a47e59f0a25b56461ec8212b2b42644bf8003b4a |
🤖 I have created a release *beep* *boop* --- # Release notes [0.86.0](v0.85.0...v0.86.0) (2024-02-15) ## 🎉 **What's new** * add refresh_mode and initialize to dynamic tables ([#2437](#2437)) ([d301b20](d301b20)) * add resource snowflake_user_password_policy_attachment ([#2162](#2162)) ([#2307](#2307)) ([93af462](93af462)) * create a workaround for granting privileges on all pipes ([#2477](#2477)) ([64f2346](64f2346)) * Handle IMPORTED PRIVILEGES privileges in privilege-to-role granting resources ([#2471](#2471)) ([eb20051](eb20051)) * use external functions ([#2454](#2454)) ([417d473](417d473)) * use funcs from sdk ([#2462](#2462)) ([a5f969c](a5f969c)) * use sdk for procedures ([#2450](#2450)) ([94ac78a](94ac78a)) * Use sdk in table constraint resource ([#2466](#2466)) ([d685603](d685603)) * Use tables from SDK ([#2453](#2453)) ([fdb4f88](fdb4f88)) ## 🔧 **Misc** * Add migration notes to the docs and change jira integration ([#2497](#2497)) ([b17f1af](b17f1af)) * Change email and issue reporter ([#2470](#2470)) ([5865655](5865655)) * Grants migration guide ([#2455](#2455)) ([62c70fd](62c70fd)) * Remove unused old implementation from snowflake pkg ([#2458](#2458)) ([2d0e508](2d0e508)) * update password policy attachment ([#2485](#2485)) ([6ec9ff7](6ec9ff7)) ## 🐛 **Bug fixes** * allow DT warehouse to be updated in-place ([#2439](#2439)) ([d565af1](d565af1)) * correct test dependencies ([#2493](#2493)) ([dfb247f](dfb247f)) * FileFormat not detecting changes correctly ([#2436](#2436)) ([018bb74](018bb74)) * Fix few smaller issues ([#2507](#2507)) ([a836871](a836871)) * Fix functions and small other fixes ([#2503](#2503)) ([0d4aba4](0d4aba4)), closes [#2490](#2490) * Fix tag tests in view and in materialized view ([#2457](#2457)) ([2de942a](2de942a)) * Fix task related issues ([#2479](#2479)) ([0385650](0385650)) * Fix tests that base on default data retention ([#2465](#2465)) ([682e28c](682e28c)) * grant privileges to share test terraform dependencies ([#2473](#2473)) ([ede8d95](ede8d95)) * parameter issues ([#2463](#2463)) ([7ee4986](7ee4986)) * parse dynamic table query from DDL ([#2438](#2438)) ([d76815c](d76815c)) * Remove title and body temporarily from jira integration ([#2499](#2499)) ([672c97d](672c97d)) * SHOW GRANTS mapping for share data type ([#2508](#2508)) ([feb4d44](feb4d44)) * user error handling ([#2486](#2486)) ([dfa52b2](dfa52b2)) --- 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>
granting privileges on all pipes to role/database role
is not allowed in Snowflake. As it was just one case, we decided to create an internal workaround that would makeon_all pipes
work as it was supported by Snowflake. The workaround works as follows:snowflake_grant_privileges_to_role
,snowflake_grant_privileges_to_account_role
, andsnowflake_grant_privileges_to_database_role
Reference (check the text at the bottom of <object_type>)