-
Notifications
You must be signed in to change notification settings - Fork 262
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
Add support for any config options #375
Conversation
ed56583
to
cec18ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had one "important" comment, everything else looks 👌
@@ -221,6 +221,11 @@ | |||
# extra_packages to install | |||
default['datadog']['extra_packages'] = {} | |||
|
|||
# extra config options | |||
# If an agent is released with a new config option which is not yet supported by this cookbook | |||
# you can use this attribute to set it. Will be ignored if falsy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should ignore the value only if it's nil
. We have agent configuration options that are true
by default so setting them to false
is meaningful. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also, if you agree, a test on that would be perfect :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I didn't think of false
, only of ''
, which is useless since ini requires key: value.
@@ -221,6 +221,11 @@ | |||
# extra_packages to install | |||
default['datadog']['extra_packages'] = {} | |||
|
|||
# extra config options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could you put this right under default['datadog']['histogram_percentiles']
in this file, so that it's visually regrouped with the other agent config attribute defaults?
cec18ac
to
42c058f
Compare
It is annoying to have to wait for a new cookbook release when an agent is released with a new config option. This commit aims to fix it by allowing any config options to be added.
42c058f
to
f712ee6
Compare
Updated @olivielpeau, ready for round 2! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
🙇
It is annoying to have to wait for a new cookbook release when an agent
is released with a new config option.
This commit aims to fix it by allowing any config options to be added.
Fix #336