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

Add option to output cmd stdout with check-cmd #60

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lahdekorpi
Copy link

Purpose

Adds a flag to allow outputting the stdout of the command. Helps with getting status information about a command.

@majormoses
Copy link
Member

Thanks for your contribution to Sensu plugins! Without people like you submitting PRs we couldn't run the project. I will review it shortly.

@majormoses
Copy link
Member

Hmm it looks like rubocop is pointing out that there is quite of bit of nesting conditionals which often leads to buggy code and poor performance. I see a couple ways we can improve this:

  • I know you did not write this but we could rewrite to avoid using the ok, unknown, critical, and warning methods in any function other than run. By returning the status instead we have more options to better error handle and write cleaner code.
  • the other option is to always set the message to include the stdout regardless if they want to check the stdout or not. Reading the description I feel that should be the case anyways or the description should be modified to further clarify that this option does not apply when check output is not specified.

@lahdekorpi
Copy link
Author

I'd love to rewrite this, but I'm not really a Ruby programmer so I'm not sure if I'd be qualified.

@majormoses
Copy link
Member

I'd love to rewrite this, but I'm not really a Ruby programmer so I'm not sure if I'd be qualified.

No worries, I won't try to pressure you into doing something you are uncomfortable with.

I consider myself a sysadmin who can write ruby more than a ruby programmer but if I have some time this weekend I will try to work on a re-write ping me next week if I have not replied here.

@lahdekorpi
Copy link
Author

@majormoses Did you have time to look at this?

@majormoses
Copy link
Member

@lahdekorpi sorry I did not have the time and thanks for pinging me. Maybe the best thing would be to put inline rubocop disables and a TODO: comment to come fix it up later. @mattyjones do you agree or should we gate this until I have time to look at refactoring it?

bin/check-cmd.rb Outdated
ok "#{config[:command]} exited with #{$CHILD_STATUS.exitstatus}"
status = "#{config[:command]} exited with #{$CHILD_STATUS.exitstatus}"
status += "\nOutput: #{stdout}" if config[:echo_stdout]
status ok
Copy link
Member

Choose a reason for hiding this comment

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

btw this looks wrong to me I am guess this is meant to be ok status.

@majormoses
Copy link
Member

majormoses commented Feb 18, 2018

@lahdekorpi I finally had the time to sit down with this for a bit (honestly I'd love to find the time to rewrite it entirely) of refactoring. As there are a lack of useful automated tests if you can help with running some tests and sharing the results here it that would be appreciated.

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.

None yet

2 participants