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
Mailman refactor #86177
Mailman refactor #86177
Conversation
d8d4980
to
0b59e15
Compare
@@ -183,7 +205,17 @@ in { | |||
(requirePostfixHash [ "config" "local_recipient_maps" ] "postfix_lmtp") |
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.
Since the mailman module already touches postfix config, maybe we should set this in there as well rather than erroring if it isn't set?
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.
We used to do this, and I changed it for a reason. The user might want to include other things in that list, and the order matters. So we can’t set it automatically.
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.
The order can still be influenced by the user using mkBefore
/mkAfter
/mkOrder
, but I agree that's not pretty and shouldn't be required unconditionally.
a986829
to
5d2179e
Compare
Rebased on current master to reflect mailman updates (cc @alyssais) |
It would be good to see this in. Django 1.11 is no longer supported and with #89723 mailman-web won't build anymore either. |
Note after this is in, django_1_11 should be removed. |
Haven't played with this yet but would like to point you to similar mailing list manager we've packaged recently - https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/mail/sympa.nix https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/mail/sympa.nix#L265 defines Otherwise looking good and thanks for doing this! I've struggled trying to write mailman module when I've started with NixOS. |
This is nonsense! Postorius and Hyperkitty don't even support 1.11 anymore.
- Add serve.enable option, which configures uwsgi and nginx to serve the mailman-web application; - Configure services to log to the journal, where possible. Mailman Core does not provide any options for this, but will now log to /var/log/mailman; - Use a unified python environment for all components, with an extraPackages option to allow use of postgres support and similar; - Configure mailman's postfix module such that it can generate the domain and lmtp maps; - Fix formatting for option examples; - Provide a mailman-web user to run the uwsgi service by default - Refactor Hyperkitty's periodic jobs to reduce repetition in the expressions; - Remove service dependencies not related to functionality included in the module, such as httpd -- these should be configured in user config when used; - Move static files root to /var/lib/mailman-web-static by default. This avoids permission issues when a static file web server attempts to access /var/lib/mailman which is private to mailman. The location can still be changed by setting services.mailman.webSettings.STATIC_ROOT; - Remove the webRoot option, which seems to have been included by accident, being an unsuitable directory for serving via HTTP. - Rename mailman-web.service to mailman-web-setup.service, since it doesn't actually serve mailman-web. There is now a mailman-uwsgi.service if serve.enable is set to true.
5d2179e
to
d5cc8fb
Compare
Pity that you've decided to ignore my suggestion without any response and merge it - would be nice if there is a consistency between similar modules and the change I've suggested only makes stuff easier for end-users. At first I wanted to PR the change I've suggested to your branch and add myself as a maintainer but I'm currently swamped by other things and now even disappointed as new contributors are forced to jump thru hoops until modules are perfect while people with commit access are free to merge things as they wish. |
Sorry, I meant to comment. I'm not sure such an mta option is a good idea, since a mail server setup that can actually deliver mails well takes a lot of extra parts that can't all be configured within a NixOS configuration. To quote the docs I wrote here:
TLS is also a part of this, though here it's shown in the example config. I don't think a module like mailman's should take responsibility for all those things, but at the same time taking responsibility for a small part of them suggests to users that it's "batteries included", which can result in frustrating experiences with it not working out of the box as one may expect if it has a "configure postfix for me" knob. That said, if we can somehow offer a good experience I'm all for adding such an option. Re merging this: I figured that your suggestion to add an mta option isn't a blocker for merging this and can be done after the fact, essentially being a feature addition. |
No hard feelings, thanks for making it right! The idea about It shouldn't attempt to provide full batteries included solution as that is out-of-scope but only the required mailman integration parts of its config. Done this way it should play nicely with existing setups or setups using If the option feels too production-mailserver-suggestive to you we can add a description to it stating this explicitly - running a mailserver is not for everyone and certainly not easy to get right from scratch so it's probably a good idea to note that and e.g. suggest I'll give this a shot later when I'm done with the currently pending stuff. |
So what you're suggesting is to have an option that enables adding the three necessary postfix options (as in #86177 (review)) as opposed to doing it unconditionally or not doing it at all? Yes, I think that makes sense :) |
Motivation for this change
Add uwsgi and nginx functionality to reduce the amount of manual setup needed for mailman, as well as documentation.
Supersedes #84896.
@alyssais I've taken the liberty of putting you in the maintainers list along with myself. Feel free to object :)
TODO: release notes
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)