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

On error log STDOUT if STDERR is empty #45

Merged

Conversation

agrare
Copy link
Member

@agrare agrare commented May 13, 2019

If a process doesn't correctly log to STDERR then the default .run! log
message isn't very helpful. This logs command_result.error if it is
present, otherwise it logs command_result.output.

@coveralls
Copy link

coveralls commented May 13, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling a17cba3 on agrare:log_output_if_command_result_error_is_nil into 31528d6 on ManageIQ:master.

@agrare
Copy link
Member Author

agrare commented May 13, 2019

cc @djberg96 @carbonin

@agrare
Copy link
Member Author

agrare commented May 13, 2019

Re: the rubocop comment, I tried presence first but it isn't available and I didn't feel like pulling in active_support just for this

@djberg96
Copy link

👍

@djberg96
Copy link

I didn't realized we owned AwesomeSpawn, lol.

@agrare
Copy link
Member Author

agrare commented May 13, 2019

@djberg96 yup so it is even more awesome than you thought 😄

@carbonin
Copy link
Member

I tried presence first but it isn't available and I didn't feel like pulling in active_support just for this

@agrare Isn't String#present? also active_support?

irb(main):002:0> "".present?
NoMethodError: undefined method `present?' for "":String
Did you mean?  prepend
	from (irb):2
	from /home/ncarboni/.rubies/ruby-2.4.3/bin/irb:11:in `<main>'
irb(main):003:0> require 'active_support/all'
=> true
irb(main):004:0> "".present?
=> false

@djberg96
Copy link

djberg96 commented May 13, 2019

Yep, I think it is, so you'll have to check using String#empty? and invert the logic.

@agrare
Copy link
Member Author

agrare commented May 13, 2019

Hm specs pass:

agrare@agrare-thinkpad:~/src/manageiq/awesome_spawn$ rake
   102:     if command_result.failure?
   103:       message = CommandResultError.default_message(command, command_result.exit_status)
   104:       byebug
=> 105:       error_message = command_result.error.present? ? command_result.error : command_result.output
   106: 
(byebug) command_result.error.present?
false
(byebug) command_result.error.presence
*** NoMethodError Exception: undefined method `presence' for "":String
Did you mean?  present?

If a process doesn't correctly log to STDERR then the default .run! log
message isn't very helpful.  This logs command_result.error if it is
present, otherwise it logs command_result.output.
@agrare agrare force-pushed the log_output_if_command_result_error_is_nil branch from 3e19868 to a17cba3 Compare May 13, 2019 18:47
@agrare
Copy link
Member Author

agrare commented May 13, 2019

Okay checking for .nil? || .empty? just to be safe

@miq-bot
Copy link
Member

miq-bot commented May 13, 2019

Checked commit agrare@a17cba3 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 1 offense detected

lib/awesome_spawn.rb

  • ❗ - Line 103, Col 15 - Rails/Blank - Use command_result.error.blank? instead of command_result.error.nil? || command_result.error.empty?.

@djberg96
Copy link

My guess is that it's being pulled in by the tins gem, which is a dependency for coveralls:

https://github.com/flori/tins/blob/35c3067b998f31be88f0e42e4a4dc9523a2a692e/lib/tins/xt/blank.rb#L8

@carbonin carbonin self-assigned this May 13, 2019
@carbonin carbonin merged commit fecd068 into ManageIQ:master May 13, 2019
@agrare agrare deleted the log_output_if_command_result_error_is_nil branch May 14, 2019 12:22
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.

5 participants