-
Notifications
You must be signed in to change notification settings - Fork 79
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
Leverage run_session for SFTP #381
Leverage run_session for SFTP #381
Conversation
@miq-bot add-reviewer carbonin |
$log.debug "MiqSshUtil::get_file - Copying file #{@host}:#{from} to #{to}." if $log | ||
data = sftp.download!(from, to) | ||
data = ssh.sftp.download!(from, to) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an odd change, and I can't find documentation for getting to an sftp connection using the ssh session object and also this:
[7] pry(main)> Net::SSH::Connection::Session.instance_methods.include?(:sftp)
=> false
[8] pry(main)> Net::SSH::Connection::Session.methods.include?(:sftp)
=> false
I trust that you've tested this, but I'm not comfortable making this change which doesn't really bring any upside when I can't find this behavior documented anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, if you can find the documentation for this, then this looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, I needed to also require 'net/sftp'
because it patches.
[2] pry(main)> require 'net/ssh'
=> true
[3] pry(main)> Net::SSH::Connection::Session.instance_methods.include?(:sftp)
=> false
[4] pry(main)> require 'net/sftp'
=> true
[5] pry(main)> Net::SSH::Connection::Session.instance_methods.include?(:sftp)
=> true
Makes sense to me then 👍
@fdupont-redhat is there anything that needs this change for the Hammer branch? I'm not sure that this actually adds any functionality, so I'm tempted to label |
@carbonin AFAIK, there's no code depending on that change in Hammer. So ok for |
This PR leverages the
run_session
method to get a Net::SSH::Connection::Session object that can be used to access the SFTP subsystem. This standardize connection via SSH.