Refactor params class to use hash, fixes #62 #88
Conversation
| nginx::resource::vhost { 'rack.puppetlabs.com': | ||
| ensure => present, | ||
| proxy => 'http://puppet_rack_app', | ||
| } |
There was a problem hiding this comment.
Please use puppet-lint to check the new code.
|
What about backwards compatibility? |
|
+1 This is a big commit and it needs backwards compatibility, but I really like where this is going. |
|
@abraham1901 I think this change by definition can't be backwards compatible |
|
I'm pretty sure this is going to break all backwards compatibility, but it needs to happen. The current plan that I have right now is to tag the current rev to |
| File { | ||
| owner => 'root', | ||
| group => 'root', | ||
| mode => '0644', | ||
| } | ||
|
|
||
| file { $nginx::params::nx_conf_dir: | ||
| # some of these are accessed from other manifests/templates | ||
| $conf_dir = $options['conf_dir'] |
There was a problem hiding this comment.
I think it would be cleaner to use the merge() function here as opposed to setting several instance variables to hold this data. The pattern that I'm using here in other modules that I was planning on using in this module is:
class nginx::config(
$options = {},
) {
$config = merge($github::params::defaults, $options)
...
}
and then use the hash to reference various variables in the hash where needed in the manifest itself.
There was a problem hiding this comment.
Ah. NM. I see this pattern in https://github.com/jfryman/puppet-nginx/pull/88/files#L3R38. :)
|
@Jonanin This is ✨. Thanks for taking this on. I made a few comments in code, and want to get your input, but this is absolutely on the right track. |
|
I'm also toying with adding @Jonanin care to take a pass at this? If not, I'll start adding them late next week as my schedule opens up. |
|
Here are the fixes people have asked for. Note: that was supposed to be "Rename $config_opts -> $config". Didn't escape on the shell! |
|
@jfryman I should have some time tomorrow, i'll take a look at rspec-puppet. |
|
👍 for the solution to the never-ending-class-params problem. |
|
/cc 05ec889 |
|
I've pushed a commit that fixes all conflicts with changes in master since the initial pull request. I'm pretty busy and thus don't have time to look at implementing rspec in addition. Is there anything else that needs to be done for this to be merged? |
|
Way old. Thanks for taking a pass at this... going to revisit when 🕐 allows. |
Hi, this is my stab at fixing #62 by adding a hash to contain params as you suggested in #79.
Some other goodies in this PR:
🍔