paramiko-ssh-config-support#2941
Conversation
eedgar
changed the title
eedgar
force-pushed
the
paramiko-ssh-config-support
branch
from
d12a519 to
ee1d2bb
Compare
samirsss
left a comment
samirsss
left a comment
There was a problem hiding this comment.
The changes look good to me. @bigmstone @humblearner @Kami do you folks want to review this as well?
|
Thank you for your PR. |
1 similar comment
|
Thank you for your PR. |
| return client | ||
|
|
||
| @staticmethod | ||
| def _get_conninfo_from_ssh_config(conninfo): |
There was a problem hiding this comment.
A minor thing that's concerning is that you are passing an object and mutating it inside a static method. Plus the method is called _get_conninfo...
Should we refactor this to something like:
if cfg.CONF.ssh_runner.use_ssh_config:
conninfo_ssh_config = self._get_conninfo_from_ssh_config_for_host(host)
conninfo.update(conninfo_ssh_config)
There was a problem hiding this comment.
Yeah, what lakshmi has said - In fact, I would prefer this method to just retrieve info from the SSH config and return a dictionary and the merging to happen somewhere else.
|
You should also edit CHANGELOG.rst and call it as a new feature and put your name next to it :) |
|
Besides those comments, LGTM. Once you have them addressed, I'll merge. |
|
Nice work, thanks. It would also be great if you can add corresponding documentation to https://github.com/stackstorm/st2docs |
| help='Use the .ssh/config file. Useful to override ports etc.') | ||
| help='Use the .ssh/config file. Useful to override ports etc.'), | ||
| cfg.StrOpt('ssh_config_path', | ||
| default='.ssh/config', |
There was a problem hiding this comment.
Shouldn't this default to ~/.ssh/config if we want it to use config from the user under which action runner process is running (stanley)?
Otherwise it seems it will default to a relative path based on the cwd where action runner is started.
eedgar
force-pushed
the
paramiko-ssh-config-support
branch
from
ee1d2bb to
d3ef1d1
Compare
|
Fixed here: #3032 |
|
Thanks to both of you - let's continue in #3032 then. |
6 participants
No description provided.