Add support for Chef Solo/RightScale, fill out metadata, more directive support in template #4

Open
wants to merge 15 commits into
from

5 participants

@flaccid

The client recipe has been improved to support chef-solo where collectd servers can be specified via a node attribute.

Metadata has been completed (recipes and attribute definitions) as well as adding metadata.json back as this is required for usage on RightScale.

The Hostname and FQDNLookup directives have been added to the template for user specifiable configuration to be more flexible on arbitrary node requirements.

@zach-rightscale

Nice work Chris!

@coderanger coderanger commented on an outdated diff Sep 6, 2012
attributes/default.rb
@@ -22,6 +22,9 @@
default[:collectd][:types_db] = ["/usr/share/collectd/types.db"]
default[:collectd][:interval] = 10
default[:collectd][:read_threads] = 5
+default[:collectd][:fqdn_lookup] = "true"
@coderanger
Owner
coderanger added a line comment Sep 6, 2012

Is there a particular reason to use a string instead of a real Ruby boolean? Usually nice to use native types when possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@coderanger coderanger commented on an outdated diff Sep 6, 2012
recipes/client.rb
@@ -19,9 +19,13 @@
include_recipe "collectd"
-servers = []
-search(:node, 'recipes:"collectd::server"') do |n|
- servers << n['fqdn']
+if node['collectd']['servers'] or Chef::Config[:solo]
@coderanger
Owner
coderanger added a line comment Sep 6, 2012

Rather than just use an empty list in Solo, it would probably be better to raise an error. As I write this, it can probably be more generalized to either solo or client that finding 0 servers should be an error. What do you think of this logic?

servers = if !node[:collectd][:servers].empty?
  node[:collectd][:servers]
elsif !Chef::Config[:solo]
  search(:node, 'recipes:"collectd::server"').map {|n| n['fqdn'] }
else
 []
end
raise "something" if servers.empty?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@simonoff

Any update?

@coderanger
Owner

This variable could probably be retitled to be a little clearer, also by default the package resource uses the install action, which isn't globally idempotent since multiple servers can end up with different versions over time depending on what was "latest" when it ran (unless you pin packages some other way).

@coderanger

Why did you remove this?

@kpumuk kpumuk and 1 other commented on an outdated diff Jun 29, 2013
@@ -5,3 +5,86 @@
long_description IO.read(File.join(File.dirname(__FILE__), 'README.md'))
version "1.0.0"
supports "ubuntu"
+
+depends "apache2"
@kpumuk
kpumuk added a line comment Jun 29, 2013

It only depends on apache2 if collectd_web is used. We don't use it for example

@flaccid
flaccid added a line comment Oct 21, 2013

@kpumuk Chef doesn't support optional dependencies (in metadata) so technically its a dependency. What the best practice is, I'm not sure but personally I think its better to do this as recipes don't have a mechanism to even check nicely on the cookbook needed (which would also create a lot of extra redundant code in every cookbook that has optional deps).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@flaccid

@coderanger I have finally updated this pr! Please let me know if it needs more work before it can be merged. Thanks for your year-long patience :)

@coderanger
Owner

Just as a heads up I'm about to get really busy for a week or two, and this no longer cleanly merges, so it may take a while to get it sorted.

@flaccid

No problem coderanger. I'll go on standby and look at for any action items I should do. Worse case, I don't mind re-doing the whole thing with a fresh fork, but it would be cool to avoid that if possible.

@jhmartin

Perhaps you could rebase on top of master so this becomes a clean merge?

@flaccid

@coderanger hmm what should we do now? Please let me know :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment