hectcastro / chef-collectd

A Chef cookbook to install collectd.
Apache License 2.0
23 stars 65 forks source link

Better robustness when installing collected from source. #19

Closed hochmann closed 10 years ago

hochmann commented 10 years ago

The folloieng changes allow you to download any version you want from collectd site using wget instead of remote_file.

fixes the following issues:

hectcastro commented 10 years ago

Hey @ahochman,

This is the first time I've seen direct use of wget in place of the remote_file resource. I don't think I'm in agreement of using it to replace remote_file.

Also, the reason wget works and remote_file doesn't against collectd.org (I think) is because they are blocking the Chef user agent (passed by default by remote_file). My understanding is that they are blocking Chef user agents to prevent bandwidth costs from going through the roof. Because of that, I think the better approach is to use the existing remote_file resource, but override the node["collectd"]["url"] attribute with your own collectd.tar.gz.

Another alternative would be to open a PR that allows the headers attribute of the remote_file resource to be overridden by another node["collectd"] level attribute: http://docs.opscode.com/resource_remote_file.html

hochmann commented 10 years ago

Hi @hectcastro , First of all thank you for your quick input.

I agree with your perspective about remote_file, it is better than Hard coded wget. Regarding Configuration Folders, don't you think it's still something that should be added?

directory "#{node["collectd"]["dir"]}" do
  action :create
end

directory "#{node["collectd"]["dir"]}/etc" do
  action :create
end

directory "#{node["collectd"]["dir"]}/etc/conf.d" do
  action :create
end

another thing, I thought about start hosting collectd-5.4.0.tar.gz in S3 as a public resource, i think it can be a nice attributes update.

lmk what u think.

hectcastro commented 10 years ago

Did you change the default node["collectd"]["dir"] attribute to trigger the missing configuration file directory error? If so, what did you use so that I can attempt to reproduce it on my end?

If that ends up being the issue, perhaps you can condense the fix to something like:

directory "#{node["collectd"]["dir"]}/etc/conf.d" do
  action :create
  recursive true
end

And ensure that the directory block gets moved up before the template resource. recursive should create etc and conf.d for you in one step.

With regard to using your hosted tar.gz of collectd, that's a very generous offer. Perhaps use it as the default just for the test suite so that your S3 bills don't get too high?

hochmann commented 10 years ago

Ok so i double checked it, there is no issue regarding the directories (i used the default node["collectd"]["dir"]), probably due to a failure with the "tar -zxf" i got into a case where it appeared.

Any way regarding my S3 bucket providing collectd.tar.gz, the all size of the file is just 1.9MB. i discussed with my colleagues and we thought it can be a great alternative for the collectd.org remote_file issue.

So what u think? would u like to merge a PR with this attributes update?

override["collectd"]["version"] = "5.4.0"
override["collectd"]["url"] = "https://s3.amazonaws.com/collectd-5.4.0/collectd-#{node["collectd"]["version"]}.tar.gz"
hectcastro commented 10 years ago

Personally, I like the idea of using it as the default for .kitchen.yml (the configuration file for this cookbook's test suite):

https://github.com/hectcastro/chef-collectd/blob/develop/.kitchen.yml#L26-L28

I also think it would be useful to include a section in the README under Usage that provides a list of alternative endpoints for downloading collectd.

Could you add yours there?

hochmann commented 10 years ago

Yeah sure, i'll take care of it