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

Remove sensu_check_config #764

Closed
jeffmccune opened this issue Jul 21, 2017 · 6 comments
Closed

Remove sensu_check_config #764

jeffmccune opened this issue Jul 21, 2017 · 6 comments
Assignees
Labels

Comments

@jeffmccune
Copy link
Collaborator

Garrett and I discussed this today. There is confusion surrounding sensu_check_config, the JSON provider doesn't seem to be well-behaved, the type isn't used anywhere in the module, there isn't any documentation, and the sensu::check type appears to provide all the necessary functionality for checks.

One possible situation is sensu_check_config is able to set keys and values at the same level as "checks" in the target file, whereas sensu::check can only set custom configuration nested under checks['mycheck'].

If it is necessary to add configuration in the top-level map of the JSON files in conf.d/checks/, then sensu::check should take a config property which is merged together with the checks top level key.

@jeffmccune
Copy link
Collaborator Author

Related to #759 but is a separate task unrelated to the reference for provider spec testing.

jeffmccune added a commit to jeffmccune/sensu-puppet that referenced this issue Jul 21, 2017
Without this patch there are no rspec examples of how providers should behave.
This is a problem because there should be a reference to build from when
changing the providers.

This patch addresses the problem by adding rspec coverage for the
sensu_check_config json provider.

Note: Garrett and I decided to remove the sensu_check_config type and provider.
It doesn't operate as expected in the master branch, is not used anywhere in the
module, and the custom configuration map in sensu_check provides the same
behavior for checks.  Each specific type will implement the generic
configuration map similar to sensu_check.

The reference implementation of the spec tests will move to sensu_check in a
subsequent patch.  sensu_check_config will be removed in a subsequent patch.

Relates to sensu#764
@cwjohnston
Copy link
Contributor

One possible situation is sensu_check_config is able to set keys and values at the same level as "checks" in the target file, whereas sensu::check can only set custom configuration nested under checks['mycheck'].

Sensu is agnostic of custom configuration scopes, so plugins and extensions of all sorts can load Sensu's config on disk and read their settings from arbitrary keys. For example, handler-mailer, handler-mailer-ses and handler-mailer-mailgun plugins in the sensu-plugins-mailer project look for their configuration under mailer, mailer-ses and mailer-mailgun top-level keys, respectively.

If it is necessary to add configuration in the top-level map of the JSON files in conf.d/checks/, then sensu::check should take a config property which is merged together with the checks top level key.

I think it's worth noting that this configuration convention isn't specific to check plugins. It can apply to handler plugins, mutator plugins and extensions of all sorts, be they check, filter, mutator or handler. With this in mind, I think instructing users of this module to provide custom configuration using sensu::write_json directly instead of adding a config property on sensu::check might make more sense.

@jeffmccune jeffmccune self-assigned this Jul 26, 2017
@jeffmccune
Copy link
Collaborator Author

Working on this now, I'm going to proceed with the approach of adding a config property to the sensu_check type and provider, primarily because this retains the modeling of Sensu configuration as Puppet resources, e.g. sensu::check instead of sensu::write_json. The implementation and spec test coverage for the provider will serve as an example for the other native types, which will reduce the time required to implement a config property for all types.

jeffmccune added a commit to jeffmccune/sensu-puppet that referenced this issue Jul 27, 2017
Without this patch there is no way to specify arbitrary configuration at the top
level of the sensu check configuration object.  This patch addresses the problem
by adding a new properto to sensu::check and sensu_check named `config`.  This
property accepts a Hash map and merges all other configuration on top of this
base starting point.

This patch also adds a new mix-in method, `PuppetX::Sensu::SortHash#sort_hash`.
This method returns a hash sorted recursively, meaning all nested hash values
are sorted in the same manner.  This method is intended for use in the #flush
method of all Sensu providers.

See sensu#764
jeffmccune added a commit to jeffmccune/sensu-puppet that referenced this issue Jul 27, 2017
Without this patch there is no way to specify arbitrary configuration at the top
level of the sensu check configuration object.  This patch addresses the problem
by adding a new properto to sensu::check and sensu_check named `config`.  This
property accepts a Hash map and merges all other configuration on top of this
base starting point.

This patch also adds a new mix-in method, `PuppetX::Sensu::SortHash#sort_hash`.
This method returns a hash sorted recursively, meaning all nested hash values
are sorted in the same manner.  This method is intended for use in the #flush
method of all Sensu providers.

See sensu#764
jeffmccune added a commit to jeffmccune/sensu-puppet that referenced this issue Jul 27, 2017
Without this patch there is no way to specify arbitrary configuration at the top
level of the sensu check configuration object.  This patch addresses the problem
by adding a new properto to sensu::check and sensu_check named `config`.  This
property accepts a Hash map and merges all other configuration on top of this
base starting point.

This patch also adds a new mix-in method, `PuppetX::Sensu::SortHash#sort_hash`.
This method returns a hash sorted recursively, meaning all nested hash values
are sorted in the same manner.  This method is intended for use in the #flush
method of all Sensu providers.

See sensu#764
jeffmccune added a commit to jeffmccune/sensu-puppet that referenced this issue Jul 27, 2017
Without this patch there is no way to specify arbitrary configuration at the top
level of the sensu check configuration object.  This patch addresses the problem
by adding a new properto to sensu::check and sensu_check named `config`.  This
property accepts a Hash map and merges all other configuration on top of this
base starting point.

This patch also adds a new mix-in method, `PuppetX::Sensu::SortHash#sort_hash`.
This method returns a hash sorted recursively, meaning all nested hash values
are sorted in the same manner.  This method is intended for use in the #flush
method of all Sensu providers.

See sensu#764
jeffmccune added a commit to jeffmccune/sensu-puppet that referenced this issue Jul 27, 2017
Without this patch there is no way to specify arbitrary configuration at the top
level of the sensu check configuration object.  This patch addresses the problem
by adding a new properto to sensu::check and sensu_check named `config`.  This
property accepts a Hash map and merges all other configuration on top of this
base starting point.

This patch also adds a new mix-in method, `PuppetX::Sensu::SortHash#sort_hash`.
This method returns a hash sorted recursively, meaning all nested hash values
are sorted in the same manner.  This method is intended for use in the #flush
method of all Sensu providers.

See sensu#764
@jeffmccune
Copy link
Collaborator Author

@cwjohnston After spending a little time looking into a config property, I think you're suggestion of using sensu_write_json() is way better. There are a bunch of edge cases around the merging of an arbitrary config map with all the other properties, especially the custom property which itself is an arbitrary map. Adding config significantly increases complexity of the types, whereas refactoring the defined types to use sensu_write_json() should reduce the complexity located in the native types. The puppet code will be a bit more complex, but this should be easier to maintain compared to native types over the long run.

I'm going to close the WIP related to the config property and switch over to refactoring the defined types to use sensu_write_json().

@jeffmccune
Copy link
Collaborator Author

Next steps on this are to get #785 merged, let it bake in the community, then remove sensu_check_config, replaced by the content parameter of sensu::check.

@ghoneycutt
Copy link
Collaborator

In order to preserve semver, this will wait until the module is redone for Sensu 2.

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

No branches or pull requests

3 participants