-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
nixos/postfix: fix default postfix config #34060
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
Conversation
With your PR I think it is impossible to remove default configs. For example the default postfix config for recipient_canonical_maps is
If I want to set it to
which would now merge to
|
@uwap you are right about the merging of the values, which is the intended behaviour, but you should still be able to override the default value with a 'lower' priority e.g.
|
I think the changed behavior should be documented in the release changelog then? cc @fpletz |
@@ -24,7 +24,7 @@ let | |||
then [ (concatStringsSep ", " (map (s: "reject_rbl_client " + s) cfg.dnsBlacklists)) ] | |||
else []; | |||
|
|||
clientRestrictions = concatStringsSep ", " (clientAccess ++ dnsBl); | |||
clientRestrictions = clientAccess ++ dnsBl; |
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 don't really get why you do this change. Could you explain it?
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 think I made a mistake there, as I can't think of why I did that. Thank you 👍
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 module adds commas inconsistently. Commas seem to be optional in postfix's main.cf so we should either add them for all options that are lists or not.
`services.postfix.config` is now correctly merged with the default attrset specified in the module. Some options that are lists in postfix also have to be lists in nix to be merged correctly. Other default options are now set with `mkDefault` so they can be overridden via the module system.
8eb5488
to
9bd7798
Compare
@uwap I consider the previous behaviour a bug because the options would not merge nicely as other options do in the module system. We will document the change in the release notes. |
@uwap Regarding your concern with merging default options: We made sure that we only set list options if another option in If the old behaviour is used and the user sets |
I've added a mention in the release notes and a commit to add commas between list option elements. This makes the output of |
@fpletz Great! Thank you! |
services.postfix.config
is now correctly merged with the default attrsetspecified in the module. Some options that are lists in postfix also
have to be lists in nix to be merged correctly. Other default options are
now set with
mkDefault
so they can be overridden via the module system.Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)/cc @qknight @globin @fpletz