postfix: Make config an attribute set #20218

Open
wants to merge 1 commit into
from

Projects

None yet

5 participants

@uwap
Contributor
uwap commented Nov 6, 2016
Motivation for this change

An attribute set allows us better modifying of config options than a string. So we basically want to change the config type from lines to attrs. This does also integrate the postfix module nicely into nix.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot

@uwap, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @avnik and @peti to be potential reviewers.

@peti
Member
peti commented Nov 6, 2016

I don't see what problem this PR solves. Can you give a practical example of a task that you could do after this PR has been applied but that wasn't possible before?

@@ -354,6 +328,18 @@ in
";
};
+ config = mkOption {
+ type = with types; attrsOf (either bool (either str (listOf str)));
@ericsagnes
ericsagnes Nov 7, 2016 Contributor

This feel like a little wild type, types.attrs might be enough and could provide more appropriate declaration merging than either.

The approach used in the influxdb module for a similar setting, defining a configuration attribute set and update with an attrs option, might interest you as it can be a good trade-off of verbosity and complexity.
Or there is the possibility to use a submodule to enforce the type checking, but at the cost of verbosity.

@uwap
uwap Nov 8, 2016 Contributor

I think this option is a good way to have some advantages from both sides. If I'd use attrs, what would { test = { foo = bar; }; } be? The postfix config has flags: "yes" and "no", lists: comma seperated strings and usual strings. This type allows a good abstraction for it.

@uwap
Contributor
uwap commented Nov 8, 2016

Before, when you had multiple modules adding a certain option to the config it put it in multiple times and used the last one. This is suboptimal. With this it will only put the option once in the main.cf. Also if you have a setting that accepts a list and modules want to add just one item to that setting they would override each other.
My solution is a general abstraction not requiring special implementations for each setting to deal with these problems.

@peti
Member
peti commented Nov 9, 2016

Before, when you had multiple modules adding a certain option to the config it put it in multiple times and used the last one.

Can you please share a concrete example of how this could happen? I understand it's a theoretical possibility, but I don't see that happening in practice and I don't quite see how the attribute set approach helps avoiding that issue.

@uwap
Contributor
uwap commented Nov 15, 2016

@peti: When you have two modules that configure a map for local_recipient_maps, module A sets services.postfix.extraConfig = "local_recipient_maps = $aliasmaps"; and module B sets services.postfix.extraConfig = "local_recipient_maps = proxy:unix:passwd.byname" then the setting will be the one that has been set as last option.

With attrsets module B could just say services.postfix.config.local_recipient_maps = ["proxy:unix:passwd.byname"]; and that would merge those two lists to one.

@peti
Member
peti commented Dec 6, 2016

IMHO, this is not a very convincing use case. If there are two modules that both modify Postfix's configuration in a way that could potentially conflict, then these modules need to to cooperate with each other, i.e. there needs to be a place that considers the options declared in those two modules and merges them into a proper configuration. This is a process that needs to be thought about and implemented, i.e. it doesn't come for free regardless of whether you represent main.conf as a string or as a dictionary.

In your examle, you say:

you have two modules that configure a map for local_recipient_maps, module A sets services.postfix.extraConfig = "local_recipient_maps = $aliasmaps"; and module B sets services.postfix.extraConfig = "local_recipient_maps = proxy:unix:passwd.byname" then the setting will be the one that has been set as last option.

But NixOS modules don't work that way. A NixOS module that needs to affect Postfix doesn't set things directly in main.conf. Rather, it declares a set of config options, which will then be mapped to an implementation at a separate location, presumably the Postfix module.

Personally, I am not in favor of this PR because I don't see what problem it solves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment