Reuse ssh connection#869
Reuse ssh connection #869
Uh oh!
There was an error while loading. Please reload this page.
Conversation
amjith commented Aug 3, 2020
@pasenor I don't have a good way to check the ssh connection locally, I trust you've done the necessary testing. The code looks good. I merged some older PRs, so now the code has some conflicts with master. Can you please resolve them? |
gfrlv commented Aug 3, 2020
Yes, I have tested the SSH connection. I wonder how to add it to the test suite. Looking at the paramiko documentation, they have a server implementation, maybe we can try to use it for testing the SSH-related stuff? (not in this PR, just thinking out loud) |
codecov-io commented Jan 30, 2021
Codecov Report
@@ Coverage Diff @@## master #869 +/- ## ============================= ============================= Continue to review full report at Codecov.
|
rolandwalker left a comment
rolandwalker left a comment There was a problem hiding this comment.
Also tested connection and completion on the branch. Works.
mycli/main.py Outdated
| database=databaseorcnf['database'] | ||
| # Socket interface not supported for SSH connections | ||
| ifportorhostorssh_hostorssh_port: | ||
| ifportorhostorself.ssh_client: |
There was a problem hiding this comment.
On rebase this should become a little different, like
if port or (host and host != 'localhost') or self.ssh_client: socket = '' | click.secho(alias) | ||
| sys.exit(0) | ||
| iflist_ssh_config: |
There was a problem hiding this comment.
Side note: the list_ssh_config feature is unreliable if the SSH config file uses certain features such as Match. Listing the hosts is not really in the scope of a tool such as mycli, and we ought to consider removing it.
| # Paramiko prior to version 2.7 raises Exception on parse errors. | ||
| # In 2.7 it has become paramiko.ssh_exception.SSHException, | ||
| # but let's catch everything for compatibility | ||
| exceptExceptionaserr: |
There was a problem hiding this comment.
except FileNotFoundError as e: should come ahead of more general Exception.
mycli/sqlexecute.py Outdated
| '\tssh_port: %r' | ||
| '\tssh_password: %r' | ||
| '\tssh_key_filename: %r' | ||
| '\tssl: %r', |
There was a problem hiding this comment.
We could still log these values, if we wanted to, right?
This pull request introduces 2 alerts when merging 6392029 into 05c87d8 - view on LGTM.com new alerts:
|
Description
We were spawning a separate SSH connection for the completer thread. It seems unnecessary since we can just open a second channel on the same connection.
Checklist
changelog.md.AUTHORSfile (or it's already there).