Modernize module #35
Modernize module #35
Conversation
|
Could you please enable travis on this repo? |
| PuppetLint.configuration.send('disable_80chars') | ||
| PuppetLint.configuration.ignore_paths = ["spec/**/*.pp", "pkg/**/*.pp"] | ||
|
|
||
| desc "Run puppet in noop mode and check for syntax errors." |
There was a problem hiding this comment.
You should have a look at puppet-syntax since it's a lot faster than calling things in shell.
There was a problem hiding this comment.
Good point! I'll look into it.
Think it is good to merge as is though, considering that puppet uses this code now by default when generating modules.
There was a problem hiding this comment.
I created ghoneycutt#1 to integrate this, plus some other things.
|
@hunner as the last person who touched this module, what do you think about this PR? |
| @@ -23,6 +25,8 @@ | |||
| validate_array($ntpservers) | |||
|
|
|||
| include dhcp::params | |||
| include dhcp::monitor | |||
| include concat::setup | |||
There was a problem hiding this comment.
I thought concat::setup was deprecated.
There was a problem hiding this comment.
Not sure why that is there..good eye. Shall I just remove that line?
|
PuppetLabs folks: I'll squash these commits upon request, please do not merge as is. |
| @@ -1,6 +1,10 @@ | |||
| fixtures: | |||
| repositories: | |||
| concat: 'git://github.com/puppetlabs/puppetlabs-concat.git' | |||
| stdlib: 'git://github.com/puppetlabs/puppetlabs-stdlib.git' | |||
| concat: | |||
There was a problem hiding this comment.
I'd prefer always checking against the newest git versions.
There was a problem hiding this comment.
That does not make for very good tests. We should be explicit about what version is supported and test against an exact ref, not HEAD.
|
https://github.com/garethr/puppet-module-skeleton has a very nice set of defaults for Gemfile, Rakefile and .travis.yml btw. |
| class dhcp ( | ||
| $dnsdomain, | ||
| $nameservers, | ||
| $dnsdomain = [ $::domain ], |
There was a problem hiding this comment.
puppet-lint produces a false positive here.
There was a problem hiding this comment.
Yeah, its got a bug when you use a fact as a value for a param. It is OK though.
There was a problem hiding this comment.
Not if you want to set up puppet-lint to fail on warnings, which IMHO you do want.
There was a problem hiding this comment.
If $::domain is undef, then this will blow up in the template. Many customers have boxes without a domain fact value.
There was a problem hiding this comment.
|
I think we're ok with most of this! We made a few comments along the way but it's in pretty good shape for merging. We're going to update the style guide to say inheriting from params.pp is fine. |
|
@apenney moved that logic into params. Anything else you want me to fix up before squashing commits? |
|
@apenney squashed the commits |
|
@ghoneycutt I think it also needs a rebase now. |
|
rebased |
| mode => '0644', | ||
| before => Package[$packagename], | ||
| notify => Service[$servicename], | ||
| content => template('dhcp/debian/default_isc-dhcp-server'), | ||
| } | ||
| 'redhat','centos','fedora','Scientific': { |
There was a problem hiding this comment.
This should be rewritten to $::osfamily as well.
|
@apenney Could you please merge this? |
|
@ghoneycutt No, you should first fix the issue in |
|
You changed rewrote the case statement to an if statement and in the rebase this resulted in invalid syntax. That must be fixed before it can be merged. |
|
@ekohl fixed |
Taking a quick pass at adding support for Travis, Puppet v3, Ruby 1.8.7, 1.9.3, and 2.0.0 and passing lint. Also specifies the version of concat (1.1.0) and stdlib (4.1.0).
|
@apenney rebased so this can be merged. |
This patch enforces the use of parameterized classes and encoding data in your manifests.
|
The only issue I'm aware of now is the dnsdomain => parameter that defaults to $::domain. Because this may be undef for many users this'll make the template catch on fire and explode. I think that's literally the only issue left before this can be merged. (Having said that some of this PR, the travis stuff, etc) will eventually get overwritten as we're starting to manage those through a central repo that can automatically build and commit these files for all modules. |
|
@ghoneycutt ping? Can you update the last issue with this PR? |
|
@mhaskel Thanks for the reminder as I had forgotten about this code.. pushed up what I was working on previously. |
| } else { | ||
| fail('dhcp::dnsdomain must be set and domain fact is missing to use as a default value.') | ||
| } | ||
| } |
There was a problem hiding this comment.
I think you're missing an
else {
$dnsdomain_real = $dnsdomain
}
|
@ghoneycutt ping? Can you update the last issue that @mhaskel mentioned. |
This patch works around the case where the domain fact is absent while ensuring that all parameters have default values.
|
Updated the code as requested as well as updating the Travis-ci framework. This code needs to be rebased, which I have no interest in doing. Perhaps someone that uses this module and wants the extra functionality that I added will have time to do so. |
Taking a quick pass at adding support for Travis, Puppet v3, Ruby 1.8.7,
1.9.3, and 2.0.0 and passing lint. Also specifies the version of concat
(1.1.0) and stdlib (4.1.0).