Updated Module to Puppet >= 4.X #40
Conversation
.travis.yml
Outdated
| @@ -2,22 +2,10 @@ | |||
| language: ruby | |||
|
|
|||
| rvm: | |||
| - 1.9.3 | |||
| - 2.1.0 | |||
| - 2.3.1 | |||
There was a problem hiding this comment.
The pc1 aio packaging still ships with ruby 2.1.9
/opt/puppetlabs/puppet/bin/ruby --version
ruby 2.1.9p490 (2016-03-30 revision 54437) [x86_64-linux]Unless there are plans from puppet to change that, I think we should stick with 2.1.9 and continue to test against that.
If we want to do 2.3.1 also, that's probably OK.
| @@ -2,22 +2,11 @@ | |||
| language: ruby | |||
|
|
|||
| rvm: | |||
| - 1.9.3 | |||
| - 2.1.0 | |||
| - 2.1.9 | |||
| - 2.3.1 | |||
There was a problem hiding this comment.
I dont think they are using 2.3.1 yet and might skip it entirely for 2.4. Suggest removing this.
| - PUPPET_GEM_VERSION="~> 3.7.0" CHECK=test | ||
| - PUPPET_GEM_VERSION="~> 3.8.0" CHECK=test | ||
| - PUPPET_GEM_VERSION="~> 3.8.0" CHECK=build DEPLOY_TO_FORGE=yes | ||
| - PUPPET_GEM_VERSION="~> 3" FUTURE_PARSER="yes" CHECK=test | ||
| - PUPPET_GEM_VERSION="~> 4.0.0" CHECK=test |
There was a problem hiding this comment.
Do we want to explicitly support all releases of v4? I'm thinking of only supporting the current release and the two before it. Things really do change in minor versions and you'd be surprised how it can add it. It also greatly increases testing time.
| @@ -44,7 +44,7 @@ end | |||
| if (puppetversion = ENV['PUPPET_GEM_VERSION']) | |||
There was a problem hiding this comment.
we shouldn't need a version specified here at all. It will use the installed version or install the newest version if none is specified, which is what we want.
if puppetversion = ENV['PUPPET_GEM_VERSION']
gem 'puppet', puppetversion, :require => false
else
gem 'puppet', :require => false
end
| ) | ||
|
|
||
| validate_re($service_ensure, '^running|true|stopped|false$') | ||
|
|
||
| anchor { 'sssd::begin': } -> |
There was a problem hiding this comment.
Now is the time to get rid of all of these classes and the ancient bandaid of anchor'ing. We should put everything in one class.
There was a problem hiding this comment.
I don't agree with putting every thing in one class. I think having separate classes makes the code more readable and easier to understand. Most of the puppetlabs modules take this approach.
As for the anchors, I agree. We should remove them and switch to using contain.
| @@ -31,7 +31,7 @@ | |||
| } | |||
| ], | |||
| "requirements": [ | |||
| {"name":"puppet","version_requirement":">= 3.0.0 < 5.0.0" } | |||
| {"name":"puppet","version_requirement":">= 4.0.0 < 5.0.0" } | |||
| ], | |||
| "dependencies": [ | |||
| {"name":"puppetlabs/stdlib","version_requirement":">= 4.1.0 < 5.0.0"} | |||
There was a problem hiding this comment.
We should probably bump stdlib to the latest at this point, which we will want for the data types.
https://github.com/puppetlabs/puppetlabs-stdlib#data-types
This will mean we also need to modify the .fixtures.yml
|
The open PR's should be merged or closed and the open issues addressed. Luckily there are only a couple of each. |
|
@ghoneycutt, I agree with the rest of your suggestions, but should we hold up this MR to get all of that done? If @dpattmann is willing to do it, then great, but otherwise we can move that stuff to other issues/PRs and not cut 2.0.0 until its all in. |
|
I would like my PR that was merged before you branched this to be included. |
|
@edestecd having multiple classes makes things harder to read and understand. There is no point for having all these subclasses or the params class. What it really means is that I have to have all of them open to understand what is happening. They don't have any real value on their own. The params class only makes sense if you have multiple classes that need to share information, as this is a simple module that is only invoked with |
|
@edestecd @ghoneycutt I would prefer to merge this PR first and then have another PR to cleanup the module. |
|
Thanks for the contribution! |
Puppet 3.X is deprecated - This PR should fix #36