jmadureira / netdata-cookbook

Chef cookbook to install FireHol's NetData
Apache License 2.0
14 stars 12 forks source link

add config, install resources #14

Closed nictrix closed 7 years ago

nictrix commented 7 years ago

fixes #7

From our previous discussions; I've made quite a few updates to the cookbook. I saw duplication occurring with the other python plugin resources, so I've collapsed them into one and deprecated the others. I also tried to update the supporting files to more closely match other community cookbooks; though I did keep the Gemfile, Rakefile (we could mimic this style of cookbook testing: https://github.com/chef-cookbooks/community_cookbook_documentation/blob/master/TESTING.MD) if you want.

I didn't create resources for stream, health, app_groups, node, statsd, etc.. as the current proposed changes are already large enough and I wanted to get acceptance of netdata_python_plugin. (future PRs can handle resources that manage those configs)

When you read through some of the deprecations, here are my thoughts around them in future updates:

Thanks for considering my PR and I look forward to your feedback!

nictrix commented 7 years ago

Quick note, I just realized the netdata_install resource is not restarting the service, I'm looking for examples and a solution to this problem, will comment once I have that fixed in this PR.

nictrix commented 7 years ago

Alright, I resolved the issue with the service not restarting after config changes for netdata_config. I also ran bundle exec rake test:integration which ran through all operating systems successfully.

nictrix commented 7 years ago

@sergiopena @jmadureira I know this is a huge PR - I didn't expect it to turn out this way, but one optimization lead to another and this is where it took me. I can break the changes up into multiple subsequent PRs if that helps decrease the number of things to look through.

Just let me know, looking for feedback and open to suggestions/changes as required.

jmadureira commented 7 years ago

Hello @nictrix

Sorry for my absence as I didn't have enough time to give a proper review to your changes. Nevertheless thank you for your work. I had already made some of these changes myself so you saved me some time :)

My only remark is with the deprecation of the default netdata recipe. I agree that the installation process can be provided via chef resource but having a recipe that is simply a wrapper for that resource gives users a cleaner mechanism of installing netdata. I have several cases myself where I simply include the recipe on my run list because the default installation is more than enough.

What do you think?

nictrix commented 7 years ago

@sergiopena thanks for the approval

@jmadureira I think that is more than reasonable and I can update the cookbook to keep that functionality.

nictrix commented 7 years ago

Updated with the changes @jmadureira wanted. Ran through style, unit and integration tests just to make sure everything is still looking good!

This PR is ready for approval/merge.