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

[bdellegrazie]

…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 #162

[blkperl]

PIng @txaj for review

[blkperl]

Is it possible to write a spec test to test the new behavior?

[bdellegrazie]

Hi @blkperl, I'd like to add tests for the new functionality but I'm very new to puppet-rspec and I'm not sure how. Specifically I'm having difficulty accessing the $_collectd_version created in init.pp as its essentially being used as a module-wide variable in the templates.
Do you have any suggestions?

[bdellegrazie]

ping @blkperl and @txaj - any update or further comments?

[txaj]

Could you be more explicit about how this parameter works ? like "setting this parameter to 9.99 will makes this module assume at the first run (when the fact responsible to provide the collectd version is not yet avaibable) that your systems are running collectd 9.99, and generate the configuration accordingly"

[bdellegrazie]

Sure will do

[txaj]

@bdellegrazie for testing I only see one way : take a module that generates a different configuration depending on the collectd version, and test the output when you set the fact to undef/value1/value2 and the default to undef/value1/value2 and check that it makes sense

[bdellegrazie]

@txaj, @blkperl: I've updated the PR to do the following:

• renamed collectd_version fact to collectd_real_version
• corrected an error with the regexp which selects the version for the templates.

I had to use a test module in the fixtures dir to be able to create the tests around the collect_version but they do work.

[igalic]

thanks @bdellegrazie and @txaj! i've merged this manually.

[igalic]

since merging this, we now have a failing test

@bdellegrazie do i understand it correctly, that we should be checking for collectd_real_version here (as well as in the code)?

[bdellegrazie]

yes the fact should be collectd_real_version - I thought it was on my
branch?

On 28 September 2015 at 16:15, Igor Galić notifications@github.com wrote:

since merging this, we now have a failing test
https://github.com/puppet-community/puppet-collectd/blob/master/spec/classes/collectd_init_spec.rb#L73-L77

@bdellegrazie https://github.com/bdellegrazie do i understand it
correctly, that we should be checking for collectd_real_version here (as
well as in the code)?

—
Reply to this email directly or view it on GitHub
#305 (comment)
.

Kind regards,

Brett Delle Grazie

[bdellegrazie]

Okay, I realise someone has added more tests (great!) but when merging the
fact needs to be collectd_real_version - then the internal
'collectd_version' variable that is used by the templates will resolve
correctly.

On 28 September 2015 at 16:41, Brett Delle Grazie <
brett.dellegrazie@gmail.com> wrote:

yes the fact should be collectd_real_version - I thought it was on my
branch?

On 28 September 2015 at 16:15, Igor Galić notifications@github.com
wrote:

since merging this, we now have a failing test
https://github.com/puppet-community/puppet-collectd/blob/master/spec/classes/collectd_init_spec.rb#L73-L77

@bdellegrazie https://github.com/bdellegrazie do i understand it
correctly, that we should be checking for collectd_real_version here (as
well as in the code)?

—
Reply to this email directly or view it on GitHub
#305 (comment)
.

Kind regards,

Brett Delle Grazie

Kind regards,

Brett Delle Grazie

[igalic]

@bdellegrazie did i do a poor job of rebasing / merging your branch?

[bdellegrazie]

@igalic I'd hardly count two functions as a poor merge :)
No, there is however something 'broken' with Puppet 4.0 according to Travis output from PR #343.
I don't know how to fix this one however.