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

Updating net-ssh parameter names #220

Merged
merged 1 commit into from Oct 27, 2017
Merged

Updating net-ssh parameter names #220

merged 1 commit into from Oct 27, 2017

Conversation

SnarkyArtificer
Copy link
Contributor

In response to net-ssh/net-ssh@c3fbcdc

@plribeiro3000
Copy link
Member

Thanks @JustMcAfee.

To change that we need to either lock net-ssh version to the newest one or
check for both and allow people to use older versions of net-ssh (just in case the are locked)

I believe we should go the latter and set both parameters. WDYT?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 74.021% when pulling 14e2c32 on JustMcAfee:master into 9fabe1c on fog:master.

@SnarkyArtificer
Copy link
Contributor Author

My understanding was that net::ssh is not included as an explicit dependency to fog by intent (I am not aware of the reasons for this), so locking it to a minimum version seems unlikely. The purpose of this PR is to suppress the warning messages the latest net:ssh includes if :paranoid is at all specified, even if it is alongside :verify_ssh_key. I think if the plan is to continue to not specify net::ssh (which seems incorrect to me personally, as it's explicitly called), then I can specify both. Ideally, though, we'd re-add the dependency.

@plribeiro3000
Copy link
Member

@JustMcAfee Sure. This decision was made a while ago to keep support of old ruby versions.
Even that it might not be the case now, we need to specify both keys in this change.

We can revisit this net-ssh once we are done with the ruby < 2 odyssey.

@SnarkyArtificer
Copy link
Contributor Author

@plribeiro3000 Understood and updated accordingly

@plribeiro3000
Copy link
Member

👍

@plribeiro3000 plribeiro3000 merged commit 65e33d0 into fog:master Oct 27, 2017
@geemus
Copy link
Member

geemus commented Oct 27, 2017

The dependency was omitted because some people use fog for purely storage (or similar), where it doesn't make any sense. As we split out individual providers it becomes less of an issue, but that was why we did it initially anyway. I agree that it's a bit odd and unfortunate, but there still is at least some reason behind it.

Thanks!

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.

None yet

4 participants