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
nixos/phpfpm: module cleanup #65706
nixos/phpfpm: module cleanup #65706
Conversation
5c7fcd6
to
e215418
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also linking to #65728 and NixOS/rfcs#42
The change itself seems fine to me, however I don't consider myself sufficiently experienced with phpfpm configurations (and IIRC I didn't do too much with that in nixpkgs), so I'm removing the request for my review here. |
@GrahamcOfBorg test limesurvey mediawiki |
This pull request has been mentioned on Nix community. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/44 |
@etu I know you're on vacation so I'm not expecting much... but if you do get a moment to review this I would appreciate it ❤️ Thanks all. |
With the feature freeze coming up the need to merge this is rising. I'm hoping for some positive feedback in the next week, but if this is going to be merged it needs to happen before feature freeze. That being said if there are no more requests for change I'll be merging this within the next few days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the comment I made I'm very happy with these changes overall.
My comment is important though since the manual doesn't build without it.
@etu Everything should be good to go now. The manual builds again with Merging now so we have some time before freeze to catch anything that might come up as a result of this. |
I think this probably needed release notes, since the user and group options are required now. I wonder what I should set these options to? |
The https://www.php.net/manual/en/install.fpm.configuration.php
I agree that more documentation is better. There are more changes coming to this module in the near future. When those changes occur I will revisit the existing documentation for this module. Thanks @grahamc! |
0ce8317 is a pretty major breaking change that isn't in the 19.09 release notes. |
Motivation for this change
This PR aims to prep us for a smooth transition to running
php-fpm
services as nonroot
in a future release of NixOS.Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)