rusackas commented on PR #40139: URL: https://github.com/apache/superset/pull/40139#issuecomment-4633896642
@aminghadersohi Thanks for the thorough re-review — addressed the actionable items in 51cf2cdfda: - **BLOCKER (circular import):** added a comment on the deferred `get_default_port` import naming the `superset.utils.ssh_tunnel → superset.databases.ssh_tunnel.models → superset.extensions` cycle. - **MEDIUM (PasswordRequiredException untested):** added `test_create_tunnel_passphrase_protected_key_without_password` — generates a passphrase-encrypted ed25519 key, calls `create_tunnel` with no password, and asserts `PasswordRequiredException` propagates and `open_tunnel` is never invoked. - **MEDIUM (all-types-fail untested):** added `test_create_tunnel_invalid_key_raises_combined_error` — passes garbage and asserts the `SSHException` message lists all three attempted type names. - **MEDIUM (verify PKey.from_private_key polymorphic):** confirmed it is *not* polymorphic — the base `PKey.from_private_key` is inherited by each subclass but still parses only that subclass's format; there's no auto-detecting base loader. paramiko 3.2+ does have a polymorphic `PKey.from_path()`, but it takes a filesystem path and would require writing key material to disk, so the per-class try-loop stands. Clarified the docstring accordingly. - **NIT (no-op continue):** removed. - **NIT (RSA test too weak):** strengthened the ed25519 and RSA tests with `isinstance(..., Ed25519Key/RSAKey)` assertions on the parsed `ssh_pkey`. - **NIT (duplicated setup):** extracted `_make_manager()` / `_make_ssh_tunnel()` helpers shared across all four key-path tests. - **NIT (assertion messages):** consistent failure messages across the key-type tests. Also folded in the two earlier non-blocking NITs (the `last_exc` NOTE and the `PKey.from_path()` docstring context). All six tests pass and pre-commit is clean. -- 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]
