Clean up params logic. Add rspec tests.

[decibelhertz]

A fairly substantial PR, but I hope it helps.

MacPorts is explicitly required for Darwin, so I added some logic in params to do this, rather than count on the (optional) ::provider Facter fact. Further, users should generally be able to override anything in params, so I set init to inherit that class. I also made use of some path paramters in several resources in order to not depend on variables from another class in all the defined types.

All this necessitated a change in what to include for all the defined types. Since the init class now controls everything, all defined types should include that, not params. To convince myself all was working, I paid the pain of writing a bunch of rspec tests for this module to confirm. While looking at this, there was some unneeded items in the templates that I cleaned out.

To test all that rspec, I had to hack on Gemfile, Rakefile and spec/spec_helper.rb. Not sure if whatever was being done will stay working; I'm happy to adjust that (or other items) upon request.

[rlex]

May i ask why you removed $dnsmasq::params::dnsmasq_conffile from target => ? Overall, this looks great.

[decibelhertz]

Thanks; happy to help. Looks like Travis CI is complaining about Facter so I'll do one more push to fix that.

I removed $dnsmasq::params::dnsmasq_conffile to minimize importing variables and make the init class a bit more consistent. E.G., the service resource already had its title and name set to 'dnsmasq'/$dnsmasq_service respectively to ease difference between OSes; seemed like concat would benefit from that too.

One can set $name and $path in concat (like file), so in the init class (and just the init class), we can call the resource 'dnsmasq.conf' and set the path to the variable. In this way, all those defined types can refer to something static without any need to refer to the variable in the params class.

Further, the previous way the defined types were set up made one have to declare/include the init class explicitly. Now, since each defined type includes this init class instead of just dnsmasq::params, one can cause dnsmasq to spring into existence by only calling a defined type of interest. If the defaults of the init class are not desired, one can tweak in hiera (Puppet 3.x) and never explicitly call the init class in your manifests. I.E.:

Old (hiera):

---
dnsmasq::save_config_file: false
...

Old (puppet):

include dnsmasq
dnsmasq::domain { 'example.com: }

New (hiera):

---
dnsmasq::save_config_file: false
...

New (puppet)

dnsmasq::domain { 'example.com: }

[rlex]

Add some basic input checkers where its easy. Punted on IPv6. I think stdlib will someday have a widespread function that will confirm a string is a valid IP, but not with Puppet 2.x support still going.

Stdlib 4 has support for puppet 2.7 and i think we can safely drop support for 2.6. Anyway, you already require stdlib for validate_bool.

[decibelhertz]

Correct. I should have been more clear: I was avoiding newer stdlib functions in order to not stomp on 2.x installations. If you want, I can switch to is_ip_address().

[rlex]

What's wrong with is_mac_address, btw?

[decibelhertz]

I must have pulled a bad ref from stdlib when I tested. Better to pull 4.0.0 so as not to use newer functions for 2.7 (and above) compatibility. New commit for that.

[rlex]

I need to check if dnsmasq supports "short" mac address. If i remember correctly, mac address can drop trailing zero from octet, so 10:10:10:10:10:10 and 1:1:1:1:1:1 is basically same addresses.

[decibelhertz]

http://www.thekelleys.org.uk/dnsmasq/docs/dnsmasq-man.html does not explicitly state it takes non-padded syntax. I think its probably safer to change to validate_re($mac,'^[a-fA-F0-9]{2}:[a-fA-F0-9]{2}:[a-fA-F0-9]{2}:[a-fA-F0-9]{2}:[a-fA-F0-9]{2}:[a-fA-F0-9]{2}$')

[decibelhertz]

Looking at this more, we really ought to standardize on upper or lower case for the rendered product. That also makes validation easier. \h -- what I used before -- doesn't work with ruby 1.8.7, so better to use stdlib's style.

jmeyer@anfwebproc:~ $ ruby --version
ruby 1.8.7 (2011-12-28 patchlevel 357) [sparc-solaris2.10]
jmeyer@anfwebproc:~ $ irb
irb(main):001:0> '40' =~ Regexp.compile('\d{2}')
=> 0
irb(main):002:0> '40' =~ Regexp.compile('\h{2}')
=> nil

[rlex]

Tested against my old manifest, throws

Dec 25 17:07:41 php-1.srv.pnc9.lan puppet-agent[24300]: Could not retrieve catalog from remote server: Error 400 on SERVER: Invalid parameter value on Dnsmasq::Ptr[uno-reverse] at /etc/puppet/environments/puppetdev/manifests/nodes/test.pp:640 on node php-1.srv.pnc9.lan

You removed $value from dnsmasq::ptr class and this breaks existing configs.

[rlex]

Other than that, is works great.

[rlex]

PTR can take different values, please note: http://pig.made-it.com/dns-sd.html

[decibelhertz]

Ok. That ought to square things away. concat has changed a bit over its releases, so I made sure we can use older versions in rake spec, as with stdlib, too.

[rlex]

Will try to check this today and merge it

[decibelhertz]

Still planning to merge? Cheers.

[rlex]

Oh, pardon me. On a long vacation. I'll merge it and release new version as soon as i go back to work

[decibelhertz]

No problem. Thanks for the update.

[rlex]

Thank you again!

[decibelhertz]

Glad to help. Cheers.