-
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
Added Copy Grants to Snowflake Adapter #1747
Added Copy Grants to Snowflake Adapter #1747
Conversation
I noticed the failures were due to the failure of the Database Integration Tests. This is the same thing that happened on my last PR, will I need to followup or redo the PR? |
hey @Carolus-Holman - I just need to kick off the tests here - I'll do that today! More info on this here: https://github.com/fishtown-analytics/dbt/blob/dev/louisa-may-alcott/CONTRIBUTING.md#submitting-a-pull-request |
@@ -9,7 +9,7 @@ class SnowflakeAdapter(SQLAdapter): | |||
ConnectionManager = SnowflakeConnectionManager | |||
|
|||
AdapterSpecificConfigs = frozenset( | |||
{"transient", "cluster_by", "automatic_clustering", "secure"} | |||
{"transient", "cluster_by", "automatic_clustering", "secure", "copy_grants"} |
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 like this is a pep8
formatting error. Can you try swinging the copy_grants
element onto the next line? There's an example of how to run the flake8
tests locally in the Makefile
!
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 think the linter still isn't happy - can you try something like this?
AdapterSpecificConfigs = frozenset(
{"transient", "cluster_by", "automatic_clustering", "secure",
"copy_grants"}
)
Azure is pointing to the wrong build again - the correct build is passing here: https://dev.azure.com/fishtown-analytics/dbt/_build/results?buildId=690 |
@Carolus-Holman thanks for this PR! Are you able to squash your 6 commits into a single commit for this PR? Squashing commits is a nice thing to do, as it helps keep the git history clean. We don't require commits to be squashed, but in this case, I think it could be a good idea. Definitely let me know if you'd like a hand with this! |
Need some help Squashing the commits. When I committed the squash? it just added another commit of the .gitnore file. |
hey @Carolus-Holman - that's pretty strange! I think we'll want to undo the last commit, Check out the steps below -- these should help you squash your commits into a single commit with a good commit message.
Couple of quick things:
Let me know if you have any questions! |
f09da6a
to
226247d
Compare
Ok I finished the script below. Let see if that worked.
From: Drew Banin <[email protected]>
Sent: Tuesday, September 17, 2019 8:23 AM
To: fishtown-analytics/dbt <[email protected]>
Cc: Carolus Holman <[email protected]>; Mention <[email protected]>
Subject: Re: [fishtown-analytics/dbt] Added Copy Grants to Snowflake Adapter (#1747)
hey @Carolus-Holman<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Carolus-2DHolman&d=DwMCaQ&c=fa_WZs7nNMvOIDyLmzi2sMVHyyC4hN9WQl29lWJQ5Y4&r=cGUsYTIA9hWUxMz_Z6vNK0clp0WE7M1VgX5Tf_8y8YY&m=U-g25N3lbKU804DaSrrd6h3pElqM2t91UsEt-uJrirY&s=3rIp_6MJsR02tVsRr9dOYID7Zg84Rm156ThcZcCirds&e=> - that's pretty strange! I think we'll want to undo the last commit, f09da6a, before rebasing.
Check out the steps below -- these should help you squash your commits into a single commit with a good commit message.
# hard reset your branch to the commit before f09da6a
$ git reset --hard f09da6a^
# "soft" reset your branch to commit `5dc776f8`
# this is the commit that you made changes on top of
$ git reset --soft 5dc776f
# inspect the git state
$ git status
# commit your changes
$ git commit -m '(#1744) add copy grants option for Snowflake'
# Finally, force push your changes to re-write history. Be careful!
$ git push --force origin dev/louisa-may-alcott
Couple of quick things:
* in the future, you should make a branch off of dev/louisa-may-alcott for features like this, eg. feature/snowflake-copy-grants. This isn't a problem for dbt -- it will just help keep your local dev environment clean!
* always be careful when force-pushing to a branch!
Let me know if you have any questions!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_fishtown-2Danalytics_dbt_pull_1747-3Femail-5Fsource-3Dnotifications-26email-5Ftoken-3DACV2EVSJP3ZCHDYBQ6OA6NLQKDKZ3A5CNFSM4IWJPNXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD64QCZA-23issuecomment-2D532218212&d=DwMCaQ&c=fa_WZs7nNMvOIDyLmzi2sMVHyyC4hN9WQl29lWJQ5Y4&r=cGUsYTIA9hWUxMz_Z6vNK0clp0WE7M1VgX5Tf_8y8YY&m=U-g25N3lbKU804DaSrrd6h3pElqM2t91UsEt-uJrirY&s=-17Q6-mJmpUVAIZa6XeMQXp58D-l9EV472pPzg9mu3c&e=>, or mute the thread<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_ACV2EVRK3ALAZ7WUPUFD7R3QKDKZ3ANCNFSM4IWJPNXA&d=DwMCaQ&c=fa_WZs7nNMvOIDyLmzi2sMVHyyC4hN9WQl29lWJQ5Y4&r=cGUsYTIA9hWUxMz_Z6vNK0clp0WE7M1VgX5Tf_8y8YY&m=U-g25N3lbKU804DaSrrd6h3pElqM2t91UsEt-uJrirY&s=dF7uQi8vuUTJkBu_lSUkinf0W2CgHb5h42T97PwAhc8&e=>.
|
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 @Carolus-Holman - merging this!
I am doing dbt Fundamentals course and noticed that even though I set the copy_grants to true on the dbt_project.yml, and the result from the log of dbt run showed the copy grants option, the view created on Snowflake did not have this option at all. Did I miss anything? |
Added Copy Grants to Snowflake Adapter for both Views and Tables, excluding temporary tables.
Usage
{{ config(materialized='table',copy_grants=true) }}
Files Updated: adapters.sql and impl.py for Snowflake Plugin.