-
Notifications
You must be signed in to change notification settings - Fork 569
Add SSH tunnel support #1301
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
Add SSH tunnel support #1301
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this! Yes, it would be very nice to give users a built-in way to tunnel. Please go ahead with adding some tests.
This feature will need supporting documentation. Are you able to PR supporting docs into https://github.com/dbcli/pgcli.com? I suggest adding a dedicated page to describe ssh tunneling, and linking it in the docs.
@@ -84,6 +86,14 @@ | |||
|
|||
from textwrap import dedent | |||
|
|||
try: | |||
import sshtunnel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is sshtunnel difficult to install? Is there a reason we would not want to make it one of the default requirements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it optional because it is not a feature that most pgcli users will have the need for I think and it adds paramiko and sshtunnel as dependencies. But let me know if you prefer it wasn't optional.
Glad that you'd accept it! I'll add tests and documentation. |
@sweenu It seems that |
ae3adc4
to
15b9181
Compare
886b7ef
to
f994219
Compare
@j-bennet I have added tests and documentation. I also tried running black with python 3.6, but it changed nothing, so I wouldn't know where the pipeline error would be coming from if it fails again. In the end, do you want to make it optional to install sshtunnel? |
2539f4e
to
ffde1b5
Compare
de1c479
to
c0c122e
Compare
@sweenu Ok to leave |
Codecov Report
@@ Coverage Diff @@
## master #1301 +/- ##
==========================================
+ Coverage 83.80% 83.90% +0.09%
==========================================
Files 21 21
Lines 2594 2709 +115
==========================================
+ Hits 2174 2273 +99
- Misses 420 436 +16
Continue to review full report at Codecov.
|
Tests are ok now. Going to merge the PR. |
* Add initial sshtunnel support * Force CI to rerun. * Fix unit test for 3.6. * Black. Co-authored-by: Irina Truong <i.chernyavska@gmail.com>
* Add initial sshtunnel support * Force CI to rerun. * Fix unit test for 3.6. * Black. Co-authored-by: Irina Truong <i.chernyavska@gmail.com>
Closes #459
Description
This PR adds support for creating SSH tunnels thanks to the sshtunnel package.
If you want this PR to be merged, then I'll write some tests. Let me know :)
I know it might increase the support load, but I think it's worth it. Right now, basically everyone that need tunnels are doing their own wonky scripts to create tunnels and shut them down automatically after pgcli exits (I have one myself). It is work and time that can be avoided by having a central way of doing it. It's also very nice to be able to give pgcli the DB uri as if you were directly connecting to it and not have to change it to match your local bind of the SSH tunnel.
For the future:
Checklist
changelog.rst
.AUTHORS
file (or it's already there).pip install pre-commit && pre-commit install
), and ranblack
on my code.