-
Notifications
You must be signed in to change notification settings - Fork 126
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
adding privatekey auth param #1190
base: main
Are you sure you want to change the base?
adding privatekey auth param #1190
Conversation
@@ -82,11 +84,16 @@ def __init__( | |||
:param db_schema: Name of the schema to use. | |||
:param warehouse: Name of the warehouse to use. | |||
:param login_timeout: Timeout in seconds for login. By default, 60 seconds. | |||
:param private_key_file: Location of 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.
this docstring is very confusing - what's key_file
? can you maybe rephrase it?
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 chose to match the snowflake connector params to keep it consistent with the snowflake documentation, see sample code here:
I've added some further comments to the docstring to include a note to the following link:
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 can't see your changes - also my comment was not about the params names themselves, rather the docstring explaining what they are used for.
@@ -69,6 +69,8 @@ def __init__( | |||
user: str, | |||
account: str, | |||
api_key: Secret = Secret.from_env_var("SNOWFLAKE_API_KEY"), # noqa: B008 | |||
private_key_file: Optional[str] = None, | |||
private_key_file_pwd: Optional[str] = None, |
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.
this should be using the Secret
- check above how it was done for api_key
@iireland-ii thanks for your contribution! I've left a few comments Can you also please test if an "old" serialized |
@iireland-ii do you still have time to work on this PR? |
hi david going to try make some time for it this week! |
Related Issues
fixes
#1179
Proposed Changes:
Added PrivateKey and PrivateKeyPwd parameters to allow more secure connection methodologies.
How did you test it?
Tested locally with dev branch using live Snowflake instance and hatch.