mwilliamson / spur.py

Run commands and manipulate files locally or over SSH using the same interface
BSD 2-Clause "Simplified" License
267 stars 37 forks source link

Added a sock parameter to SshShell #42

Closed lindycoder closed 8 years ago

lindycoder commented 8 years ago

It is a pass-through to the paramiko.client.connect method

resolves #41

Another part of this feature could be to have an helper to craft the proxy_command that can be passed to the sock= param

mwilliamson commented 8 years ago

Thanks for the pull request! I don't suppose you'd mind adding a test to cover the new functionality?

lindycoder commented 8 years ago

I can give it a try, i was a bit lost in your test suite, that's why i didn't venture in there but i'll look more closely and see what i can do

mwilliamson commented 8 years ago

Since this only affects SSH, the place to put the test is probably ssh_tests, similarly to some of the other tests that test SSH-only functionality. I'm not entirely sure what a sensible test for this functionality would like though.

lindycoder commented 8 years ago

There's the test, i copied paramiko's socket connection routine and toned it down and passed the pre-connected socket as a param.

Since it's doing the same thing as if there was nothing passed, the test is not perfect but it does test the code...

I had to move stuff in testing to make them available outside to avoid duplicating environment variables parsing logic

Tell me what you think.

mwilliamson commented 8 years ago

Presumably if you're explicitly passing in a socket then the port is unused? Therefore, an appropriate test might be to check that the connection works when using an invalid port (perhaps with a check that the connection fails with the invalid port when not passing sock).

There are also a couple of minor stylistic changes that I'd like to make -- would you rather I point them out and let you fix them, or for me to fix them up myself? I'm happy either way.

Were you planning to add a function to help construct ProxyCommand? By no means essential for this pull request though.

lindycoder commented 8 years ago

First off i must admin i'm not an SSH pro what i'm trying to do is get rid of an in-house paramiko wrapper for an opensource one and yours seems very good for what i need. So i'mn having a hard time understanding all the implications and or relations around ssh transport and the use of a proxy command, i just have a code that works that i try to fit in yours.

That being said, about the port, i can see that paramiko use it even if you provide a sock, i'm not sure to which extent but if you check the code you can see it.

For the stylistics change, go on i don't mind, it's your code after all

And about the ProxyCommand builder, i started but ran into a big problem : The exact same code using a proxy command works in py27 but not in py34. It looks like paramiko's problem first hand, but i'm trying to dig down and given my knowledge it's not going well :)

mwilliamson commented 8 years ago

That being said, about the port, i can see that paramiko use it even if you provide a sock, i'm not sure to which extent but if you check the code you can see it.

From the (very!) quick glance I took, it seemed like it was only used for generating the host for checking keys, which shouldn't be a problem since the tests automatically accept all keys by default.

lindycoder commented 8 years ago

It seems to be working i updated the PR with this test, give me your stylistics changes and i'll apply them :)

mwilliamson commented 8 years ago

Thanks, it's looking good. I left a few stylistic comments, apologies for nitpicking!

lindycoder commented 8 years ago

I addressed the comments, thanks for the feedback, check if you liked what i did in the readme

mwilliamson commented 8 years ago

Looks good, thanks.