Skip to content
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

added option to append any string (arguments) to the ssh command #38

Merged
merged 2 commits into from
Jan 7, 2016

Conversation

ksere
Copy link
Contributor

@ksere ksere commented Sep 10, 2015

No description provided.

@jedrichards
Copy link
Owner

Hey thanks! Can we have a test for this? See this test for an example of how we test ssh stuff https://github.com/jedrichards/rsyncwrapper/blob/master/tests/remote-src.js

Also could you expand a bit in the readme? Maybe a short sentence or example about when/why this option is useful? What sort of arguments often need to be appended?

@ksere
Copy link
Contributor Author

ksere commented Sep 13, 2015

Hi,

It just appends anything in sshCmdArgs to rshCmd after the -p and -i options.

I used this to fix issues with Windows ports of ssh, which couldn't find things like the knownhosts file, so using something like
sshCmdArgs: '-oUserKnownHostsFile=/myhome/.ssh/known_hosts', solves the problem. In the same way you can add any option the ssh-client supports.

I will try to figure out "Vows Async BDD" and write a relevant test.

@jedrichards
Copy link
Owner

Cool, sounds handy. You can probably just copy and paste that test file to create your own. The basic idea is that you set noExec to true which stops the actual rsync invocation from happening, and then you test that the command string rsyncwrapper generated is what you expect it to be.

jedrichards added a commit that referenced this pull request Jan 7, 2016
added option to append any string (arguments) to the ssh command
@jedrichards jedrichards merged commit 244eb56 into jedrichards:master Jan 7, 2016
@jedrichards
Copy link
Owner

I'm going to merge this now without tests because it looks like it'll help with #37, and add a few tests myself in the next few days then make a new release. Thanks! :)

@ksere
Copy link
Contributor Author

ksere commented Jan 7, 2016

Hi, sorry i got caught up with other stuff and never got around to write
those tests myself. Thanks!

On 7 January 2016 at 12:10, Jed Richards notifications@github.com wrote:

I'm going to merge this now without tests because it looks like it'll help
with #37 #37, and add a
few tests myself in the next few days then make a new release. Thanks! :)


Reply to this email directly or view it on GitHub
#38 (comment)
.

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.

2 participants