Support grants #130
Support grants #130
Conversation
| {% set should_revoke = should_revoke(existing_relation, full_refresh_mode=True) %} | ||
| {% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %} | ||
|
|
||
| {{ run_hooks(post_hooks) }} |
There was a problem hiding this comment.
Should it also be applicable to incremental.sql?
There was a problem hiding this comment.
Checked on dbt-snowflake and applied the same onto ours.
It seems not tested within core dbt. It's however the same hooks but only applied on full_refresh.
Views is tested and is working, although no hooks in the materialization are called.
docker-compose-starburst.yml
Outdated
| hive-metastore: | ||
| image: ghcr.io/innoverio/hive-metastore-multiarch:sha-28c28bc #TODO: replace with Starburst hive container once config is merged |
There was a problem hiding this comment.
replace with Starburst hive container once config is merged
Could you please use a concrete ticket in the TODO ? For me it is not clear at the moment what is the limitation of Starburst HMS which requires the usage of a different HMS image.
There was a problem hiding this comment.
We need to discuss if we want it or not, the Starburst HMS doesn't offer direct access to the Hive config, so we would need to add environment variables and convert them to valid Hive configurations.
We also would need to consider if we want the Starburst HMS to support these exotic Hive settings.
There was a problem hiding this comment.
If I am not wrong, it's only hive.users.in.admin.role parameter which is not exposed in Starburst HMS, it shouldn't be a big deal to expose it there. Are there any other parameters which are not available in Starburst HMS?
There was a problem hiding this comment.
No other parameters (except for hive.users.in.admin.role) are required. I'll do a PR there but let that not stop us from releasing this one.
setup.py
Outdated
| @@ -86,7 +86,7 @@ def _get_dbt_core_version(): | |||
| }, | |||
| install_requires=[ | |||
| "dbt-core~={}".format(dbt_version), | |||
| "trino==0.315.0", | |||
| "trino==0.317.0", | |||
There was a problem hiding this comment.
Are all these changes related to supporting grants or could they be extracted to smaller unrelated PRs ?
There was a problem hiding this comment.
We need 0.317.0 as in that version we introduced roles support.
We even need the not yet released 0.318.0 as we need another fix as prepared statements are not yet fully working in 0.317.0 (see the CI failures and trinodb/trino-python-client#249)
There was a problem hiding this comment.
Sorry for the confusion. The PR contains 6 commits - which seemed to me (correct me if I'm wrong) are not all related to one another. Could you extract the commits which are unrelated to the grants to one or more preparatory PRs ?
There was a problem hiding this comment.
I'll make a PR with only Trino, Starburst and trino-python-client version upgrade but I'm waiting for the 0.318.0 version to be released.
|
|
||
|
|
||
| @pytest.mark.hive | ||
| # TODO: setup roles in Galaxy |
There was a problem hiding this comment.
Is this a TODO relevant for this PR ?
There was a problem hiding this comment.
This is a reminder to test grants with SEP and Galaxy.
There was a problem hiding this comment.
Please include issue number in the comment here.
There was a problem hiding this comment.
Added issues for both Galaxy and SEP tests.
|
@findinpath, @hovaesco : outside of the container image (that's being replaced right now). Are there any more blockers in this PR. I would like to make a new release of dbt 1.2 today (including grants) and then focus on the 1.3 version. |
Overview
Support grants as described in https://docs.getdbt.com/reference/resource-configs/grants
Tested using
sql-standardaccess control on a Hive catalog.Checklist
README.mdupdated and added information about my changechangie newto create a changelog entry