Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rip back out puppet-module-data #501

Merged
merged 25 commits into from
Nov 22, 2014
Merged

Rip back out puppet-module-data #501

merged 25 commits into from
Nov 22, 2014

Conversation

jfryman
Copy link
Contributor

@jfryman jfryman commented Nov 10, 2014

Well, it was a nice idea in #453, but there have been several reported issues with the update.

  1. Performance hits. This is a bit anecdotal... I've asked for data but have not received any yet. Nonetheless, I have heard from a few folks that introducing puppet-module-data adds extra time to catalog compilation.
  2. Installation difficulty. I field quite a bit of out of band support requests associated with puppet-module-data. It has proven to be hacky enough to cause errors in several edge cases. Because it is not part of Puppet Core, it increases the barrier to entry for a fair number of folks.

I love this pattern, but it need a lot of love, and until it becomes a first class citizen, it needs to be ripped out before we hit 1.0. So, while I hate making a big change and then ripping it out... it's better to do now than before it becomes too unwieldy.

include nginx
class { 'nginx::config':
gzip => false,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nginx::config is already declared in nginx class so this would result in a duplicate declaration error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some logic in another branch that covered this that I didn't 🍒-pick and put here. I've grabbed that.

d57c787

@3flex
Copy link
Contributor

3flex commented Nov 10, 2014

I think this is for the best for now.

Though with this change it looks like if a user just does include nginx then all the undef defaults in the nginx class will be passed to nginx::config, overriding that class's defaults and creating an invalid config?

$_daemon_user = 'http'
}
'Debian': {
$_pid = 'www-data'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be $_daemon_user = 'www-data'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@3flex 3flex mentioned this pull request Nov 13, 2014
@jfryman
Copy link
Contributor Author

jfryman commented Nov 17, 2014

Though with this change it looks like if a user just does include nginx then all the undef defaults in the nginx class will be passed to nginx::config, overriding that class's defaults and creating an invalid config?

This is the good part about the undef here... it actually doesn't pass undef through. So, if it's been declared, it'll pass through. Otherwise, the defaults in nginx::config will take precedent.

$global_owner = $nginx::params::global_owner,
$global_group = $nginx::params::global_group,
$global_mode = $nginx::params::global_mode,
$log_dir = $nginx::params::log_dir,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter in this class should be logdir to match the old name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did consider this... this is the only variable that didn't seem to match the existing style in the rest of code. I'll put another deprecation warning to make it less painful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HA! It actually looks like you did that already. 🙇

$worker_connections = 1024,
$worker_processes = 1,
$worker_rlimit_nofile = 1024,
### END Nginx Configuration ###
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to inherit nginx::params here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good 👀

jfryman added a commit that referenced this pull request Nov 22, 2014
@jfryman jfryman merged commit c9b82e7 into master Nov 22, 2014
@jfryman jfryman deleted the rip-out-module-data branch November 22, 2014 18:53
@jfryman jfryman mentioned this pull request Nov 24, 2014
jfryman added a commit that referenced this pull request Nov 24, 2014
As part of merging #501, default values for config.pp were integers. However,
many of the current logic expects to see strings for many values. This commit
fixes things up while the validation logic can be adjusted to take account
integer.
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Oct 23, 2017
As part of merging voxpupuli#501, default values for config.pp were integers. However,
many of the current logic expects to see strings for many values. This commit
fixes things up while the validation logic can be adjusted to take account
integer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants