I think we need the config directory option as well here so that an application can add additional config without having to change the unit.

Ideally the switch-to-configuration.pl script will automatically handle the for you...

ping @flokli to discuss this over winter break.

I think what I would like to see here is a settingsFiles option that is a list of strings or paths. This allows ad-hoc configuration when enabling the service.

This is the only request that I haven't had time to implement/test yet.

Would you mind giving a shot at it @cpcloud, doing it as in your consul PR?

Even though this will materialize as /var/lib/nomad, nomad does not set data_dir by default so I think the unit should set data_dir in cfg.settings to "/var/lib/nomad".

Done.

A question though, @cpcloud:

Is what you do in https://github.com/cpcloud/nixpkgs/blob/f84c234a4f328b5c181cb9c955798cdfb5f57240/nixos/modules/services/networking/consul.nix#L7-L9 better:

  settings = {
    data_dir = "/var/lib/consul";
  } // cfg.settings;
in
      settings = mkOption {
        type = format.type;
        default = { };

or what I did with

      settings = mkOption {
        type = format.type;
        default = {
          data_dir = "/var/lib/nomad";
        };

? Does one have benefits vs the other? The let-bout one seems to have the drawback of not being obvious from documentation.

That is a good question. I think that what you did is probably more obvious from a documentation perspective.

You should probably set it this way:

config = mkIf cfg.enable {
  services.nomad.settings = {
    data_dir = "/var/lib/nomad";
  };
};

@nh2 if you do that then data_dir will be overridden if the user puts anything in settings. By setting it the way I have mentioned it gives the sysadmin an escape hatch where they can set services.nomad.settings.data_dir = lib.mkForce "/something/else"; if they absolutely need to.

I think what I would like to see here is a settingsFiles option that is a list of strings or paths. This allows ad-hoc configuration when enabling the service.

I think there's a slight chicken-egg problem here. If user configures data_dir = "/my/nomad/data"; we'd still create StateDirectory not at that location. What should we do with that? Only set StateDirectory if the dirname of data_dir is /var/lib?

My preferred way is to only provision the data/state directory if the user hasn't "customized" it. In this example there is a dataDir option, but the same idea can apply here:

serviceConfig = mkMerge [
  { # standard options
  }
  (mkIf (cfg.settings.data_dir == "/var/lib/nomad") { StateDirectory = "nomad"; })
];

I take special care to explain to the sysadmin their responsibilities, if any:

The directory or NFS/SMB network share where MPD reads music from. If left
as the default value this directory will automatically be created before
the MPD server starts, otherwise the sysadmin is responsible for ensuring
the directory exists with appropriate ownership and permissions.

Why? See the motivation section of the mentioned PR.

I think you can use lib.optionalAttrs here:

lib.optionalAttrs cfg.enableDocker { SupplementaryGroups = "docker"; }

Or

serviceConfig = mkMerge [
  { DynamicUser = cfg.dropPrivileges;
    ...
  }
  (mkIf cfg.enableDocker { SupplementaryGrousp = "docker"; })
];

Also, isn't SupplementaryGroups a list (conceptually)?

Does data_dir need to be configurable? If so, then we should make StateDirectory match, and not set StateDirectory in cases where its dirname isn't /var/lib

See my previous comment.

Additional to this you should add an assertion corresponding to dropPrivileges - the only possibility for data_dir is under /var/lib if DynamicUser is used.

I'm not even sure that having this open is that useful. It seems to give the illusion that a user only need set this to true to allow nomad to launch containers, and that isn't the case. You have to enable to docker task driver plugin.

I think we either remove this option altogether, or we have to add a client.enabled = true option (and then probably a server.enabled = true option) and then we have to enable the plugin.

Unfortunately, that still probably won't be enough. Users will want to be able to launch containers using images from a private registry, in which case we'd basically have to replicate the nomad client config structure here. Of course, when people see that option they might start expecting that other config options can be set in the same way and before long we've replicated the entire nomad client and server configuration structs here!

I propose that we leave this option out to avoid confusion.

It seems to give the illusion that a user only need set this to true to allow nomad to launch containers, and that isn't the case. You have to enable to docker task driver plugin.

@cpcloud For me it worked out of the box, I didn't have to enable a task driver plugin (as stated in #105739 (comment) with the config shown in there).

Ah interesting! Ok then, I guess I'm wrong here. I agree then that we should keep this option.

@lovesegfault @aanderse @cpcloud Given this, what's your opinion on the default?

Mine is that we should default to true because docker is the most common way to use Nomad, and one can only play through the official tutorial when it's enabled.

@nh2 I agree with you. enableDocker = true seems like the right default.