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

iancward
Copy link
Contributor

No description provided.

Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

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'
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

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

@iancward
Copy link
Contributor Author

iancward 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
Copy link
Member

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants