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

Fixed #429 - Add Recipe for disk integration #430

Merged
merged 3 commits into from May 3, 2017

Conversation

Projects
None yet
2 participants
@iancward
Copy link
Contributor

commented Apr 28, 2017

No description provided.

@olivielpeau
Copy link
Member

left a comment

Thanks @iancward! This recipe was indeed missing from the cookbook.

Just added a comment on a minor thing.

excluded_disk_re: '/dev/sde.*',
tag_by_filesystem: 'no',
excluded_mountpoint_re: '/mnt/somebody-elses-problem.*',
all_partitions: 'no'

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau May 2, 2017

Member

Even though the disk check appears to correctly handle strings for boolean parameters, it's recommended to set them as actual booleans. Could you use booleans for use_mount, tag_by_filesystem and all_partitions? (and update the example you provided in the recipe accordingly).

This comment has been minimized.

Copy link
@iancward

iancward May 2, 2017

Author Contributor

Hi @olivielpeau: I actually took the examples directly from http://docs.datadoghq.com/integrations/disk/, but I've made the requested changes in bdec562.

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau May 2, 2017

Member

Thanks!

Actually the examples in the docs are correct, in yaml yes and no are booleans (but 'yes' and 'no' are strings).

This comment has been minimized.

Copy link
@iancward

iancward May 2, 2017

Author Contributor

Ah, right. I think I had quoted those because the 'yes' and 'no' string attributes were getting converted to true/false booleans in the file and the spec was failing. Using true/false directly definitely removes confusion here.

@olivielpeau olivielpeau added this to the 2.10.0 milestone May 2, 2017

@olivielpeau

This comment has been minimized.

Copy link
Member

commented May 2, 2017

@iancward One last thing: could you fix the small rubocop failure? https://travis-ci.org/DataDog/chef-datadog/builds/228122567#L266

@iancward

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2017

Oopsie. Fixed in f3b925d .
I had originally tried using a do/end block instead of the { and } (which is supposed to be used for single line things), but it couldn't get it to work properly. I had the same experience with the chefspec in the for the nginx integration.

@olivielpeau olivielpeau added the feature label May 3, 2017

@olivielpeau

This comment has been minimized.

Copy link
Member

commented May 3, 2017

Thanks @iancward! PR looks good, merging.

The issue you had with do/end is probably related to the lower precedence of do/end compared to {/}... I'd say it's fine to use curly braces here anyway.

@olivielpeau olivielpeau merged commit d00a136 into DataDog:master May 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.