-
Notifications
You must be signed in to change notification settings - Fork 1
Refactor _grants_collect_schemas macro to accept schema_names and is_… #23
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
base: main
Are you sure you want to change the base?
Changes from all commits
3a4f751
b243bf4
1ebb7f5
7fcdc28
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| {% macro grant_internal_share_read(share_name, include_schemas, dry_run=false) %} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (bug_risk): Macro name and file name are mismatched and appear to duplicate an existing macro, which is likely to override the original implementation. This file is named |
||
| {% if flags.WHICH in ['run', 'run-operation'] %} | ||
| {% if execute %} | ||
| {% set database = target.database %} | ||
| {% if include_schemas is none %} | ||
| {% set include_schemas = [] %} | ||
| {% elif include_schemas is string %} | ||
| {% set include_schemas = [include_schemas] %} | ||
| {% elif include_schemas is not iterable %} | ||
| {% set include_schemas = [] %} | ||
| {% endif %} | ||
| {% if include_schemas | length == 0 %} | ||
| {% do log("No schemas to grant for share: " ~ share_name, info=True) %} | ||
| {% do return(none) %} | ||
| {% endif %} | ||
| {% set schemas = dbt_dataengineers_utils._grants_collect_schemas(include_schemas, is_exclude_list=false) %} | ||
| {% do log("Granting SELECT on all tables and views in all schemas for share: " ~ share_name ~ " (dry_run=" ~ dry_run ~ ")", info=True) %} | ||
| {% for schema in schemas %} | ||
| {% set grant_usage %} | ||
| grant usage on schema {{ database }}.{{ schema }} to share {{ share_name }}; | ||
| {% endset %} | ||
| {% if dry_run %} | ||
| {% do log(grant_usage, info=True) %} | ||
| {% else %} | ||
| {% do run_query(grant_usage) %} | ||
| {% endif %} | ||
| {# Grant SELECT on all tables in schema #} | ||
| {% set grant_tables_sql %} | ||
| grant select on all tables in schema {{ database }}.{{ schema }} to share {{ share_name }}; | ||
| {% endset %} | ||
| {% if dry_run %} | ||
| {% do log(grant_tables_sql, info=True) %} | ||
| {% else %} | ||
| {% do run_query(grant_tables_sql) %} | ||
| {% endif %} | ||
|
|
||
| {# Grant SELECT on views individually #} | ||
| {% set views_query %} | ||
| show views in schema {{ database }}.{{ schema }}; | ||
| {% endset %} | ||
| {% set views_result = run_query(views_query) %} | ||
| {% if execute and views_result is not none %} | ||
| {% for row in views_result %} | ||
| {% set grant_view_sql %} | ||
| grant select on view {{ database }}.{{ schema }}.{{ row.name }} to share {{ share_name }}; | ||
| {% endset %} | ||
| {% if dry_run %} | ||
| {% do log(grant_view_sql, info=True) %} | ||
| {% else %} | ||
| {% do run_query(grant_view_sql) %} | ||
| {% endif %} | ||
| {% endfor %} | ||
| {% endif %} | ||
| {% endfor %} | ||
| {% endif %} | ||
| {% endif %} | ||
| {% endmacro %} | ||
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.
suggestion: Callers and helper both manage
INFORMATION_SCHEMAin exclude mode, leading to duplicated responsibility.The new
is_exclude_listflag makes_grants_collect_schemasappendINFORMATION_SCHEMAfor exclude lists, but some callers (e.g.grant_*macros) still append it before calling_grants_collect_schemas(..., is_exclude_list=true). Even though double-insertion is avoided, the responsibility for excludingINFORMATION_SCHEMAis now split. It would be clearer to handle this in a single place (either only in_grants_collect_schemasor only in the callers).Suggested implementation:
To fully implement the suggestion and avoid duplicated responsibility:
_grants_collect_schemas(for example, ingrant_*macros in the grants-related SQL macro files).is_exclude_lististrueor omitted to use the default), remove any manual appending of"INFORMATION_SCHEMA"to the schema list before calling_grants_collect_schemas.{% set exclude_schemas = exclude_schemas + ["INFORMATION_SCHEMA"] %}{% do exclude_schemas.append("INFORMATION_SCHEMA") %}before
_grants_collect_schemas(exclude_schemas, is_exclude_list=true)so that
"INFORMATION_SCHEMA"is no longer added explicitly.is_exclude_listisfalse) do not rely on this helper to manage"INFORMATION_SCHEMA"; they should pass the exact include list they want.