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

Connect using a SSH transport #634

Merged
merged 7 commits into from
Sep 29, 2018
Merged

Connect using a SSH transport #634

merged 7 commits into from
Sep 29, 2018

Conversation

meeuw
Copy link
Contributor

@meeuw meeuw commented Aug 12, 2018

Description

Connect using a SSH transport

This only works when paramiko is installed, I don't know if we want to hard depend on paramiko (with all it's dependencies).

Checklist

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

@codecov-io
Copy link

codecov-io commented Aug 12, 2018

Codecov Report

Merging #634 into master will decrease coverage by 0.03%.
The diff coverage is 75.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #634      +/-   ##
==========================================
- Coverage   78.05%   78.02%   -0.04%     
==========================================
  Files          26       26              
  Lines        2297     2334      +37     
==========================================
+ Hits         1793     1821      +28     
- Misses        504      513       +9
Impacted Files Coverage Δ
mycli/completion_refresher.py 94.91% <ø> (ø) ⬆️
mycli/sqlexecute.py 86.91% <73.07%> (-2.32%) ⬇️
mycli/main.py 77.37% <80%> (+0.12%) ⬆️

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 7b0e527...7b97f8f. Read the comment docs.


if ssh_host and paramiko:
if not ssh_user:
ssh_user = getpass.getuser()
Copy link
Member

Choose a reason for hiding this comment

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

@meeuw It looks like paramiko will already call getpass.getuser() if the username is None. So, maybe we don't need this on the mycli side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, you're right.

@tsroten
Copy link
Member

tsroten commented Aug 19, 2018

@meeuw I'm not sure exactly what my issue is, but I can't get this to work on python 2.7 or 3.6. I get an error 'Channel' object has no attribute 'recv_into'. It looks like this might fix it: paramiko/paramiko#553

@grooverdan mentions that PR is required on a PyMySQL issue: PyMySQL/PyMySQL#77

@meeuw
Copy link
Contributor Author

meeuw commented Aug 19, 2018

That's stange! I presume you're testing this on MacOS? Have you tried running it on Linux?

@tsroten
Copy link
Member

tsroten commented Aug 19, 2018

@meeuw Yeah, that was tested on Linux (both a Linux client and a Linux server).

I just tested a MacOS client connecting to a Linux SSH server and that worked fine. I'm going to do some more testing with some Linux boxes real quick.

@tsroten
Copy link
Member

tsroten commented Aug 19, 2018

@meeuw Okay, everything continues to work well on the MacOS computer. On the Linux servers I got a variety of errors, some of which I think are related to the ssh server configuration. I'm not too worried about that at this point.

One thing we might want to improve (not in this pull request) is that use opens up a new connection. So, when you are using SSH, it opens another SSH connection and then another database connection. It potentially takes a second or two. That's unnecessary since all use is ultimately doing is setting the sqlexecute.dbname instance variable and triggering the completions thread to run. A new connection isn't needed. But, that's an existing issue, not related to the SSH changes.

I added a couple of changes to the click help messages.

I do think we should add paramiko to the setup.py file, maybe under the optional name ssh. What do you think?

@meeuw
Copy link
Contributor Author

meeuw commented Aug 27, 2018

Yes, it's a good idea to add an optional name in setup.py for ssh, just committed that.
I only see this as a convenience feature, I'll definately keep using my own tunnel script to reuse the same ssh connection.

@tsroten tsroten merged commit 4a11a02 into master Sep 29, 2018
@tsroten tsroten deleted the feature/ssh_transport branch September 29, 2018 20:42
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.

3 participants