betodealmeida commented on code in PR #28246:
URL: https://github.com/apache/superset/pull/28246#discussion_r1583044226
##########
superset/commands/database/update.py:
##########
@@ -76,126 +68,47 @@ def run(self) -> Model: # pylint:
disable=too-many-statements, too-many-branche
)
try:
- database = DatabaseDAO.update(self._model, self._properties,
commit=False)
+ database = DatabaseDAO.update(
+ self._model,
+ self._properties,
+ commit=False,
+ )
database.set_sqlalchemy_uri(database.sqlalchemy_uri)
-
- ssh_tunnel = DatabaseDAO.get_ssh_tunnel(database.id)
-
- if "ssh_tunnel" in self._properties:
- if not is_feature_enabled("SSH_TUNNELING"):
- db.session.rollback()
- raise SSHTunnelingNotEnabledError()
-
- if self._properties.get("ssh_tunnel") is None and ssh_tunnel:
- # We need to remove the existing tunnel
- try:
- DeleteSSHTunnelCommand(ssh_tunnel.id).run()
- ssh_tunnel = None
- except SSHTunnelDeleteFailedError as ex:
- raise ex
- except Exception as ex:
- raise DatabaseUpdateFailedError() from ex
-
- if ssh_tunnel_properties := self._properties.get("ssh_tunnel"):
- if ssh_tunnel is None:
- # We couldn't found an existing tunnel so we need to
create one
- try:
- ssh_tunnel = CreateSSHTunnelCommand(
- database, ssh_tunnel_properties
- ).run()
- except (
- SSHTunnelInvalidError,
- SSHTunnelCreateFailedError,
- SSHTunnelDatabasePortError,
- ) as ex:
- # So we can show the original message
- raise ex
- except Exception as ex:
- raise DatabaseUpdateFailedError() from ex
- else:
- # We found an existing tunnel so we need to update it
- try:
- ssh_tunnel_id = ssh_tunnel.id
- ssh_tunnel = UpdateSSHTunnelCommand(
- ssh_tunnel_id, ssh_tunnel_properties
- ).run()
- except (
- SSHTunnelInvalidError,
- SSHTunnelUpdateFailedError,
- SSHTunnelDatabasePortError,
- ) as ex:
- # So we can show the original message
- raise ex
- except Exception as ex:
- raise DatabaseUpdateFailedError() from ex
-
- # adding a new database we always want to force refresh schema list
- # TODO Improve this simplistic implementation for catching DB conn
fails
- try:
- schemas = database.get_all_schema_names(ssh_tunnel=ssh_tunnel)
Review Comment:
> this is one of the few places where we re fetch current schemas from the
analytical dbs, and we take the chance to update any missing schemas
permissions also. I don't think this is being done on the sqlalchemy hooks
But it would be better to move this code to the hook, right?
--
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-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
