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

Update docs and add SSH config file passing #490

Merged
merged 11 commits into from May 29, 2018

Conversation

Projects
None yet
2 participants
@Red-M
Member

Red-M commented May 20, 2018

As per the title, this is for issues #489 and #429.

@takluyver it might be a good idea to do a docs update from commit 6b741f9 as this will solve a small piece of miscommunication I introduced by doing doc updates for my own additions but not for pre-existing features.

Red-M added some commits Apr 9, 2018

Merge pull request #5 from pexpect/master
pxssh changelog for release 4.5 (#484)
@takluyver

The idea looks fine, but there are a couple of minor changes to be made.

@@ -294,8 +294,15 @@ def login (self, server, username, password='', terminal_type='ansi',
session to do so. Setting this option to `False` and not having an active session
will trigger an error.
Set ``ssh_key`` to `True` to force passing the current SSH authentication socket to the
Setting ``ssh_key`` to a file path to an SSH private key will use that SSH key

This comment has been minimized.

@takluyver

takluyver May 21, 2018

Member

I'd like the docstring to be consistent about using either "Set ... to ..." or "Setting ... will ...". I don't have a strong preference which (though it's easier to change the bits you've just added), but it should use the same one throughout.

This comment has been minimized.

@Red-M

Red-M May 23, 2018

Member

Sure thing.

try:
if spawn_local_ssh:
os.path.isfile(ssh_config)
except:

This comment has been minimized.

@takluyver

takluyver May 21, 2018

Member

Catching all exceptions like this is not good practice, because it can obscure other problems. In this case it also won't do what you want, because isfile() returns False rather that raising an exception.

Maybe you want:

if spawn_local_ssh and not os.path.isfile(ssh_config):
    raise ...

This comment has been minimized.

@Red-M

Red-M May 23, 2018

Member

Odd because I copied what was already there.
I'll push this into a function as well as I debating either to leave it as it is or functionize it.

@@ -85,6 +85,13 @@ def test_remote_ssh_tunnel_string(self):
if confirmation_strings!=len(confirmation_array):
assert False, 'String generated from remote tunneling is incorrect.'
def test_ssh_config_passing_string(self):
ssh = pxssh.pxssh(debug_command_string=True)
config_path = '/fakepath/fake/config_file'

This comment has been minimized.

@takluyver

takluyver May 21, 2018

Member

And once the file-exists check is working, you presumably need to create a real named temp file to test with.

It would also be good to have a test with a nonexistent file to check that it errors.

This comment has been minimized.

@Red-M

Red-M May 23, 2018

Member

yeap.

@takluyver

This comment has been minimized.

Member

takluyver commented May 21, 2018

I'm not 100% sure what you mean about a docs update, but if you mean rebuilding them, there's good news! Readthedocs rebuilds docs automatically whenever changes are made, so it will update once this is merged.

@Red-M

This comment has been minimized.

Member

Red-M commented May 23, 2018

I'll fix these on the weekend and its good to hear about the docs at least!

Red-M added some commits May 29, 2018

@Red-M

This comment has been minimized.

Member

Red-M commented May 29, 2018

Sorry for not getting back to this sooner however this should be up to spec now.

to the desired ``hostname``.
Set ``ssh_config`` to a file path string of an SSH client config file will pass that

This comment has been minimized.

@takluyver

takluyver May 29, 2018

Member

These need another minor tweak to fix the grammar for "Set" rather than "Setting":

  • Setting X to Y will do Z
  • Set X to Y to do Z

This comment has been minimized.

@Red-M

Red-M May 29, 2018

Member

Fair enough.

@Red-M

This comment has been minimized.

Member

Red-M commented May 29, 2018

Fixed as per request.

@takluyver takluyver merged commit 99afe20 into pexpect:master May 29, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.7%) to 89.333%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment