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. The user should add the private key and password in `encryption_extra` 
similar to Trino: 
https://superset.apache.org/docs/databases/trino#authentications . You are free 
to choose the structure of the JSON object, but I think the format chosen by 
the Trino spec is good, as it makes it easy to add additional authentication 
types as they become supported later. 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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to