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

MiqSshUtil updates #437

Merged
merged 3 commits into from
Jul 2, 2019
Merged

MiqSshUtil updates #437

merged 3 commits into from
Jul 2, 2019

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Jun 24, 2019

This PR fixes a bug in the MiqSshUtil#exec method, and makes a few tweaks to the constructor and exec method in general.

Fixes:

Error handling is currently semi-busted. The on_extended_data hook is missing a yield param. The net result is that there is never any data in the error output. In addition, the status check could break since it defaults to nil, with the result that it will try to call .zero? on a nil object, which will raise an exception. So, I changed the default from nil to 0.

Updates:

  • Some unused code that was commented out was removed, i.e. logging.
  • Some workaround code for older versions of net-ssh that is no longer relevant was removed.
  • You can now configure the use_agent option. It still defaults to false for backwards compatibility.
  • The error handling was updated slightly. It now no longer relies on activesupport methods, and checks the error buffer, too.

You can see these issues in practice with this simple script:

# Assuming you're in the manageiq-gems-pending repo
$:.unshift('lib/gems/pending/util')
require 'MiqSshUtil'
require 'etc'

# Adjust as desired
hostname = 'localhost'
username = Etc.getlogin
password = 'xxxxxxx'
command  = 'bogus'

pubkey = IO.read(File.join(Dir.home, '.ssh', 'id_rsa.pub'))

# You will have to edit MiqSshUtil.rb if you're on master and need 'use_agent'.
opts = {
  :key_data => pubkey,
  :use_agent => true
}

ssh = MiqSshUtil.new(hostname, username, password)

begin
  result = ssh.exec(command)
rescue => err
  puts "OOPS: #{err.message}"
else
  puts "RESULT: #{result}"
end

BEFORE:

RESULT: <---- Blank! And not registered as an error!

AFTER:

OOPS: MiqSshUtil::exec - Command 'bogus' failed: bash: bogus: command not found, status: 127

Partially addresses https://bugzilla.redhat.com/show_bug.cgi?id=1719689

@miq-bot
Copy link
Member

miq-bot commented Jun 24, 2019

Checked commit https://github.com/djberg96/manageiq-gems-pending/commit/c950fa8fede20a014dcc9b3bc885978adbdababd with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 1 offense detected

lib/gems/pending/util/MiqSshUtil.rb

@djberg96
Copy link
Contributor Author

@roliveri How's it look?

@djberg96
Copy link
Contributor Author

@miq-bot add_reviewer @bdunne

@miq-bot miq-bot requested a review from bdunne June 26, 2019 14:10
@djberg96
Copy link
Contributor Author

djberg96 commented Jul 1, 2019

@miq-bot add_label bug, changelog/yes

@djberg96
Copy link
Contributor Author

djberg96 commented Jul 1, 2019

@miq-bot add_label hammer/yes

@djberg96
Copy link
Contributor Author

djberg96 commented Jul 2, 2019

@carbonin Alright, I've added some specs that should demonstrate that it behaves as expected. I didn't want to go down this road originally because I knew it would mean making a couple other refactorings.

Namely, the Channel#exec block had to be moved up (so that channel failure could be checked before command failure), and I had to strip the signal check result because it can (apparently) return a blank string of more than 1 character.

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but leaving the final merge to @carbonin

@Fryguy Fryguy merged commit 02c9fb6 into ManageIQ:master Jul 2, 2019
@Fryguy Fryguy added this to the Sprint 115 Ending Jul 8, 2019 milestone Jul 2, 2019
simaishi pushed a commit that referenced this pull request Jul 2, 2019
@simaishi
Copy link
Contributor

simaishi commented Jul 2, 2019

Hammer backport details:

$ git log -1
commit 7300bf53e17784a7c12dc1133239bc8b76a61f8a
Author: Jason Frey <jfrey@redhat.com>
Date:   Tue Jul 2 16:16:08 2019 -0400

    Merge pull request #437 from djberg96/miq_ssh_util_updates
    
    MiqSshUtil updates
    
    (cherry picked from commit 02c9fb67797331640e2840682d8234f2f4a5415c)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1726439

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants