Skip to content
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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

iireland-ii
Copy link

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.

@iireland-ii iireland-ii requested a review from a team as a code owner November 14, 2024 10:16
@iireland-ii iireland-ii requested review from Amnah199 and removed request for a team November 14, 2024 10:16
@github-actions github-actions bot added integration:snowflake type:documentation Improvements or additions to documentation labels Nov 14, 2024
@@ -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
Copy link
Contributor

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?

Copy link
Author

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:

Copy link
Contributor

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,
Copy link
Contributor

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

@davidsbatista
Copy link
Contributor

@iireland-ii thanks for your contribution! I've left a few comments

Can you also please test if an "old" serialized SnowflakeTableRetriever object can be deserialized with the code in this new version you are proposing?

@davidsbatista
Copy link
Contributor

@iireland-ii do you still have time to work on this PR?

@iireland-ii
Copy link
Author

hi david going to try make some time for it this week!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration:snowflake type:documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants