Conversation
gfrlv
force-pushed
the
reuse-ssh-connection
branch
from
cb84e6a to
456f86b
Compare
|
@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? |
|
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 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 = database or cnf['database'] | ||
| # Socket interface not supported for SSH connections | ||
| if port or host or ssh_host or ssh_port: | ||
| if port or host or self.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) | ||
|
|
||
| if list_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 | ||
| except Exception as err: |
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:
|
|
Closing per #1464 (comment) . |
4 participants
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).