-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
CT-808 grant adapter tests #5447
Conversation
Draft PR for commenting and discussion. I had to change a couple of things in ct-660 to get the basic test working, i.e. remove resource.type from the grant statement. |
427c131
to
8e9a4f3
Compare
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
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.
Nice work! Thanks for putting up the draft PR.
With a few small modifications (ct-808-grant_adapter_tests...jerco/grants-testing, the substance of which I've noted in comments below), I was able to get this running successfully against both Postgres + Snowflake (using slightly modified version of dbt-labs/dbt-snowflake#178).
It looks like there are two approaches to validate grant behavior:
- Querying the database after the model runs. I have a little bit of code that might be able to help in doing this across adapters.
- Checking the logs, which is trickier to match up across adapters. This does feel like a good way to validate that we're taking the "most efficient" strategy possible, following the
should_revoke=True|False
logic. We can countshow
/grant
/revoke
statements, and validate that we're only running them in cases where we actually need to.
core/dbt/include/global_project/macros/adapters/apply_grants.sql
Outdated
Show resolved
Hide resolved
core/dbt/include/global_project/macros/adapters/apply_grants.sql
Outdated
Show resolved
Hide resolved
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.
Looks great
* init push or ct-660 work * changes to default versions of get_show_grant_sql and get_grant_sql * completing init default versions of all macros being called for look over and collaboration * minor update to should_revoke * post pairing push up (does have log statements to make sure we remove) * minor spacing changes * minor changes, and removal of logs so people can have clean grab of code * minor changes to how get_revoke_sql works * init attempt at applying apply_grants to all materialzations * name change from recipents -> grantee * minor changes * working on making a context to handle the diff gathering between grant_config and curreent_grants to see what needs to be revoked, I know if we assign a role, and a model becomes dependent on it we can't drop the role now still not seeing the diff appear in log * removing logs from most materializations to better track diff of grants generation logs * starting to build out postgres get_show_grant_sql getting empty query errors hopefully will clear up as we add the other postgres versions of macros and isn't a psycopg2 issue as indicated by searching * 6/27 eod update looking into diff_grants variable not getting passed into get_revoke_sql * changes to loop cases * changes after pairing meeting * adding apply_grants to create_or_replace_view.sql * models are building but testing out small issues around revoke statement never building * postgrest must fixes from jeremy's feedback * postgres minor change to standarize_grants_dict * updating after pairing with dough and jeremey incorporating the new version of should revoke logic. * adding ref of diff_of_two_dicts to base keys ref * change of method type for standardize_grants_dict * minor update trying to fix unit test * changes based on morning feedback * change log message in default_apply_grants macro * CT-808 grant adapter tests (#5447) * Create initial test for grants Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> * rename grant[privilege] -> grant_config[privilege] * postgres macro rename to copy_grants * CT-808 more grant adapter tests (#5452) * Add tests for invalid user and invalid privilege * Add more grant tests * Macro touch ups * Many more tests * Allow easily replacing privilege names * Keep adding tests * Refactor macros to return lists, fix test * Code checks * Keep tweaking tests * Revert cool grantees join bc Snowflake isnt happy * Use Postgres/BQ as standard for standardize_grants_dict * Code checks * add missing replace * small replace tweak, add additional dict diffs * All tests passing on BQ * Add type cast to test_snapshot_grants * Refactor for DRYer apply_grants macros Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com> * update to main, create changelog, whitespace fixes Co-authored-by: Gerda Shank <gerda@dbtlabs.com> Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>
* init push or ct-660 work * changes to default versions of get_show_grant_sql and get_grant_sql * completing init default versions of all macros being called for look over and collaboration * minor update to should_revoke * post pairing push up (does have log statements to make sure we remove) * minor spacing changes * minor changes, and removal of logs so people can have clean grab of code * minor changes to how get_revoke_sql works * init attempt at applying apply_grants to all materialzations * name change from recipents -> grantee * minor changes * working on making a context to handle the diff gathering between grant_config and curreent_grants to see what needs to be revoked, I know if we assign a role, and a model becomes dependent on it we can't drop the role now still not seeing the diff appear in log * removing logs from most materializations to better track diff of grants generation logs * starting to build out postgres get_show_grant_sql getting empty query errors hopefully will clear up as we add the other postgres versions of macros and isn't a psycopg2 issue as indicated by searching * 6/27 eod update looking into diff_grants variable not getting passed into get_revoke_sql * changes to loop cases * changes after pairing meeting * adding apply_grants to create_or_replace_view.sql * models are building but testing out small issues around revoke statement never building * postgrest must fixes from jeremy's feedback * postgres minor change to standarize_grants_dict * updating after pairing with dough and jeremey incorporating the new version of should revoke logic. * adding ref of diff_of_two_dicts to base keys ref * change of method type for standardize_grants_dict * minor update trying to fix unit test * changes based on morning feedback * change log message in default_apply_grants macro * CT-808 grant adapter tests (dbt-labs#5447) * Create initial test for grants Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> * rename grant[privilege] -> grant_config[privilege] * postgres macro rename to copy_grants * CT-808 more grant adapter tests (dbt-labs#5452) * Add tests for invalid user and invalid privilege * Add more grant tests * Macro touch ups * Many more tests * Allow easily replacing privilege names * Keep adding tests * Refactor macros to return lists, fix test * Code checks * Keep tweaking tests * Revert cool grantees join bc Snowflake isnt happy * Use Postgres/BQ as standard for standardize_grants_dict * Code checks * add missing replace * small replace tweak, add additional dict diffs * All tests passing on BQ * Add type cast to test_snapshot_grants * Refactor for DRYer apply_grants macros Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com> * update to main, create changelog, whitespace fixes Co-authored-by: Gerda Shank <gerda@dbtlabs.com> Co-authored-by: Jeremy Cohen <jeremy@dbtlabs.com> Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com>
resolves #5437
Description
Adapter zone grant tests.
Checklist
changie new
to create a changelog entry