Skip to content

Conversation

sweenu
Copy link
Contributor

@sweenu sweenu commented Dec 22, 2021

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:

  • It'd be nice to be able to pass extra arguments to sshtunnel. But this should already cover most usecases, special config can just go in the ssh conf.
  • To have a section in the config that will allow matching hostnames and automatically create a tunnel for those.

Checklist

  • I've added this contribution to the changelog.rst.
  • I've added my name to the AUTHORS file (or it's already there).
  • I installed pre-commit hooks (pip install pre-commit && pre-commit install), and ran black on my code.
  • Please squash merge this pull request (uncheck if you'd like us to merge as multiple commits)

Copy link
Contributor

@j-bennet j-bennet left a 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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@sweenu
Copy link
Contributor Author

sweenu commented Dec 23, 2021

Glad that you'd accept it! I'll add tests and documentation.

@j-bennet
Copy link
Contributor

@sweenu It seems that black check is failing in CI, take a look at that too.

@sweenu sweenu force-pushed the master branch 2 times, most recently from ae3adc4 to 15b9181 Compare December 26, 2021 16:57
@sweenu sweenu force-pushed the master branch 3 times, most recently from 886b7ef to f994219 Compare December 26, 2021 19:00
@sweenu
Copy link
Contributor Author

sweenu commented Dec 26, 2021

@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?

@j-bennet
Copy link
Contributor

j-bennet commented Jan 17, 2022

@sweenu Ok to leave sshtunnel as-is. Please take a look at those test failures in CI. Were you able to run tests successfully in your local environment, including behave tests?

@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2022

Codecov Report

Merging #1301 (f2dfc3d) into master (3a95530) will increase coverage by 0.09%.
The diff coverage is 79.41%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
pgcli/completion_refresher.py 90.58% <50.00%> (-1.08%) ⬇️
pgcli/pgexecute.py 80.36% <74.72%> (-1.18%) ⬇️
pgcli/main.py 77.62% <85.13%> (+1.69%) ⬆️
pgcli/__init__.py 100.00% <100.00%> (ø)
pgcli/packages/parseutils/tables.py 97.67% <100.00%> (ø)
pgcli/pgcompleter.py 96.95% <100.00%> (ø)

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 78843ac...f2dfc3d. Read the comment docs.

@j-bennet
Copy link
Contributor

Tests are ok now. Going to merge the PR.

@j-bennet j-bennet merged commit ed9d123 into dbcli:master Feb 18, 2022
j-bennet added a commit to sweenu/pgcli that referenced this pull request Feb 18, 2022
* Add initial sshtunnel support

* Force CI to rerun.

* Fix unit test for 3.6.

* Black.

Co-authored-by: Irina Truong <i.chernyavska@gmail.com>
j-bennet added a commit to sweenu/pgcli that referenced this pull request Feb 18, 2022
* Add initial sshtunnel support

* Force CI to rerun.

* Fix unit test for 3.6.

* Black.

Co-authored-by: Irina Truong <i.chernyavska@gmail.com>
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.

Setup and teardown an SSH tunnel
3 participants