[GitHub] [superset] eric-briscoe commented on a diff in pull request #22689: feat: add ssh tunneling to dynamic form for Database Connection UI

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

eric-briscoe commented on code in PR #22689:
URL: https://github.com/apache/superset/pull/22689#discussion_r1067565937


##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -549,7 +555,10 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
         DB.backend === db?.engine || DB.engine === db?.engine,
     ) as DatabaseObject
   )?.engine_information?.allow_ssh_tunneling;
-  const sshTunneling = isFeatureEnabled(FeatureFlag.SSH_TUNNELING);
+  const sshTunneling =

Review Comment:
   nit: would be more clear to use name like: `sshTunnelingEnabled` or `isSshTunnelingEnabled`



##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1287,6 +1296,37 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
     });
   };
 
+  const renderSSHTunnelForm = () => (
+    <SSHTunnelForm
+      isEditMode={isEditMode}
+      sshTunneling={sshTunneling}
+      db={db as DatabaseObject}

Review Comment:
   Not new to this PR so maybe not in scope, but is there any validation on `db` to ensure it is a `DatabaseObject`?  Forcing a coercion here and on line 1304 basically removes any useful type checking.   We may want to add a type guard or other validation that db does have the structure SSHTunnelForm expects.



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