Improve version handling to avoid multiple puppet runs for some situa…

…tions

When an administrator knows the version of collectd that will be used or
at least the minimum version available the need for two puppet runs before
convergence can be avoided or at least minimised.

Instead of using the fact in the templates they now use a class variable
set to one of (in priority order):
* collectd_version (i.e. the fact)
* version (the semver matched part of it only)
* minimum_version (undef by default)

Existing behaviour is preserved except for a corner case where version is
set to something specific and collectd is not yet installed. In this case
puppet will only take one run and assume the version specified when creating
the templates

references voxpupuli#162

README.md

@@ -27,9 +27,10 @@ declaration:
 
 ```puppet
 class { '::collectd':
-  purge        => true,
-  recurse      => true,
-  purge_config => true,
+  purge           => true,
+  recurse         => true,
+  purge_config    => true,
+  minimum_version => '5.4',
 }
 ```
 
@@ -38,6 +39,10 @@ the default configurations shipped in collectd.conf and use
 custom configurations stored in conf.d. From here you can set up
 additional plugins as shown below.
 
+Specifying the version or minimum_version of collectd as shown above reduces the need for
+two puppet runs to coverge. See [Puppet needs two runs to correctly write my conf, why?](#puppet-needs-two-runs-to-correctly-write-my-conf,-why?) below.
+
+
 Simple Plugins
 --------------
 
@@ -1086,7 +1091,10 @@ See metadata.json for supported platforms
 
 ##Known issues
 
+###Puppet needs two runs to correctly write my conf, why?
+
 Some plugins will need two runs of Puppet to fully generate the configuration for collectd. See [this issue](https://github.com/pdxcat/puppet-module-collectd/issues/162).
+This can be avoided by specifying an explicit version (`$version`) or a minimum version (`$minimum_version`) to the collectd class
 
 ##Development
 

manifests/init.pp

@@ -14,12 +14,16 @@
   $write_queue_limit_low  = undef,
   $package_name           = $collectd::params::package,
   $version                = installed,
+  $minimum_version        = undef,
 ) inherits collectd::params {
 
   $plugin_conf_dir = $collectd::params::plugin_conf_dir
   validate_bool($purge_config, $fqdnlookup)
   validate_array($include, $typesdb)
 
+  # Version for templates
+  $_collectd_version = pick($::collectd_version, regsubst($version, '^\d+(?:\.\d+){1.2}', '\0'), $minimum_version)
+
   package { $package_name:
     ensure   => $version,
     name     => $package_name,

spec/classes/collectd_plugin_cpu_spec.rb

@@ -33,7 +33,7 @@
     context 'cpu options should be set with collectd 5.5' do
       let :facts do
         {:osfamily            => 'RedHat',
-         :collectd_version    => '5.5',
+         :_collectd_version    => '5.5',
         }
       end
       let :params do

spec/classes/collectd_plugin_exec_spec.rb

@@ -9,7 +9,7 @@
       :id               => 'root',
       :kernel           => 'Linux',
       :path             => '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin',
-      :collectd_version => '5.0'
+      :_collectd_version => '5.0'
     }
   end
 

spec/classes/collectd_plugin_libvirt_spec.rb

@@ -9,10 +9,10 @@
       }
     end
 
-    context 'with collectd_version < 5.0' do
+    context 'with _collectd_version < 5.0' do
       let :facts do
         { :osfamily => 'Debian',
-          :collectd_version => '4.10.1',
+          :_collectd_version => '4.10.1',
         }
       end
 
@@ -22,10 +22,10 @@
       end
     end
 
-    context 'with collectd_version >= 5.0' do
+    context 'with _collectd_version >= 5.0' do
       let :facts do
         { :osfamily => 'Debian',
-          :collectd_version => '5.0.0',
+          :_collectd_version => '5.0.0',
         }
       end
 

spec/classes/collectd_plugin_memory_spec.rb

@@ -18,7 +18,7 @@
   context ':ensure => present, specific params, collectd version 5.4.2' do
     let :facts do
       { :osfamily         => 'Redhat',
-        :collectd_version => '5.4.2'
+        :_collectd_version => '5.4.2'
       }
     end
 
@@ -38,7 +38,7 @@
   context ':ensure => present, specific params, collectd version 5.5.0' do
     let :facts do
       { :osfamily         => 'Redhat',
-        :collectd_version => '5.5.0'
+        :_collectd_version => '5.5.0'
       }
     end
 

spec/classes/collectd_plugin_openvpn_spec.rb

@@ -8,7 +8,7 @@
   context ':ensure => present, default params' do
     let :facts do
       { :osfamily => 'RedHat',
-        :collectd_version => '5.4',
+        :_collectd_version => '5.4',
       }
     end
 
@@ -24,7 +24,7 @@
   context ':statusfile param is an array' do
     let :facts do
       { :osfamily => 'RedHat',
-        :collectd_version => '5.4',
+        :_collectd_version => '5.4',
       }
     end
 
@@ -47,7 +47,7 @@
   context ':statusfile is a string but not an absolute path' do
     let :facts do
       { :osfamily => 'RedHat',
-        :collectd_version => '5.4',
+        :_collectd_version => '5.4',
       }
     end
 
@@ -64,7 +64,7 @@
   context ':statusfile param is not a string or array' do
     let :facts do
       { :osfamily => 'RedHat',
-        :collectd_version => '5.4',
+        :_collectd_version => '5.4',
       }
     end
 
@@ -80,7 +80,7 @@
   context ':improvednamingschema is not a bool' do
     let :facts do
       { :osfamily => 'RedHat',
-        :collectd_version => '5.4'}
+        :_collectd_version => '5.4'}
     end
     let :params do
       {:improvednamingschema => "true"}
@@ -94,7 +94,7 @@
   context ':collectcompression is not a bool' do
     let :facts do
       { :osfamily => 'RedHat',
-        :collectd_version => '5.4'}
+        :_collectd_version => '5.4'}
     end
     let :params do
       {:collectcompression => "true"}
@@ -108,7 +108,7 @@
   context ':collectindividualusers is not a bool' do
     let :facts do
       { :osfamily => 'RedHat',
-        :collectd_version => '5.4'}
+        :_collectd_version => '5.4'}
     end
     let :params do
       {:collectindividualusers => "true"}
@@ -122,7 +122,7 @@
   context ':collectusercount is not a bool' do
     let :facts do
       { :osfamily => 'RedHat',
-        :collectd_version => '5.4'}
+        :_collectd_version => '5.4'}
     end
     let :params do
       {:collectusercount => "true"}
@@ -136,7 +136,7 @@
   context ':interval is not default and is an integer' do
     let :facts do
       { :osfamily => 'RedHat',
-        :collectd_version => '5.4'}
+        :_collectd_version => '5.4'}
     end
     let :params do
       {:interval => 15}
@@ -154,7 +154,7 @@
   context ':ensure => absent' do
     let :facts do
       { :osfamily         => 'RedHat',
-        :collectd_version => '5.4',
+        :_collectd_version => '5.4',
       }
     end
     let :params do

spec/classes/collectd_plugin_processes_spec.rb

@@ -8,7 +8,7 @@
       :id               => 'root',
       :kernel           => 'Linux',
       :path             => '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin',
-      :collectd_version => '5.0'
+      :_collectd_version => '5.0'
     }
   end
 

spec/classes/collectd_plugin_python_spec.rb

@@ -9,7 +9,7 @@
       :id               => 'root',
       :kernel           => 'Linux',
       :path             => '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin',
-      :collectd_version => '5.0'
+      :_collectd_version => '5.0'
     }
   end
 

spec/classes/collectd_plugin_swap_spec.rb

@@ -18,7 +18,7 @@
   context ':ensure => present, specific params, collectd version 5.0' do
     let :facts do
       { :osfamily         => 'Redhat',
-        :collectd_version => '5.0'
+        :_collectd_version => '5.0'
       }
     end
 
@@ -34,7 +34,7 @@
   context ':ensure => present, specific params, collectd version 5.2.0' do
     let :facts do
       { :osfamily         => 'Redhat',
-        :collectd_version => '5.2.0'
+        :_collectd_version => '5.2.0'
       }
     end
 
@@ -50,7 +50,7 @@
   context ':ensure => present, specific params, collectd version 5.5.0' do
     let :facts do
       { :osfamily         => 'Redhat',
-        :collectd_version => '5.5.0'
+        :_collectd_version => '5.5.0'
       }
     end
 

spec/classes/collectd_plugin_varnish_spec.rb

@@ -4,7 +4,7 @@
   context 'When the version is not 5.4' do
     let :facts do
       { :osfamily => 'RedHat',
-        :collectd_version => '5.3',
+        :_collectd_version => '5.3',
       }
     end
     let :params do
@@ -23,7 +23,7 @@
   context 'When the version is nil' do
     let :facts do
       { :osfamily => 'RedHat',
-        :collectd_version => nil,
+        :_collectd_version => nil,
       }
     end
     let :params do
@@ -43,7 +43,7 @@
   context 'When the version is 5.4' do
     let :facts do
       { :osfamily => 'RedHat',
-        :collectd_version => '5.4',
+        :_collectd_version => '5.4',
       }
     end
     context 'when there are no params given' do

spec/classes/collectd_plugin_write_graphite_spec.rb

@@ -9,7 +9,7 @@
       :id               => 'root',
       :kernel           => 'Linux',
       :path             => '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin',
-      :collectd_version => '5.0'
+      :_collectd_version => '5.0'
     }
   end
 
@@ -101,7 +101,7 @@
         :id               => 'root',
         :kernel           => 'Linux',
         :path             => '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin',
-        :collectd_version => '5.3'
+        :_collectd_version => '5.3'
       }
     end
     let :params do

spec/defines/collectd_plugin_network_listener_spec.rb

@@ -6,7 +6,7 @@
     let :facts do
       {
         :osfamily         => 'Redhat',
-        :collectd_version => '4.6'
+        :_collectd_version => '4.6'
       }
     end
     let (:title) {'mylistener'}
@@ -29,7 +29,7 @@
     let :facts do
       {
         :osfamily         => 'Redhat',
-        :collectd_version => '5.1.0'
+        :_collectd_version => '5.1.0'
       }
     end
     let (:title) {'mylistener'}

spec/defines/collectd_plugin_network_server_spec.rb

@@ -6,7 +6,7 @@
     let :facts do
       {
           :osfamily         => 'Redhat',
-          :collectd_version => '5.1.0'
+          :_collectd_version => '5.1.0'
       }
     end
     let (:title) {'node1'}

spec/defines/collectd_plugin_spec.rb

@@ -6,7 +6,7 @@
     let(:title) { 'test' }
     let :facts do
       {
-          :collectd_version => '5.3',
+          :_collectd_version => '5.3',
           :osfamily         => 'Debian',
       }
     end
@@ -21,7 +21,7 @@
     let(:title) { 'test' }
     let :facts do
       {
-          :collectd_version => '4.9.3',
+          :_collectd_version => '4.9.3',
           :osfamily         => 'Debian',
       }
     end

spec/defines/collectd_plugin_write_graphite_spec.rb

@@ -14,7 +14,7 @@
     let(:title) { 'graphite_udp' }
     let :facts do
       { :osfamily => 'RedHat',
-        :collectd_version => '5.3',
+        :_collectd_version => '5.3',
         :concat_basedir => tmpfilename('collectd-graphite'),
       }
     end
@@ -34,7 +34,7 @@
     let(:title) { 'wg' }
     let :facts do
       { :osfamily => 'RedHat',
-        :collectd_version => '5.4',
+        :_collectd_version => '5.4',
         :concat_basedir => tmpfilename('collectd-graphite'),
       }
     end

templates/loadplugin.conf.erb

@@ -1,8 +1,8 @@
 # Generated by Puppet
-<% if @collectd_version and (scope.function_versioncmp([@collectd_version, '4.9.4']) >= 0) -%>
+<% if @_collectd_version and (scope.function_versioncmp([@_collectd_version, '4.9.4']) >= 0) -%>
 <LoadPlugin <%= @plugin %>>
   Globals <%= @globals %>
-<% if @interval and @collectd_version and (scope.function_versioncmp([@collectd_version, '5.2']) >= 0) -%>
+<% if @interval and @_collectd_version and (scope.function_versioncmp([@_collectd_version, '5.2']) >= 0) -%>
   Interval <%= @interval %>
 <% end -%>
 </LoadPlugin>

templates/mysql-database.conf.erb

@@ -5,7 +5,7 @@
     Host "<%= @host %>"
     User "<%= @username %>"
     Password "<%= @password %>"
-<% if @collectd_version and (scope.function_versioncmp([@collectd_version, '5.0']) >= 0) -%>
+<% if @_collectd_version and (scope.function_versioncmp([@_collectd_version, '5.0']) >= 0) -%>
     Port "<%= @port %>"
 <% else -%>
     Port <%= @port %>

templates/plugin/cpu.conf.erb

@@ -1,4 +1,4 @@
-<% if @collectd_version and (scope.function_versioncmp([@collectd_version, '5.5']) >= 0) -%>
+<% if @_collectd_version and (scope.function_versioncmp([@_collectd_version, '5.5']) >= 0) -%>
 <Plugin cpu>
   ReportByState = <%= @reportbystate %>
   ReportByCpu = <%= @reportbycpu %>

templates/plugin/curl-page.conf.erb

@@ -16,10 +16,10 @@
 <% if @cacert -%>
     CACert "<%= @cacert %>"
 <% end -%>
-<% if @header and @collectd_version and (scope.function_versioncmp([@collectd_version, '5.3']) >= 0) -%>
+<% if @header and @_collectd_version and (scope.function_versioncmp([@_collectd_version, '5.3']) >= 0) -%>
     Header "<%= @header %>"
 <% end -%>
-<% if @post and @collectd_version and (scope.function_versioncmp([@collectd_version, '5.3']) >= 0) -%>
+<% if @post and @_collectd_version and (scope.function_versioncmp([@_collectd_version, '5.3']) >= 0) -%>
     Post "<%= @post %>"
 <% end -%>
 <% unless @measureresponsetime.nil? -%>