-
-
Notifications
You must be signed in to change notification settings - Fork 382
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 support for SSH connection via aliases from ~/.ssh/config
#790
Conversation
@@ -65,7 +79,7 @@ def parse_uri(uri_as_string): | |||
uri_path=_unquote(split_uri.path), | |||
user=_unquote(split_uri.username), | |||
host=split_uri.hostname, | |||
port=int(split_uri.port or DEFAULT_PORT), | |||
port=int(split_uri.port) if split_uri.port else None, |
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.
Why are we setting port to None instead of the default here?
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.
If I recall correctly, this is to ensure that non-default ports from configuration files are loaded correctly. We think it best that the parse_uri function should not inject additional information, such as a default port, to a URI, as this is the job of the configuration parser. This is important for cases where a non-default port is specified in the config file for a connection, but not in the URI as-provided. If parse_uri injects the default here, then the non-default port from the config will be ignored, which is not what we want. We only want to override ports specified in the config if explicitly provided by the user as part of the URI.
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.
Thank you!
…open into patch-2 * 'develop' of https://github.com/RaRe-Technologies/smart_open: Propagate __exit__ call to underlying filestream (piskvorky#786) Retry finalizing multipart s3 upload (piskvorky#785) Fix `KeyError: 'ContentRange'` when received full content from S3 (piskvorky#789) Add support for SSH connection via aliases from `~/.ssh/config` (piskvorky#790) Make calls to smart_open.open() for GCS 1000x faster by avoiding unnecessary GCS API call (piskvorky#788) Add zstandard compression feature (piskvorky#801) Support moto 4 & 5 (piskvorky#802) Secure the connection using SSL when connecting to the FTPS server (piskvorky#793) upgrade dev status classifier to stable (piskvorky#798) Fix formatting of python code (piskvorky#795)
Add support for SSH connection via aliases from
~/.ssh/config
Motivation
For commonly-used connections, machines with pre-existing SSH configurations, and general convenience, we find it useful to be able to infer (or interpolate, as you please) a full connection configuration from the host alias in the standard SSH config file.
We suggest that configurations as specified as such should only be used as default values, and if alternatives username and port are provided as part of the file URI, these should override pre-existing values. For example,
ssh://new-user@host-alias:<new-port>/...
should override the pre-defined port and username in the SSH configuration file.Implementation Details
As currently implemented, the connection configuration interpolation supports the following configuration options, which are passed to
paramiko.client.SSHClient.connect
:As an example, the following configuration would be fully and properly utilised when called with
smart_open.open("ssh://another-host/...")
Tests
Additional unit tests have been added in
smart_open/tests/test_ssh.py
, following the existing test pattern.Checklist
Before you create the PR, please make sure you have: