Change kwargs in Snowflake URI generation #84
Conversation
…d kwargs as fallback
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
| try: # auto generated connection kwargs Snowflake provider >=4.0.0 | ||
| uri_string = f"snowflake://{self.conn.login}:{self.conn.password}@{self.conn.extra_dejson['account']}.{self.conn.extra_dejson['region']}/{self.conn.extra_dejson['database']}/{schema}?warehouse={self.conn.extra_dejson['warehouse']}&role={self.conn.extra_dejson['role']}" # noqa | ||
| except KeyError: # auto generated connection kwargs Snowflake provider < 4.0.0 |
There was a problem hiding this comment.
I think a more direct approach to check package versions here would be better, something like the answer here:
https://stackoverflow.com/a/32965521
There was a problem hiding this comment.
That makes sense, yes. Changed it :)
One issue: referencing the snowflake package is breaking the unit testing. I tried using the package import and then .version but the snowflake package does not seem to have that as an attribute.
The code itself works fine. Just the test is understandably unhappy.
There was a problem hiding this comment.
Maybe @kaxil has an idea of how packages can be brought in for testing? I haven't done anything like that before afaik
There was a problem hiding this comment.
Oh, I think we just have to install apache-airflow-providers-snowflake as part of the pip install in the testing suite.
There was a problem hiding this comment.
Ok I was wrong about the package approach -- totally forgot about .get() even though I used it today 😓
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
| @@ -21,7 +21,9 @@ | |||
| from datetime import datetime | |||
| from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional, Union | |||
|
|
|||
| import airflow.providers.snowflake # noqa | |||
There was a problem hiding this comment.
This makes snowflake provider a hard dependency for installing great-expectations.
Do we really want that?
There was a problem hiding this comment.
It is not used anywhere as well, why do we need this import line?
There was a problem hiding this comment.
We shouldn't, just took a look at your suggested change and that's definitely the right approach.
Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>
for more information, see https://pre-commit.ci
Fixes #83 .
Snowflake provider 4.0.0 changed the kwargs generated when the connection is defined in the Airflow UI. I left the old kwargs as a fallback.