Setting nginx::config::xxx options in hiera does not work with puppet 4.3

[ckaenzig]

Puppet 4.3 seems to have a new data-binding code which breaks this module. With puppet 4.3, the default values of nginx::config are always used in the call to this class in init.pp. The data-binding mechanism is not called anymore, which means the values nginx::config::xxx are not used anymore.

It seems that in Puppet >= 4.3, passing undef to a class parameter stops the data-binding mechanism from happening and uses the class parameter's default value instead.

[ckaenzig]

I fixed my code by setting nginx::xxx instead of nginx::config::xxx (for example nginx::worker_processes), but now I have to live with that big deprecation warning.

[quixoten]

I also ran into an issue with this. I think the problem will go away when the deprecation warning is removed. I replaced the whole if !defined(Class['::nginx::config']) { block with include ::nginx::config and it started working.

When I tried to explicitly define

class { '::nginx::config': }

in the manifest that had include ::nginx I got resource conflicts between my ::nginx::config and the one in the init.pp of this module (Puppet decided to load the one in init.pp first). When I tried

class { '::nginx::config': } -> class { '::nginx': }

it created a cyclic dependency. Because I am using manifest ordered resources, I can work around the issue by just doing:

class { '::nginx::config': }
class { '::nginx': }

But without manifest ordered resources, there doesn't seem to be a reliable way to force a hiera backed nginx::config class to be loaded before the default nginx::config in the init.pp is loaded.

[mcanevet]

@jfryman this is really annoying because this module is not usable anymore on the latest version of Puppet.

[jfryman]

@mcanevet would you be up to submit a PR?

[mcanevet]

@jfryman I think depending on hiera for a component level module is a bad idea. Maybe you should remove this whole bloc https://github.com/jfryman/puppet-nginx/blob/master/manifests/init.pp#L143-L207. Would you be agree with a PR in that direction?

[jfryman]

@mcanevet I think you are right. I have found my own patterns tend to push any custom hiera related data into a profile-specific setting, and then in the profile is where I map values to where I need them in the class declaration.

Between this issue and the other issues that have come up, something has to give. I know I've said that I'll "fix" this, but man... has #startuplife been brutal. Let me explain the path that I'm trying to nudge this PR toward (which has to be slow and purposeful... lots of consumers of this module)

What I am trying to move this module toward is: #529

• Remove the resources defined type namespace, and move everything to top-level.
• Move all external variables set at Class['nginx'] scope, with defaults at Class['nginx::params'].
• Normalize types of each input.

That's a lot of work wrapped up in this simple thing, but it's all sort of tangled together. If you can help alleviate the immediate pressure along these lines, I would be very much appreciative. I will (hopefully) have some time this 🎄 🎅 to try and take another pass at the larger goal.

[quixoten]

@mcanevet: the latest version of puppet is manifest ordered by default

By default, Puppet applies resources in the order they’re declared in their manifest.

So both:

include nginx::config
include nginx

and

class { '::nginx::config': }
class { '::nginx': }

Should consistently load your (the hiera backed) nginx::config class first. It's working for me at least.

This shouldn't be an issue when the parameters are no longer in both places, but if this needs to be permanently fixed before that happens, it might work to make the variables in the nginx class the defaults of the nginx::config class, i.e.,

class nginx::config (
  $client_body_temp_path = $::nginx::client_body_temp_path,
) {
...
}

Which would push the $::nginx::params::client_body_temp_path into the nginx class and make the deprecation warning check a bit more verbose, i.e.,

if $client_body_temp_path != $::nginx::params::client_body_temp_path or
   ...

It will no longer detect the usage of defaults by the user, but maybe that's a small enough edge case to ignore?

It's a large enough change that, if relying on manifest ordered resources reliable works around the issue, I'm comfortable waiting for the deprecated parameters to be removed from the nginx class. This should allow the nginx class to just include ::nginx::config rather than explicitly declaring it.

[vicinus]

information: the issue initially reported was fixed as a regression in Puppet 4.3.2 (PUP-5592)

[3flex]

@ckaenzig can you confirm fixed on 4.3.2 or higher?

[needforspeed]

Hi @quixoten and @jfryman

I'm facing a similar problem, but I'm using puppet 2.7.19 with jfryman/nginx@0.2.7. How should I setup hieradata?

I tried following, but none of them appear to work.

nginx::config::xx

nginx::params::xx

::nginx::config::xx

::nginx::params::xx

Can you please let me know how to use hieradata in my case?

Thanks in advance!

[3flex]

I'm going to close this as the original issue appears to be a regression in Puppet, fixed in version 4.3.2.

@needforspeed it would be more appropriate to open a new issue for your question since it doesn't directly relate to the original issue with Puppet 4.3.