nixos/wordpress: nginx support

[eonpatapon]

Motivation for this change

To be able to combine worpress with other modules that are using nginx.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• 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 nixpkgs-review --run "nixpkgs-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.

[c0deaddict]

Nice work! I'm currently running my own patched version of wordpress on Nginx (https://github.com/c0deaddict/nur-packages/blob/master/modules/wordpress/default.nix) Your changes look much better 👍 I especially like the switch for httpd or nginx. Would be nice to have this included in nixos-unstable.

[aanderse]

Does anyone use the httpd version of this module?

[mohe2015]

No, but if this should become a breaking change I would love to get some of #96910 in there (I can work on that if this pull request will become nginx support only but I should probably do this before it gets merged)

[eonpatapon]

Hi thanks for the interest, it's been a long time.

The one thing that bothered me is how to handle the migration to theses new options. It doesn't seem possible to do it without breakage :/

[expipiplus1]

Is it possible for them to coexist in nixpkgs, once that's done then the httpshttpd version can be deprecated if necessary.

[aanderse]

@FPtje was using this module with httpd at one point. Any input @FPtje?

[expipiplus1]

Fwiw I set this up with nginx and it "just worked"

…

On Tue, Apr 13, 2021, 9:13 AM Aaron Andersen ***@***.***> wrote: @FPtje <https://github.com/FPtje> was using this module with httpd <#62175 (comment)> at one point. Any input @FPtje <https://github.com/FPtje>? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#84446 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGRJXCR4SCZDSZOC77Y4VLTIOLDZANCNFSM4MCA4RTA> .

[eonpatapon]

When I say it will break existing configuration it's because originally the module options are:

services.wordpress = {
   my-site = { ... };
   ...
};

With theses changes the interface is now:

services.wordpress = {
   webserver = "httpd";
   sites = {
      my-site = { ... };
      ...
   };
};

[mohe2015]

@eonpatapon This would be really stupid but you could try to put the webserver option inside each site (so it would be per site). But I don't know if that's worth it and also people would probably forget to update it. But then we could default it to nginx on a specific stateVersion if we want to.

[aanderse]

I'm sorry, I forgot this actually left httpd support around... I thought it replaced, not added.

[expipiplus1]

It should be possible to watch for additional attrs in the config and warn that next version these will not work and will have to go in the "sites" attr.

…

On Tue, Apr 13, 2021, 9:27 PM Aaron Andersen ***@***.***> wrote: I'm sorry, I forgot this actually left httpd support around... I thought it replaced, not added. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#84446 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGRJXC7UA43GE5CCXXFRRLTIRBEXANCNFSM4MCA4RTA> .

[eonpatapon]

@expipiplus1 do you have something in mind where this has already been done ?

[mohe2015]

I just thought a bit about my wordpress improvements PR (#96910) and I think the following change may be benefical to do in a separate commit:
229d6a5?w=1
(it would also need to be adapted for nginx and the documentation would need to be updated).

This would be a breaking change if somebody relies on accessing the attributes directly. But as there is a virtualHost attribute currently I think this is unlikely. Still this would need to be discussed before.

Another idea would be to just fix the virtualHost attribute to work with nginx also (or add an attribute ${webserver}.virtualHost) so we could change this in the future transparently. This may be easier to get merged rather soonish than the other idea.

[mohe2015]

The idea would be something like mohe2015@84dd0e1

What do you think?

[mohe2015]

@eonpatapon friendly ping

[eonpatapon]

@mohe2015 do you mean importing the commit you mention in this PR ?

[mohe2015]

@mohe2015 do you mean importing the commit you mention in this PR ?

Yeah but first what you think about the change (also adding the virtualHost attribute for nginx). I would like to have it to be able to change the nginx service name later (in my other PR) which may be needed but I'm not sure. I think this just hides the implementation details which also may be beneficial and makes usage a little bit better IMO

[eonpatapon]

@mohe2015 About the virtualHost attribute for nginx, I'm not sure this is a common pattern. Usually high level modules make use of low level modules and if you want to do some more configuration on the low level module you can do it directly using it's interface.

For the PR I will have a look and comment there

[mohe2015]

@mohe2015 About the virtualHost attribute for nginx, I'm not sure this is a common pattern. Usually high level modules make use of low level modules and if you want to do some more configuration on the low level module you can do it directly using it's interface.

I would claim that changing things like enabling acme or ssl are pretty common.
Another option I would be fine with would be to prepend "wordpress-" or "wordpress-nginx-" or so to the service name.

For the PR I will have a look and comment there

I'm not sure if it would really break but I assume if:
you have a wordpress service:
a.example.org
And you want to move a.example.org to b.example.org and move another webservice to a.example.org (with nginx service name "a.example.org")
Then the service names would clash and this would probably break stuff.
The assumption is that you can't change the wordpress service name as then the directory would change. So either I think the nginx service name should be based on the actual current hostname or it would probably need a prefix.

[nlewo]

It would be nice to move forward on this PR which is in a pretty good shape:

• Several people like the idea and its implementation,
• Nobody is complaining,
• Backward compatibility is supported,
• A warning is emitted explaining how to migrate to the new interface,
• Tests are passing.

@eonpatapon could you add a release note mentionning the wordpress implementation change and explaining the backward compatibility will be removed for 22.05.
You should also rebase it on master to check tests are still green.

@mohe2015 I think the current implementation of this PR is an improvement and there is a consensus on it. So, we could move forward on it and continue to discuss on other improvements in some followups PRs.

@AndersonTorres @aanderse @mmilata as previous wordpress module contributors, do you see any issue with these changes? Ideally, do you approve it? :)

I would then propose to wait for 1 week more (once my requested changes are applied) and if nobody complains, merge it.
What do you think?

[eonpatapon]

@nlewo thanks for the review! I've rebased and added some release notes

[nlewo]

@eonpatapon thx!

So, if nobody complains, i will merge it on Friday 16/07.

[AndersonTorres]

Wordpress? Me? Strange, I do not remember that.

The latest thing I have relinquished was XFCE small tools.
Anyway, good.

[nlewo]

@AndersonTorres arf, excuse me! I actually wanted to ping @aanderse.

[aanderse]

@nlewo i only ever touched wordpress code in an effort to cleanup apache-httpd, so I'm not much of an interested party.

[nlewo]

@GrahamcOfBorg test wordpress

[nlewo]

Thanks @eonpatapon !