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

Added parameters daemon_group and set_daemon_group #647

Closed
wants to merge 2 commits into from

Conversation

jakm
Copy link

@jakm jakm commented Jun 17, 2015

Allow specify daemon's group as documented at http://nginx.org/en/docs/ngx_core_module.html#user

@3flex
Copy link
Contributor

3flex commented Jun 17, 2015

@jakm thanks for the PR!

I'd ask that you don't add the $set_daemon_group parameter and always populate the daemon_group in the template, there's no need to make it conditional as the defaults you've set will be safe.

@@ -216,11 +218,13 @@
file {$client_body_temp_path:
ensure => directory,
owner => $daemon_user,
group => $daemon_group,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove this. The group is 'root' but with permissions 0644 it will be readable even if you change the daemon user.

@jakm
Copy link
Author

jakm commented Jun 23, 2015

Parameter $set_daemon_group is added because of backward compatibility. If $daemon_group is set to, for example, web-nginx-14 then generated nginx.conf will contain user web-nginx-14; and nginx's processes will have set both user and group to web-nginx-14. Patch without $set_daemon_group parameter would break configurations with non-standard user as described above. Because $daemon_group's default is set to www-data (www, ...).

@wyardley
Copy link
Collaborator

wyardley commented Oct 13, 2016

@jakm: are you able / willing to add tests to this request? Do you need any help determining what / how to test? The commits would also need to be squashed.

At the least, there should be more or less a copy / paste of https://github.com/voxpupuli/puppet-nginx/blob/master/spec/classes/config_spec.rb#L95

https://github.com/voxpupuli/puppet-nginx/blob/master/.github/CONTRIBUTING.md
describes a bit more how to run the tests etc.

As far as the true / false $set_daemon_group param, why not just make it default to undef, and leave out all the params for each OS, that way, people can set it explicitly if they need to override the default. Given that this is a module with a lot of bells and whistles (and that we're trying to move towards leaving more things default when not explicitly set), that seems like it might be a better way to go.

Do you know if nginx always supported the group as well as the user, or is that a change that was added at some point?

@wyardley
Copy link
Collaborator

wyardley commented Feb 2, 2017

No response, going to close this one for now, though I would like to see a reworked version of this at some point as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants