[GitHub] [superset] villebro commented on a diff in pull request #21180: feat: add snowflake private key support

[GitBox <gi...@apache.org>]

villebro commented on code in PR #21180:
URL: https://github.com/apache/superset/pull/21180#discussion_r959140949


##########
superset/databases/commands/test_connection.py:
##########
@@ -73,8 +74,12 @@ def run(self) -> None:
 
             database.set_sqlalchemy_uri(uri)
             database.db_engine_spec.mutate_db_for_connection_test(database)
-
-            engine = database.get_sqla_engine()
+            
+            engine = None
+            if "snowflake" in str(url) and "privatekey" in str(url):

Review Comment:
   @xiayanzheng yes - please see how the Trino connector is currently doing it: 
   In short here's what I think should work:
   
   1. You assume the user should add the private key and password in `encryption_extra` similar to Trino: https://superset.apache.org/docs/databases/trino#authentications . It's important you also add documentation for the precise format in which users should add this information to the JSON object in this file: https://github.com/apache/superset/blob/master/docs/docs/databases/snowflake.mdx
   2. You move the cryptography logic to `superset.db_engine_specs.snowflake.SnowflakeEngineSpec.update_encrypted_extra_params` like Trino does it here: https://github.com/apache/superset/blob/034ee1c3c1fda20b238ba1cc9093a4156d9fa01e/superset/db_engine_specs/trino.py#L183-L221



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org