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

Fix: require keyring on snowflake #2789

Merged
merged 3 commits into from
Sep 25, 2020
Merged

Fix: require keyring on snowflake #2789

merged 3 commits into from
Sep 25, 2020

Conversation

jtcohen6
Copy link
Contributor

@jtcohen6 jtcohen6 commented Sep 24, 2020

resolves #2779

Description

  • Require extra dependency on all installations of dbt-snowflake: snowflake-connector-python[secure-local-storage], which is really just keyring

Alternatives

We could put snowflake-connector-python[secure-local-storage] under extras_require instead of install_requires, since it's only really needed for external-browser auth. I opted for the more aggressive approach since:

I don't really know what I'm doing, though, when it comes to python deps!

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about python deps either, but the rationale makes sense.

@@ -48,6 +48,7 @@
install_requires=[
'dbt-core=={}'.format(package_version),
'snowflake-connector-python==2.2.10',
'snowflake-connector-python[secure-local-storage]',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the line above can just read

snowflake-connector-python[secure-local-storage]==2.2.10

I think these two things might be equivalent, but it might make sense to combine them here

Copy link
Contributor

@kwigley kwigley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@marksabincarnivaluk
Copy link

Note there are other requirements for Snowflake MFA caching to work with Python connector, listed here:
https://docs.snowflake.com/en/user-guide/security-mfa.html#using-mfa-token-caching-to-minimize-the-number-of-prompts-during-authentication-optional

Python Connector for Snowflake version 2.3.7 (or later).
pip install dbt-snowflake installs snowflake-connector-python==2.3.6. This can be fixed with:
pip install "snowflake-connector-python[secure-local-storage]" --upgrade

Add this line to your Snowflake profile in profiles.yml
authenticator: username_password_mfa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants