Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reuse ssh connection #869

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Reuse ssh connection #869

wants to merge 7 commits into from

Conversation

gfrlv
Copy link
Member

@gfrlv gfrlv commented May 25, 2020

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

  • I've added this contribution to the changelog.md.
  • I've added my name to the AUTHORS file (or it's already there).

@gfrlv gfrlv mentioned this pull request May 25, 2020
@amjith
Copy link
Member

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
Copy link
Member Author

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

Codecov Report

Merging #869 (456f86b) into master (f3442f4) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@      Coverage Diff      @@
##   master   #869   +/-   ##
=============================
=============================

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3442f4...57b7520. Read the comment docs.

Copy link
Contributor

@rolandwalker rolandwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also tested connection and completion on the branch. Works.

mycli/main.py Outdated
@@ -387,7 +383,7 @@ def connect(self, database='', user='', passwd='', host='', port='',

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On rebase this should become a little different, like

if port or (host and host != 'localhost') or self.ssh_client:
            socket = ''

@@ -1098,16 +1093,22 @@ def cli(database, user, host, port, socket, password, dbname,
else:
click.secho(alias)
sys.exit(0)

if list_ssh_config:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

except FileNotFoundError as e: should come ahead of more general Exception.

'\tssh_port: %r'
'\tssh_password: %r'
'\tssh_key_filename: %r'
'\tssl: %r',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could still log these values, if we wanted to, right?

@lgtm-com
Copy link

lgtm-com bot commented Mar 1, 2021

This pull request introduces 2 alerts when merging 6392029 into 05c87d8 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Accepting unknown SSH host keys when using Paramiko

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants