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

nixos/phpfpm: module cleanup #65706

Merged
merged 6 commits into from Aug 24, 2019
Merged

nixos/phpfpm: module cleanup #65706

merged 6 commits into from Aug 24, 2019

Conversation

aanderse
Copy link
Member

@aanderse aanderse commented Aug 1, 2019

Motivation for this change

This PR aims to prep us for a smooth transition to running php-fpm services as non root in a future release of NixOS.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Member

@infinisil infinisil left a 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

@infinisil
Copy link
Member

@Ma27
Copy link
Member

Ma27 commented Aug 11, 2019

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.

@aanderse
Copy link
Member Author

aanderse commented Aug 14, 2019

@GrahamcOfBorg test limesurvey mediawiki
@GrahamcOfBorg test nextcloud roundcube
@GrahamcOfBorg test wordpress

@aanderse aanderse marked this pull request as ready for review August 15, 2019 01:34
@nixos-discourse
Copy link

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

@aanderse
Copy link
Member Author

@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 ❤️
@wmertens having you review and approve (or not) this PR would be of value to me.
@globin I hope my comments have swayed your opinion enough for you to get behind this. I would appreciate your feedback either way, of course.
@dasJ I'm not sure how much of a phpfpm expert you are, but I know you've put at least some effort into the module in the past, so if you have any feedback it would be appreciated.

Thanks all.

@aanderse
Copy link
Member Author

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.

Copy link
Contributor

@wmertens wmertens left a 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!

Copy link
Contributor

@etu etu left a 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.

@aanderse
Copy link
Member Author

aanderse commented Aug 24, 2019

@etu Everything should be good to go now. The manual builds again with nix-build nixos/release.nix -A manual --arg supportedSystems '[ "x86_64-linux" ]'. Thanks for taking the time out to review!

Merging now so we have some time before freeze to catch anything that might come up as a result of this.

@grahamc
Copy link
Member

grahamc commented Nov 2, 2019

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?

@aanderse
Copy link
Member Author

aanderse commented Nov 2, 2019

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 user option was required before this change. The difference now is a nixos-rebuild error, as opposed to a runtime error.

https://www.php.net/manual/en/install.fpm.configuration.php

user string
Unix user of FPM processes. This option is mandatory.

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!

@leo60228
Copy link
Member

leo60228 commented Jan 7, 2020

0ce8317 is a pretty major breaking change that isn't in the 19.09 release notes.

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.

None yet

10 participants