Closed rclagett closed 5 years ago
This change should have been part of https://github.com/Netuitive/chef-netuitive/pull/61 but was forgotten about.
The change looks good to me, but is metrics_whitelist =
(i.e. blank) going to do the right thing in the template? You might need <% unless @docker_collector_metrics_whitelist.to_s == "" %>
or something along those lines leaving the line out if it's blank/nil
I see what you mean.
What about <% unless @docker_collector_metrics_whitelist.empty? %>
?
It would follow the convention used for tags: https://github.com/Netuitive/chef-netuitive/blob/master/templates/default/netuitive-agent.conf.erb#L64
You're right. I always forget that .empty?
isn't an ActiveSupport feature but built into the language. That's the proper fix :)
Updated!
@majormoses Would you mind providing a review as well when you get a chance? Sorry we weren't able to get it right the first time 😞
I think you're right about changing the default. That said, empty and nil are both illegal for the config file I think, so maybe the empty check is still appropriate? Is there a use case for setting the white list regex to an empty string?
An empty whitelist is perfectly acceptable in terms of the config file allowing it and the resulting behavior. It's the same as having no filtering done at all.
We could set the default to .*
which is effectively the same thing as an empty string.
It would also mean checking for nil or empty wouldn't be necessary...
I'll push the commits for you to take a look. I can revert if need be.
Pull Request Checklist
Is this in reference to an existing issue?
General
[x] Update Changelog following the conventions laid out on Keep A Changelog
[ ] Update README with any necessary configuration snippets
[x] RuboCop passes
[ ] Existing tests pass
New Features
[ ] Tests
[ ] Add any required documentation to to the README
Purpose
Define docker_collector_metrics_whitelist
Known Compatibility Issues