[GitHub] [superset] villebro commented on a change in pull request #13214: fix: Changes to support presto impersionation with ldap

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

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



##########
File path: superset/db_engine_specs/base.py
##########
@@ -909,24 +909,9 @@ def modify_url_for_impersonation(
             url.username = username
 
     @classmethod
-    def get_configuration_for_impersonation(  # pylint: disable=invalid-name
+    def update_connect_args_for_impersonation(
         cls, uri: str, impersonate_user: bool, username: Optional[str]
-    ) -> Dict[str, str]:
-        """
-        Return a configuration dictionary that can be merged with other configs
-        that can set the correct properties for impersonating users
-
-        :param uri: URI
-        :param impersonate_user: Flag indicating if impersonation is enabled
-        :param username: Effective username
-        :return: Configs required for impersonation
-        """
-        return {}
-
-    @classmethod
-    def connect_arg_for_impersonation(
-        cls, uri: str, impersonate_user: bool, username: Optional[str]
-    ) -> Dict[str, str]:
+    ) -> Dict[str, Any]:

Review comment:
       I think we should add `connect_args` to the signature and mutate it here, as I'm not sure we can always call `connect_args.update()` in `core.py` (the required changes might not merge nicely into the original dict). Also, if we change it like this, the method doesn't need to return the object, as it's just mutating it within the method.




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

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