Support insecure mode in SnowflakeHook #20106
Conversation
boring-cyborg
bot
added
area:providers
kind:documentation
provider:snowflake
docs/apache-airflow-providers-snowflake/connections/snowflake.rst
Outdated
Show resolved
Hide resolved
| return False | ||
| elif value.lower() == 'true': | ||
| return True | ||
| return value |
There was a problem hiding this comment.
I'm not sure about the return - what it means it terms of boolean value?
There was a problem hiding this comment.
If it can be converted to a bool value then a conversation is performed, but if an incorrect value is passed it is passed unchanged and then the layer below can handle it.
There was a problem hiding this comment.
I changed the behavior and for an invalid value an exception is now raised
There was a problem hiding this comment.
Did you not push the change? The implementation here does not raise, from what I can tell.
There was a problem hiding this comment.
Also we have airflow.utils.strings.to_boolean(). Its behaviour is different from the one you have here, but might be more expected since the function is used in other parts of Airflow.
There was a problem hiding this comment.
to_boolean looks interesting. Updated and. pushed.
| conn_params = self._get_conn_params() | ||
| return self._conn_params_to_sqlalchemy_uri(conn_params) | ||
|
|
||
| def _conn_params_to_sqlalchemy_uri(self, conn_params): |
There was a problem hiding this comment.
| def _conn_params_to_sqlalchemy_uri(self, conn_params): | |
| def _conn_params_to_sqlalchemy_uri(self, conn_params: Dict): |
| :return: the created engine. | ||
| """ | ||
| if engine_kwargs is None: | ||
| engine_kwargs = {} |
There was a problem hiding this comment.
| engine_kwargs = {} | |
| engine_kwargs = engine_kwargs or {} |
mik-laj
force-pushed
the
insecure-mode-snowflake
branch
from
3ac23d3
to
84a9047
Compare
github-actions
bot
added
the
okay to merge
|
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
|
Tests are failing. |
mik-laj
force-pushed
the
insecure-mode-snowflake
branch
from
cdfe453
to
25d72d4
Compare
mik-laj
force-pushed
the
insecure-mode-snowflake
branch
from
25d72d4
to
4fd4eae
Compare
docs/apache-airflow-providers-snowflake/connections/snowflake.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Tzu-ping Chung <[email protected]>
Close: #19797
CC: @mattpolzin @harishkrao @sfc-gh-turbaszek
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.