Fail defined types if nginx class was not declared before #1070
Conversation
Because the `ngnix::resource:*` types access ::ngnix class parameters the nginx class needs to be declared before calling the defined type. `include ::nginx` inside the defined type is not enough because params are evaluated before include is parsed. Two of the defined types only access params inside. The include way would work there - but for consistency I also added the fail. Closes #983
|
@law @timatooth @triforce you might be interested in reviewing because of your interest in #983 |
vinzent
added
bug
|
probably needs proper tests? :-) |
There was a problem hiding this comment.
This makes sense to me, but I'd like to see input from someone who's been working more actively on the Puppet 4 stuff to just doublecheck that this is a good way to implement it. Maybe ask on Slack or IRC in #voxpupuli or wait for someone to chime in here?
In many other parts of the module's namespace, we're already explicitly making these private, but I think these are intended to be referenced directly.
And yes, tests.
| @@ -63,6 +63,10 @@ | |||
| Optional[Boolean] $proxy_recursive = undef | |||
| ) { | |||
|
|
|||
| if ! defined(Class['nginx']) { | |||
There was a problem hiding this comment.
I think this is fine and probably goes with the style of the module as a whole... that said, I think some people would say that 'unless' is more idiomatic, and probably lines up better with https://docs.puppet.com/puppet/4.9/lang_conditional.html ? I like 'if not' better than 'if !' but IIRC, it may not work in the DSL?
There was a problem hiding this comment.
If or unless is IMHO very opinionated. I for myself prefer if over unless in most situations. But I'll change to unless if requested.
There was a problem hiding this comment.
I mostly just don't like if ! vs if not but I don't have a strong opinion, and it's used all throughout the code, so I don't see any reason to change it.
|
@wyardley this isn't related at all with puppet4. The very same parse order problem exists with puppet since forever. This fail if not defined behaviour here is the very same as puppetlabs/apache uses for apache::vhost |
|
@wyardley I havent checked if any of the defined types are just private-style types. If there sre some it might make sense to make to mark them private in addition. In addition to even ensure with internal use the class gets included before. |
|
this isn't related at all with puppet4 Sorry, I just meant that there might be a different way to test for this with some of the features that are being added now that we're dropping Puppet 3 support. I havent checked if any of the defined types are just private-style types. I think most of these are intended to be called directly via as described in the docs, so something along these lines is probably the right approach. |
|
@vinzent gentle bump |
|
Ran this by @bastelfreak and decided we should merge this in even without tests |
…efined Fail defined types if nginx class was not declared before
…efined Fail defined types if nginx class was not declared before
Because the
ngnix::resource:*types access ::ngnix classparameters the nginx class needs to be declared before calling
the defined type.
include ::nginxinside the defined type is not enough becauseparams are evaluated before include is parsed.
Two of the defined types only access params inside. The include
way would work there - but for consistency I also added the fail.
Closes #983