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_real_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 #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
 --------------
 
@@ -1168,7 +1173,12 @@ See metadata.json for supported platforms
 
 ##Known issues
 
-Some plugins will need two runs of Puppet to fully generate the configuration for collectd. See [this issue](https://github.com/puppet-community/puppet-collectd/issues/162).
+###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`) for the collectd class. e.g. Setting either of these to 1.2.3 will
+make this module assume on the first run (when the fact responsible to provide the collectd version is not yet available) that your systems are running collectd 1.2.3
+and generate the configuration accordingly.
 
 ##Development
 

lib/facter/collectd_version.rb → lib/facter/collectd_real_version.rb

@@ -1,12 +1,12 @@
-# Fact: collectd_version
+# Fact: collectd_real_version
 #
 # Purpose: Retrieve collectd version if installed
 #
 # Resolution:
 #
 # Caveats:  not well tested
 #
-Facter.add(:collectd_version) do
+Facter.add(:collectd_real_version) do
   setcode do
     if Facter::Util::Resolution.which('collectd')
       collectd_help = Facter::Util::Resolution.exec('collectd -h') and collectd_help =~ /^collectd ([\w.]+), http:\/\/collectd.org\//

manifests/init.pp

@@ -15,12 +15,22 @@
   $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_real_version,                                                      # Fact takes precedence
+    regsubst(
+      regsubst($version,'^(absent|held|installed|latest|present|purged)$', ''), # standard package resource ensure value? - strip and return undef
+      '^\d+(?:\.\d+){1.2}', '\0'),                                                 # specific package version? return only semantic version parts
+    $minimum_version,
+    '1.0')
+
   package { $package_name:
     ensure   => $version,
     name     => $package_name,

spec/classes/test_collectd_version_spec.rb

@@ -0,0 +1,59 @@
+require 'spec_helper'
+
+describe 'test::collectd_version' do
+
+ let :facts do
+   {:osfamily => 'RedHat'}
+ end
+
+ it { should compile }
+
+ context 'when no explicit value is specified' do
+   it { should contain_file('collectd_version.tmp').with_content(/^1\.0$/) }
+ end
+
+ context 'when minimum_version is specified' do
+   let :params do
+     {
+       :version         => 'installed',
+       :minimum_version => '5.4',
+     }
+   end
+   it { should contain_file('collectd_version.tmp').with_content(/^5\.4$/) }
+ end
+
+ context 'when version is explicit and greater than minimum_version' do
+   let :params do
+     {
+       :version         => '5.6.3',
+       :minimum_version => '5.4',
+     }
+   end
+   it { should contain_file('collectd_version.tmp').with_content(/^5\.6\.3$/) }
+ end
+
+ context 'when version is explicit and less than minimum_version' do
+   let :params do
+     {
+       :version         => '5.3',
+       :minimum_version => '5.4',
+     }
+   end
+   it { should contain_file('collectd_version.tmp').with_content(/^5\.3$/) }
+ end
+
+ context 'when collectd_real_version is available' do
+   let :facts do
+     {
+       :osfamily              => 'Redhat',
+       :collectd_real_version => '5.6',
+     }
+   end
+   let :params do
+     {
+       :minimum_version => '5.4'
+     }
+   end
+   it { should contain_file('collectd_version.tmp').with_content(/^5\.6$/) }
+ end
+end

spec/fixtures/modules/test/manifests/collectd_version.pp

@@ -0,0 +1,19 @@
+# class used solely to test the collectd_version expansion in init.pp
+# Note that fact collectd_real_version is also used by init.pp
+# Write the generated value to a template so we can test it
+class test::collectd_version(
+  $version         = undef,
+  $minimum_version = undef,
+) {
+  class { '::collectd':
+    version         => $version,
+    minimum_version => $minimum_version,
+  }
+
+  file { 'collectd_version.tmp':
+    ensure  => file,
+    path    => '/tmp/collectd_version.tmp',
+    content => template('test/collectd_version.tmp.erb'),
+    require => Class['Collectd'],
+  }
+}

spec/fixtures/modules/test/templates/collectd_version.tmp.erb

@@ -0,0 +1 @@
+<%= scope.lookupvar('::collectd::collectd_version') %>

spec/unit/collectd_version_spec.rb → spec/unit/collectd_real_version_spec.rb

@@ -1,22 +1,22 @@
 require 'spec_helper'
 
-describe 'collectd_version', :type => :fact do
+describe 'collectd_real_version', :type => :fact do
   before { Facter.clear }
   after { Facter.clear }
 
   it 'should be 5.1.0 according to output' do
     Facter::Util::Resolution.stubs(:which).with("collectd").returns("/usr/sbin/collectd")
     sample_collectd_help = File.read(fixtures('facts','collectd_help'))
     Facter::Util::Resolution.stubs(:exec).with("collectd -h").returns(sample_collectd_help)
-    expect(Facter.fact(:collectd_version).value).to eq('5.1.0')
+    expect(Facter.fact(:collectd_real_version).value).to eq('5.1.0')
 
   end
 
   it 'should be 5.1.0.git according to output' do
     Facter::Util::Resolution.stubs(:which).with("collectd").returns("/usr/sbin/collectd")
     sample_collectd_help_git = File.read(fixtures('facts','collectd_help_git'))
     Facter::Util::Resolution.stubs(:exec).with("collectd -h").returns(sample_collectd_help_git)
-    expect(Facter.fact(:collectd_version).value).to eq('5.1.0.git')
+    expect(Facter.fact(:collectd_real_version).value).to eq('5.1.0.git')
   end