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

[RFC 0042] NixOS settings options #42

Merged
merged 44 commits into from
Jan 14, 2021
Merged

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Mar 23, 2019

Summary

Part 1: Structural settings instead of stringly extraConfig

NixOS modules often use stringly-typed options like extraConfig to allow specifying extra settings in addition to the default ones. This has multiple disadvantages: The defaults can't be changed, multiple values might not get merged properly, inspection of it is almost impossible because it's an opaque string and more. The first part of this RFC aims to discourage such options and proposes generic settings options instead, which can encode the modules configuration file as a structural Nix value. Here is an example showcasing some advantages:

{
  # Old way
  services.foo.extraConfig = ''
    # Can't be set in multiple files because string concatenation doesn't merge such lists
    listen-ports = 456, 457, 458 
    
    # Can't override this setting because the module hardcodes it
    # bootstrap-ips = 172.22.68.74

    enable-ipv6 = 0
    ${optionalString isServer "check-interval = 3600"}
  '';
  
  # New way
  services.foo.settings = {
    listen-ports = [ 456 457 458 ];
    bootstrap-ips = [ "172.22.68.74" ];
    enable-ipv6 = false;
    check-interval = mkIf isServer 3600;
  };
}

Jump to the detailed design of part 1

Part 2: Balancing module option count

Since with this approach there will be no more hardcoded defaults and composability is not a problem anymore, there is not a big need to have NixOS options for every setting anymore. Traditionally this has lead to huge modules with dozens of options, each of them only for a single field in the configuration. Such modules are problematic because they're hard to write, review and maintain, are generally of lower quality, fill the option listing with noise and more. Additional options aren't without advantages however: They are presented in the NixOS manual and can have better type checking than the equivalent with settings.

The second part of this RFC aims to encourage module authors to strike a balance for the number of additional options such as to not make the module too big, but still provide the most commonly used settings as separate options. Quality is encouraged over quantity: Authors should spend more time on writing documentation, NixOS tests or useful high-level abstractions. This is in contrast to the fiddly labor of copying dozens of options from upstream to NixOS. With a settings option, it's also very easy to add additional options over time if the need arises. In contrast, removing options has always been nigh impossible.

Jump to the detailed design of part 2

Rendered

Write (not-so-)detailed design and drawbacks

Adjust formatting

Expand Additional config options

Add section for configuration types

Add format writers and documentation sections

Adjust additional options example

Add some more links and change name

Move file
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
@timokau
Copy link
Member

timokau commented Mar 23, 2019

I think I like this overall. Documentation is important here. Also I'd like to keep the extraConfig escape hatch.

One (admittedly minor) drawback is that it can feel a little bit uncanny-valley like when the upstream configuration options don't adhere to nix conventions like starting with a lowercase letter (NixOS/nixpkgs#57554 (comment)).

@infinisil
Copy link
Member Author

@timokau

Also I'd like to keep the extraConfig escape hatch.

I'd rather not if possible, because then we have problem of not being able to merge values set in extraConfig with all other values, there are essentially two incompatible ways of specifying configuration then. I didn't think of this earlier (I'll add it to the RFC), but with a config option, you can even do this which alleviates the need for extraConfig (for formats we have a parser for at least):

{
  services.foo.config = builtins.fromJSON ''
    {
      "logDir": "/var/log/foo"
      # ...
    }
  '';
}

If the user really doesn't want to use the modular config, what I suggest instead is to have a configFile option as an escape hatch, which when set, overrides the file generated by the config option. This also has the advantage that the user can just set that option to something stateful if they wish, like "/var/lib/foo/config.json" and manage it outside of NixOS.

@timokau
Copy link
Member

timokau commented Mar 23, 2019

Also I'd like to keep the extraConfig escape hatch.

I'd rather not if possible, because then we have problem of not being able to merge values set in extraConfig with all other values, there are essentially two incompatible ways of specifying configuration then.
[...]
If the user really doesn't want to use the modular config, what I suggest instead is to have a configFile option as an escape hatch

The advantage of extraConfig is that the user can keep a standard format config file around (which can also be used outside of nixos) and just import that with readFile. But nix can still set some options, e.g. when another module needs to do some configuration on this module.

That mostly makes sense for rc-type configs, less for json I guess.

@7c6f434c
Copy link
Member

On the other hand, JSON can actually be imported with fromJSON

Ideally, declarative config formats could be parsed and merged, but for imperative configs indeed extraConfig makes sense.

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for bringing this up! After reading through the RFC I'm confident that the overall idea is a good thing to do, both from a collaborator's and user's perspective 👍

Below are some thoughts and comments from my side.

rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
# Set minimal config to get service working by default
services.foo.config = {
dataPath = "/var/lib/foo";
logLevel = mkDefault "WARN";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just thought a little bit about this and I personally think that one would like to see some kind of mechanism that prints all default values into the summary of config. But IIRC only config options are evaluated when generating the manual, so I'm not even sure if that's possible without too much effort.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would certainly be nice in the future, I feel like it should be possible, by just determining the default by actually evaluating the options value with an empty configuration.nix, instead of just looking at the options default attribute.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea and would be something I'd probably help to implement. How about adding this to "Future work" for now? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is pretty important that the defaults in use should show up in the documentation. Perhaps the mkOption function could be adjusted to allow setting a priority of the default option? Then you could have something like

mkOption {
  default = {
    logLevel = "WARN";
    port = 2546;
  };
  defaultModifier = def: mapAttrs (n: mkOptionDefault) def;
  # …
}

To have user config merged into the option default value unless the user applies mkForce.

I haven't thought this through very carefully, though, so I might have missed some point causing this not to work.

But this may be an easier way to solve this documentation issue instead of attempting to evaluate an "empty" configuration. A difficulty, for example, is that we have to figure out how to actually get the config option to be populated. In the foo module, for example, you would have to know that the enable option should be set to true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like

mkOption {
  default = lib.noPriority (mapAttrs (n: mkOptionDefault) {
    logLevel = "WARN";
    port = 2546;
  });
}

The manual could generate

default = {
  logLevel = <default> "WARN";
  port = <default> 2546;
}

But then there's also the problem with defaults that depend on other options, these can't be in the manual. And then the default declarations would have to be split up in the module itself.

And there's also the problem with serializing nix values from nix itself. We can't properly escape stuff, and multi-line strings won't work, and interpolated values, derivations.. I feel like we almost need some Nix language change to make this work properly.

And then there's also the discussion on which defaults should be seen by the user at all. Does it make sense that logType = "syslog" should be in the manual for the user to see? Probably not. See the Defaults section of this RFC as well, the first type of default specifically shouldn't be seen by the user.

I guess the simplest solution is to just say "A reasonable default base configuration is set in order for the module to work, see for details". We already link to the module files already anyways.

Not entirely sure what to do about this. But then again, having this problem is nothing inherent to this RFC, lots of modules already set defaults in this way invisible to users.

Copy link

@pacien pacien Jun 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any advantage in using mkDefault to override some configuration keys over using apply = recursiveUpdate default in mkOption like here?

      config = mkOption rec {
        type = types.attrs;
        apply = recursiveUpdate default;
        default = {
          ircService = {
            databaseUri = "nedb://${dataDir}/data";
            passwordEncryptionKeyPath = "${dataDir}/passkey.pem";
          };
      };

This has the advantage of already showing the default values in the documentation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this apply way doesn't seem too bad, I don't think it's a good idea because it completely sidesteps the module system, which leads to some unexpected behavior. E.g. in your example if a user sets config.ircService = mkForce {}, this doesn't do anything, because recursiveUpdate doesn't know not to recurse over that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add addDefaultAnnotation = annotation: content: { _type = "annotation:default"; inherit content; }; and let the documentation builder use that for describing the value in the manual?

rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
rfcs/0042-config-option.md Outdated Show resolved Hide resolved
@Mic92 Mic92 added the status: open for nominations Open for shepherding team nominations label Mar 27, 2019
@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/open-for-nomination-rfc-0042-nixos-config-options/2538/1

rfcs/0042-config-option.md Outdated Show resolved Hide resolved
@danbst

This comment has been minimized.

@infinisil

This comment has been minimized.

@dotlambda dotlambda mentioned this pull request Feb 8, 2023
13 tasks
@infinisil infinisil deleted the config-option branch April 6, 2023 14:49
@dotlambda dotlambda mentioned this pull request Oct 15, 2023
12 tasks
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/community-calendar/18589/99

KAction pushed a commit to KAction/rfcs that referenced this pull request Apr 13, 2024
Co-authored-by: Domen Kožar <domen@enlambda.com>
Co-authored-by: Jörg Thalheim <Mic92@users.noreply.github.com>
Co-authored-by: Linus Heckemann <git@sphalerite.org>
@7c6f434c 7c6f434c mentioned this pull request Jul 28, 2024
13 tasks
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.