[GitHub] [superset] villebro commented on a change in pull request #19252: fix(drill): specify an SA URL parm of `impersonation_target` for drill+sadrill

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

villebro commented on a change in pull request #19252:
URL: https://github.com/apache/superset/pull/19252#discussion_r839342685



##########
File path: tests/unit_tests/db_engine_specs/test_drill.py
##########
@@ -64,4 +64,4 @@ def test_sadrill_impersonation(app_context: AppContext) -> None:
     url = URL("drill+sadrill")
     username = "DoAsUser"
     DrillEngineSpec.modify_url_for_impersonation(url, True, username)
-    assert url.username == username
+    assert url.query["impersonation_target"] == username

Review comment:
       nit: bonus points for adding impersonation tests for `drill+odbc`, `drill+jdbc` and `drill+foobar`.

##########
File path: superset/db_engine_specs/drill.py
##########
@@ -84,7 +87,7 @@ def modify_url_for_impersonation(
         if impersonate_user and username is not None:
             if url.drivername == "drill+odbc":
                 url.query["DelegationUID"] = username
-            elif url.drivername == "drill+jdbc":
+            elif url.drivername in ["drill+sadrill", "drill+jdbc"]:
                 url.query["impersonation_target"] = username
             else:
-                url.username = username
+                logger.warning("impersonation is not supported for %s", url.drivername)

Review comment:
       IMO there's risk here that the user will *think* impersonation is enabled when using other than `odbc`, `sadrill` or `jdbc`. Would it make sense to rather raise an exception in the `else` if we're trying to impersonate and it's not supported by the driver?




-- 
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