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

Mailman refactor #86177

Merged
merged 7 commits into from Jun 19, 2020
Merged

Mailman refactor #86177

merged 7 commits into from Jun 19, 2020

Conversation

lheckemann
Copy link
Member

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
  • 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.

@@ -183,7 +205,17 @@ in {
(requirePostfixHash [ "config" "local_recipient_maps" ] "postfix_lmtp")
Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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.

@ofborg ofborg bot requested a review from alyssais April 28, 2020 10:33
@lheckemann lheckemann marked this pull request as ready for review May 26, 2020 09:01
@lheckemann lheckemann changed the title [WIP] Mailman refactor Mailman refactor May 26, 2020
@lheckemann
Copy link
Member Author

Rebased on current master to reflect mailman updates (cc @alyssais)

@FRidh
Copy link
Member

FRidh commented Jun 11, 2020

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.

@FRidh FRidh added this to the 20.09 milestone Jun 11, 2020
@FRidh
Copy link
Member

FRidh commented Jun 11, 2020

Note after this is in, django_1_11 should be removed.

@sorki
Copy link
Member

sorki commented Jun 18, 2020

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 mta option which would be nice here as well (configures postfix as well if set). Right bellow are structured attrs you could re-use for the TODO.

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.
@lheckemann lheckemann merged commit aea806b into NixOS:master Jun 19, 2020
@lheckemann lheckemann deleted the mailman-upstream branch June 19, 2020 05:54
@sorki
Copy link
Member

sorki commented Jun 19, 2020

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.

@lheckemann
Copy link
Member Author

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:

[…] Note that this setup is not
sufficient to deliver emails to most email providers nor to
avoid spam -- a number of additional measures for authenticating
incoming and outgoing mails, such as SPF, DMARC and DKIM are
necessary, but outside the scope of the Mailman module.

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.

@sorki
Copy link
Member

sorki commented Jun 23, 2020

No hard feelings, thanks for making it right!

The idea about mta is to make it easier to integrate with e.g. already set-up postfix, quickly set one up for trying out mailmain and testing via NixOS tests.

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 nixos-mailserver (we use it in combination with Sympa) which is able to configure all the fluffy stuff you mention for you.

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 nixos-mailserver as a complete mailserver solution.

I'll give this a shot later when I'm done with the currently pending stuff.

@lheckemann
Copy link
Member Author

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 :)

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

4 participants