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

Force array for some sense::check params #801 #804

Merged
merged 1 commit into from
Aug 28, 2017

Conversation

alvagante
Copy link
Collaborator

@alvagante alvagante commented Aug 26, 2017

Pull Request Checklist

Changed sensu::check define and relevant spec tests.

Description

This commit converts strings to arrays with a single element, when a String is used for the handlers, subscribers and aggregates parameters in sensu::check

This PR replaces #803

Related Issue

Fixes #801

Motivation and Context

Syntax errors on check files when using strings on the given params.

How Has This Been Tested?

Tested on sensu-server vagrant vm and spec tests.

General

  • Update README.md with any necessary configuration snippets

  • New parameters are documented

  • New parameters have tests

  • Tests pass - bundle exec rake validate lint spec

@alvagante alvagante changed the title Force array for some sense::check params #801 WIP - NEEDS DECISION - Force array for some sense::check params #801 Aug 26, 2017
@alvagante
Copy link
Collaborator Author

@ghoneycutt this is finally a clean PR on #801 .
But I think we may reconsider the opportunity to throw a deprecation notice.
I see in the README (which would need update if we keep the current code) examples like this:

  sensu::check { 'check_ntp':
    command     => 'PATH=$PATH:/usr/lib/nagios/plugins check_ntp_time -H pool.ntp.org -w 30 -c 60',
    handlers    => 'default',
    subscribers => 'sensu-test'
  }

which means that a lot of existing code has strings where we expect an array.
I would just remove the deprecation notice, and convert string to array when needed.
That's probably the original intention.

@ghoneycutt
Copy link
Collaborator

@alvagante Good eye, I agree. Let's just silently convert to array for our implementation.

@alvagante alvagante changed the title WIP - NEEDS DECISION - Force array for some sense::check params #801 Force array for some sense::check params #801 Aug 27, 2017
@alvagante
Copy link
Collaborator Author

@ghoneycutt ready for merge

@ghoneycutt ghoneycutt merged commit 4396816 into sensu:master Aug 28, 2017
@ghoneycutt
Copy link
Collaborator

Thanks @alvagante

Released in v2.33.1

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.

3 participants