This repository was archived by the owner on May 17, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 294
Allow for Snowflake key-pair authentication with --dbt
#450
Closed
Closed
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Really appreciate the contribution @jakedave
Want to make sure you saw some of the downstream snowflake code, which is currently in a separate repo (likely to change).
https://github.com/datafold/sqeleton/blob/6c532733e1a3efe329b573691d3d9d18dcf4f410/sqeleton/databases/snowflake.py#L166
If we set conn_info["key"], that if statement will eventually fire with some code similar to
_get_snowflake_private_key
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.
Thank you for the callout - I was not aware of this code!
To be clear, are you asking that I follow the pattern outlined there for error-handling? Happy to follow that example!
Uh oh!
There was an error while loading. Please reload this page.
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.
More calling out that this change would not be DRY as is.
_get_snowflake_private_key
should not be necessary and we should limit the connection code in dbt.py to only putting the needed values in conn_info, as much of the key functionality already exists. If we'd like to extend that functionality, we should do so in this file/repoUh oh!
There was an error while loading. Please reload this page.
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.
ah, so remove the private key restriction that is imposed in
dbt.py
and handle downstream.It seems by the way the code flows that a wider refactor would be required -
it attempts to connect with the provided credentials before ever getting to the downstream codeI believe that code is not called in the case of dbtOur company is trying to evaluate this product; we are unable to do so while data-diff does not support key pair auth for dbt
Would it be possible to contribute this as is? I'm happy to provide further contribution in terms of DRYing out the code in the future
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.
Are you saying it's creating a snowflake connection somewhere other than here? https://github.com/datafold/sqeleton/blob/6c532733e1a3efe329b573691d3d9d18dcf4f410/sqeleton/databases/snowflake.py#L182
I may not be following, I can take a closer look later today
Uh oh!
There was an error while loading. Please reload this page.
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'm not an expert in this but it appears that in the main function, dbt has its own branch of logic which calls the DBT parser's connector as opposed to sqeleton's connector. I don't think the code you linked is called in the case of dbt specifically
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.
Hey Jake, it definitely is, the entry point for the connection creation starts here. Eventually making its way to sqeleton
data-diff/data_diff/dbt.py
Line 153 in d21c140
I can take a closer look at this later today, and implement the changes I'm describing if that helps.
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.
yea, I was about to recorrect myself. Following the inheritance as an outsider here is non-trivial. I can keep looking but if you get to it that would be great too
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.
@dlawin is the plan to refactor the sqeleton code while removing exceptions related to the presence of a priv key here? as is, sqeleton does not allow you to specify a keyphrase for a priv key and seems like it could benefit from at least the
_get_snowflake_private_key
function