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

Helper method to convert boolean strings to integers #174

Merged
merged 4 commits into from
Jul 7, 2017

Conversation

Evesy
Copy link
Contributor

@Evesy Evesy commented Jul 2, 2017

Additional helper method cast_bool_values_int to convert a provided boolean string to respective integer value.

Description

This method accepts a single value, converting a true or false string to 1 or 0 respectively.
Any other values will just be returned.

Motivation and Context

In certain metric collection plugins, metrics are occasionally generated with true/false values rather than integers. Certain TSDB's will not accept boolean values and instead expect integer equivalents (i.e. Graphite, true = 1, false = 0). See sensu-plugins/sensu-plugins-elasticsearch#49 for reference

This helper method allows, where applicable, these values to be converted into integers so they can be successfully ingested by those TSDB's.

How Has This Been Tested?

Tested new method in local environment

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have updated the Changelog following the conventions laid out on Keep A Changelog

Known Caveats

@majormoses
Copy link
Member

@Evesy it looks good, can you add a test please?

Copy link
Member

@majormoses majormoses left a comment

Choose a reason for hiding this comment

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

overall looks good, some minor enhancements and test will make this mergeable from my perspective though I will check with @cwjohnston since this is closer to the core and I want to check and see if he has any concerns.


def cast_bool_values_int(value)
case value
when 'true'
Copy link
Member

@majormoses majormoses Jul 2, 2017

Choose a reason for hiding this comment

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

to make it a bit more generic we should have when 'true', true as I am sure different implementations will return it differently and we might as well support both.

case value
when 'true'
1
when 'false'
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above when 'false', false

@majormoses
Copy link
Member

@Evesy thanks for adding those tests which look good to me.

@majormoses majormoses requested a review from cwjohnston July 2, 2017 20:20
@majormoses
Copy link
Member

@cwjohnston his looks good to me, do you have any issues with pushing this logic here? I think it could be helpful to solve the same issue across multiple plugins.

@cwjohnston
Copy link
Contributor

Seems reasonable and tests look good. Thanks!

@majormoses
Copy link
Member

cool, I will cut a release shortly then.

@majormoses majormoses merged commit 99c290a into sensu-plugins:master Jul 7, 2017
@majormoses
Copy link
Member

Apparently the build process is different for this plugin I have opened #175 as I do not see why the process should be different even if the access is. We should figure this out before accepting any more changes.

@majormoses
Copy link
Member

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