split out the snmp data and hosts into defines

this will make it a lot easier to have a "centralized" collectd snmp
collector that will gather information from network devices and such
which are puppetized but don't run their own collectd :)

manifests/plugin/snmp.pp

@@ -5,8 +5,6 @@
   $hosts    = undef,
   $interval = undef,
 ) {
-  validate_hash($data, $hosts)
-
   if $::osfamily == 'Redhat' {
     package { 'collectd-snmp':
       ensure => $ensure,

manifests/plugin/snmp/data.pp

@@ -0,0 +1,26 @@
+# https://collectd.org/wiki/index.php/Plugin:SNMP
+define collectd::plugin::snmp::data (
+  $ensure = present,
+  $type,
+  $table = false,
+  $instance,
+  $values,
+) {
+  include collectd
+  include collectd::plugin::snmp
+
+  $table_bool = str2bool($table)
+
+  $conf_dir = $collectd::params::plugin_conf_dir
+  $root_group = $collectd::params::root_group
+
+  file { "snmp-data-${name}.conf":
+    ensure  => $ensure,
+    path    => "${conf_dir}/15-snmp-data-${name}.conf",
+    owner   => 'root',
+    group   => $root_group,
+    mode    => '0640',
+    content => template('collectd/plugin/snmp/data.conf.erb'),
+    notify  => Service['collectd'];
+  }
+}

manifests/plugin/snmp/host.pp

@@ -0,0 +1,27 @@
+# https://collectd.org/wiki/index.php/Plugin:SNMP
+define collectd::plugin::snmp::host (
+  $ensure = present,
+  $address = $name,
+  $version = 1,
+  $community = 'public',
+  $collect,
+  $interval = undef,
+) {
+  include collectd
+  include collectd::plugin::snmp
+
+  validate_re($version, '^[12]$', 'only snmp versions 1 and 2 are supported')
+
+  $conf_dir = $collectd::params::plugin_conf_dir
+  $root_group = $collectd::params::root_group
+
+  file { "snmp-host-${name}.conf":
+    ensure  => $ensure,
+    path    => "${conf_dir}/25-snmp-host-${name}.conf",
+    owner   => 'root',
+    group   => $root_group,
+    mode    => '0640',
+    content => template('collectd/plugin/snmp/host.conf.erb'),
+    notify  => Service['collectd'];
+  }
+}

spec/classes/collectd_plugin_snmp_spec.rb

@@ -67,13 +67,5 @@
     end
   end
 
-  context ':data is not a hash' do
-    let :params do
-      {:data => []}
-    end
-    it 'Will raise an error about :data being a Array' do
-      expect {should}.to raise_error(Puppet::Error,/Array/)
-    end
-  end
 end
 

spec/defines/collectd_plugin_snmp_data_spec.rb

@@ -0,0 +1,66 @@
+require 'spec_helper'
+
+describe 'collectd::plugin::snmp::data', :type => :define do
+  let :facts do
+    {:osfamily => 'Debian'}
+  end
+
+  let (:title) { 'foo' }
+  let (:required_params) {{
+      :type     => 'bar',
+      :instance => 'baz',
+      :values   => 'bat',
+  }}
+
+  let (:filename) { 'snmp-data-foo.conf' }
+
+  context 'required params' do
+    let (:params) { required_params }
+
+    it { should contain_file(filename).with(
+      :ensure => 'present',
+      :path   => '/etc/collectd/conf.d/15-snmp-data-foo.conf'
+    ) }
+
+    it { should contain_file('snmp-data-foo.conf').that_notifies('Service[collectd]') }
+    it { should contain_file('snmp-data-foo.conf').with_content(/<Plugin snmp>/) }
+    it { should contain_file('snmp-data-foo.conf').with_content(/<Data "foo">/) }
+    it { should contain_file('snmp-data-foo.conf').with_content(/Type "bar"/) }
+    it { should contain_file('snmp-data-foo.conf').with_content(/Instance "baz"/) }
+  end
+
+  context 'values is an array' do
+    let (:params) {
+      required_params.merge({
+        :values => %w{ foo bar baz }
+      })
+    }
+    it { should contain_file('snmp-data-foo.conf').with_content(/Values foo bar baz/) }
+  end
+
+  context 'values is just a string' do
+    let (:params) {
+      required_params.merge({
+        :values => 'bat'
+      })
+    }
+    it { should contain_file('snmp-data-foo.conf').with_content(/Values bat/) }
+  end
+
+  context 'table is true' do
+    let (:params) {{
+      :table => true
+    }.merge(required_params)}
+
+    it { should contain_file('snmp-data-foo.conf').with_content(/Table true/) }
+  end
+
+  context 'table is false' do
+    let (:params) {{
+      :table => false
+    }.merge(required_params)}
+
+    it { should contain_file('snmp-data-foo.conf').with_content(/Table false/) }
+  end
+
+end

spec/defines/collectd_plugin_snmp_host_spec.rb

@@ -0,0 +1,60 @@
+require 'spec_helper'
+
+describe 'collectd::plugin::snmp::host', :type => :define do
+  let :facts do
+    {:osfamily => 'Debian'}
+  end
+
+  let (:title) { 'foo.example.com' }
+  let (:required_params) {{
+    :collect => 'foo'
+  }}
+
+  let (:filename) { 'snmp-host-foo.example.com.conf' }
+
+  context 'default params' do
+    let (:params) { required_params }
+
+    it { should contain_file(filename).with(
+      :ensure => 'present',
+      :path   => '/etc/collectd/conf.d/25-snmp-host-foo.example.com.conf'
+    ) }
+
+    it { should contain_file(filename).that_notifies('Service[collectd]') }
+    it { should contain_file(filename).with_content(/<Plugin snmp>/) }
+    it { should contain_file(filename).with_content(/<Host "foo\.example\.com">/) }
+    it { should contain_file(filename).with_content(/Address "foo\.example\.com"/) }
+    it { should contain_file(filename).with_content(/Version 1/) }
+    it { should contain_file(filename).with_content(/Community "public"/) }
+    it { should contain_file(filename).without_content(/Interval \d+/) }
+  end
+
+  context 'all params set' do
+    let (:params) {
+      required_params.merge({
+        :address   => 'bar.example.com',
+        :version   => '2',
+        :community => 'opensesame',
+        :interval  => '30',
+      })
+    }
+    it { should contain_file(filename).with_content(/Address "bar\.example\.com"/) }
+    it { should contain_file(filename).with_content(/Version 2/) }
+    it { should contain_file(filename).with_content(/Community "opensesame"/) }
+    it { should contain_file(filename).with_content(/Interval 30/) }
+  end
+
+  context 'collect is an array' do
+    let (:params) {{
+      :collect => %w{ foo bar baz }
+    }}
+    it { should contain_file(filename).with_content(/Collect foo bar baz/) }
+  end
+
+  context 'collect is just a string' do
+    let (:params) {{
+      :collect => 'bat'
+    }}
+    it { should contain_file(filename).with_content(/Collect bat/) }
+  end
+end

templates/plugin/snmp/data.conf.erb

@@ -0,0 +1,8 @@
+<Plugin snmp>
+  <Data "<%= @name %>">
+    Type "<%= @type %>"
+    Table <%= @table_bool ? 'true' : 'false' %>
+    Instance "<%= @instance %>"
+    Values <%= Array(@values).join(' ') %>
+  </Data>
+</Plugin>

templates/plugin/snmp/host.conf.erb

@@ -0,0 +1,11 @@
+<Plugin snmp>
+  <Host "<%= @name %>">
+    Address "<%= @address %>"
+    Version <%= @version %>
+    Community "<%= @community %>"
+    Collect <%= Array(@collect).join(" ") %>
+    <%- if !@interval.nil? -%>
+    Interval <%= @interval %>
+    <%- end -%>
+  </Host>
+</Plugin>