Introducing Puppet Module Data

[jfryman]

This PR is an incremental step to introduce changes that were proposed in #423.

The scope of this pull is to eliminate the params.pp class, and collapse any user-configurable data to Hiera. Instead of attempting to explain why, I'll link this wonderful post.

http://www.devco.net/archives/2013/12/08/better-puppet-modules-using-hiera-data.php

[3flex]

Make this a warning?

[jfryman]

I'm not a fan of warning - there is a tendency for those to get lost on a Puppetmaster logs, as opposed to a notify that is in-your-face on the client.

[3flex]

Looks good! Should it be titled "Introducing Puppet Module Data"?

So nginx::config will be the primary interface to the module. Just wondering why not have nginx class be the primary interface? Merging nginx::config into nginx is cleaner to me, otherwise there are nginx and nginx::config public classes. Although if you're going to make the entire nginx::config class optional like with nginx::service and nginx::package as you're planning then this separation totally makes sense and you can ignore this.

The other thing is that I'd like to see only OS-level defaults set in the module data. I don't think it makes sense to have stuff like nginx::config::gzip: on in the module data, I'd rather see that default set in config.pp and only have default user, file paths, and things that will differ by OS set in module data.

[3flex]

Facter falls back to kernel when it doesn't have a specific match for osfamily, so I think having kernel in the hierarchy is redundant

[jfryman]

That functionality is only with Facter 2.2, right? I don't know if we can assert that across the board yet. Whadda think?

[3flex]

Looks like it's always been that way: puppetlabs/facter@8f938c1

[jfryman]

Heh. TIL. :) Incoming change.

[3flex]

Last thing: the hierarchy may need to include operatingsystem (see Joyent/SmartOS comment above) and possibly operatingsystemmajrelease - things may change between OS versions that require config changes. PID on Red Hat 7 vs 6 springs to mind (#431).

So I'd suggest the hierarchy be:

- "operatingsystem/%{::operatingsystem}"
- "osfamily/%{::osfamily}"
- common

Then maybe include operatingsystemmajrelease if there's a need to.

[3flex]

Should undo this change, probably a typo

[jfryman]

So nginx::config will be the primary interface to the module. Just wondering why not have nginx class be the primary interface? Merging nginx::config into nginx is cleaner to me, otherwise there are nginx and nginx::config public classes. Although if you're going to make the entire nginx::config class optional like with nginx::service and nginx::package as you're planning then this separation totally makes sense and you can ignore this.

I could probably go either way with this... I do like the isolation with the package/file/service pattern, and to let the base-class be responsible for relationship chaining, hiera create_resources instantiation. I can see that we might get ourselves into a similar situation down the road like we have with the resources namespace, and having to collapse that in.

The other thing is that I'd like to see only OS-level defaults set in the module data. I don't think it makes sense to have stuff like nginx::config::gzip: on in the module data, I'd rather see that default set in config.pp and only have default user, file paths, and things that will differ by OS set in module data.

Yeah, I know you brought this up in the previous pull, but I really like the idea of everything data driven in the puppet-module-data. My reasoning: I have received some pretty odd requests to be able to modify some parts of config to make it mutable, versus immutable in params.pp (unless you fork, which I really would like to avoid).

I really think (hope) that by having any configurable value in Hiera that gives a the user the ability to be as flexible as possible, while not creating a ton of edge-cases in code.

[jfryman]

@3flex and thanks for cleaning up the remaining bits. I got tied up with this weekends activities.... https://twitter.com/Mekaeg/status/513429383147573248

[3flex]

I have received some pretty odd requests to be able to modify some parts of config to make it mutable, versus semi-mutable in params.pp

This totally made sense when there were some things in params.pp which were put directly into templates, making them effectively hard coded. But if they were exposed as parameters on the class (which they all are now?) then they can still be configured using normal hiera-puppet integration. Not having non-OS related stuff in the module data doesn't stop this from happening (am I making sense here?). For new functionality you still have to add the parameters to the class, but now you have to add it with default "undef" then go into common.yaml and add the real default there too, but only if that default isn't "undef". That doesn't sound any different to now where you add a parameter to a class, then add the default to params.pp.

I'll leave it at that - either way it'll have to be documented!

[jfryman]

That doesn't sound any different to now where you add a parameter to a class, then add the default to params.pp.

That is true for adding new functionality, but not the same experience if I'm purely consuming the module. I think you're right - we'll need to document the change. But, this allows a user more flexibility. That's who I'm targeting here. Does that make sense? I might be splitting hairs, but it's a thought experiment I'm playing with.

[jfryman]

OK! Final 👍 ?

[3flex]

Almost!

• I realize now that this change will force use of hiera. It will no longer be possible for users to declare nginx::config with parameters in Puppet code since this will break as nginx::config would have been declared twice (once in init.pp then again in the user's code):

#user declares nginx class
    include nginx

#then declares nginx::config with their custom parameters
    class {'nginx::config':
      ...
    }
#this will break

This might cause more pain than intended.

• package_name, package_source, package_ensure, manage_repo are now marked deprecated in init.pp. But the new hiera doc only talks about the nginx > nginx::config transition, and they're in common.yaml under nginx::config which means nginx::package will never see them.
• As for previous point but with configtest_enable, service_restart, service_ensure and the nginx::service class.

I ran out of time tonight but I haven't yet had a close look at the new hierarchy and make sure it matches everything in params.pp.

[3flex]

Sorry, my first point was kind of stupid, you already can't declare nginx::config like that for the same reason. But this change does still force users to use hiera who may have been happily ignoring it until now, since there'll be no other way to set values in nginx::config because trying to declare that class in Puppet code is impossible.

[jfryman]

I realize now that this change will force use of hiera

Well, I wonder how bad this really is. It's built into Puppet 3, so I don't feel so bad for taking advantage of what I would consider a core part of the language now.

That being said, I don't think that it's all dire. We could include a note in the upgrade notes that says something like this...

In the event that you are unable to leverage Hiera for your attribute configuration, you can use the Spaceship Operator to set the parameters for the nginx::config class. For example:

Class<| title == 'nginx::class' |> {
  proxy_cache_levels => '2',
}

The recommended path is to use Hiera, but this pattern should give you an intermediate step during the upgrade process.

That will allow us to get around the multiple declaration concern.

package_name, package_source, package_ensure, manage_repo are now marked deprecated in init.pp... configtest_enable, service_restart, service_ensure and the nginx::service class.

Hmmm. Yeah. Good point. This will get fixed with the upcoming factory pattern I proposed in #423, but probably should come out of this pull.

These are really great concerns... are they all being knocked out?

[3flex]

As well as your last commit I think you should update the service/package parameters in common.yaml as well?

But that's it from me! Sorry to be so pedantic. I think that upgrade message would be suitable. And the hierarchy looks fine. 👍 to merge!

[jfryman]

I don't think this is required - it is for rspec for some reason, but it seems to be plug 'n' play when using it normally. While testing puppet apply worked without specifically enabling the backend, and the module_data docs say

I thought that as well, but we had to include it in the spec fixtures. I guess that threw me off. I can nix that.

[jfryman]

Here we go!!!!!