Skip to content

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

Open
lahdekorpi wants to merge 2 commits into
sensu-plugins:masterfrom
lahdekorpi:patch-1
Open

Add option to output cmd stdout with check-cmd#60
lahdekorpi wants to merge 2 commits into
sensu-plugins:masterfrom
lahdekorpi:patch-1

Conversation

@lahdekorpi

Copy link
Copy Markdown

Purpose

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

@majormoses

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

@majormoses Did you have time to look at this?

@majormoses

Copy link
Copy Markdown
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?

Comment thread 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
Copy Markdown
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

majormoses commented Feb 18, 2018

Copy link
Copy Markdown
Member

@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.

2 participants