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

Added direct support for timeout,aggregate,handle,publish,occurrences and refresh parameters to sensu_check #141

Closed
wants to merge 6 commits into from

Conversation

zdenekjanda
Copy link
Contributor

As suggested on IRC. timeout/aggregate/handle/publish are used by sensu server, and occurrences/refresh by sensu handler, so its logical to have them usable directly. you can still feed them with custom param too, so should be backwards compatible.

@jamtur01
Copy link
Contributor

Be good to update any examples in the README

@jlambert121 Your thoughts?

@jlambert121
Copy link
Contributor

I think these were once first class parameters and they were pulled out and moved to the custom parameter because they weren't native sensu parameters but stuff added by the client libraries. Is my memory faulty on that one?

@jamtur01
Copy link
Contributor

It rings a bell. There's a PR for it somewhere I think. Or discussion in the custom params PR perhaps?

@jlambert121
Copy link
Contributor

I can do some more digging this afternoon MT to see what's in the sensu core unless there is a complete list somewhere we can just implement. I think doing everything in core as a parameter would be reasonable.

@zdenekjanda
Copy link
Contributor Author

so, to put more light in, timeout/aggregate/handle/publish are used in server core. occurrences/refresh is used by sensu-handler, which is direcly invoked by core. sensu-chef indeed does have timeout/aggregate/handle/publish directly within sensu_check, but it doesnt have occurrences/refresh. i am still convinced that we can add occurrences/refresh directly into sensu_check, because they will be always sent by server to sensu-handler and are used very often. Rest of parameters, for customization, makes sense to pass as hash in sensu_check.custom.

@zdenekjanda
Copy link
Contributor Author

There is a lot discussion on it, for ex. here: #75, here: #97, latest check.rb from sensu-chef by @portertech: https://github.com/sensu/sensu-chef/blob/master/resources/check.rb
I guess we dont have to discuss about timeout/aggregate/handle/publish, they should be definivelly here, but occurrences/refresh is the challenge. These two have been repetitively added and again removed to this module, i still do vote to have them in to enable typing of data and due to fact that sensu-handler is defacto part of core, but perhaps @portertech can enlight us :)

@jlambert121
Copy link
Contributor

I agree on timeout/aggregate/handle/publish - they are in sensu "core" and should be first class citizens here. I also like the parity with the chef cookbook.

I know I have a bunch of 'custom => { occurrences => 2, refresh => 60 }' in my manifests. The last time refresh was mentioned (#97) portertech chimed in and voted against. I see refresh and occurrences used by the ruby sensu-plugin but not the python sensu-plugin-python so those aren't even ubiquitous at this point with the official plugin libraries.

I'm in favor of drawing the line of native attributes to those that are implemented by sensu core. It keeps it consistent and in parity with the chef cookbook since it looks like that is the implementation there.

@jamtur01
Copy link
Contributor

+1

1 similar comment
@portertech
Copy link
Contributor

+1

@jlambert121
Copy link
Contributor

@zdenekjanda could you squash these 5 commits into one and I'll merge this? We're about to release 1.0.0 and this is a great addition - thanks!

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

Successfully merging this pull request may close these issues.

4 participants