dnscrypt-proxy2 service: init #72815
Conversation
- Remove package option, which seems to have been the only blocker to merging NixOS#48289. - Add configFile option as an alternative to translating toml/nix.
ofborg
bot
added
6.topic: nixos
|
Do we need to keep around a NixOS module for both versions? |
|
@infinisil No, I added a commit removing the original dnscrypt-proxy module. I don't believe it's been functional for some time now. |
|
I hate to be that guy, but can this be reviewed and/or merged? |
| @@ -585,7 +585,7 @@ | |||
| ./services/networking/dhcpd.nix | |||
| ./services/networking/dnscache.nix | |||
| ./services/networking/dnschain.nix | |||
| ./services/networking/dnscrypt-proxy.nix | |||
There was a problem hiding this comment.
Add an entry in nixos/modules/rename.nix to mark this module as removed
There was a problem hiding this comment.
Shouldn't that be done via an import in dnscrypt-proxy2.nix?
There was a problem hiding this comment.
I think it would be more useful in rename.nix, because since #76857 such imports can be removed recursively, meaning if it were defined in dnscrypt-proxy2.nix and you'd disable that module, the module deprecation warning would also be disabled, which doesn't make much sense.
| message = '' | ||
| Please specify either | ||
| 'services.dnscrypt-proxy2.config' or | ||
| 'services.dnscrypt-proxy2.configFile'. |
There was a problem hiding this comment.
I don't think such assertions are a good idea, because if a module sets the config to something non-{}, there's no way for the user to set the config file without disabling/changing that other module directly. I think a better approach is to do
{
config = mkIf cfg.enable {
services.dnscrypt-proxy2.configFile = mkDefault toml;
# ...
};
}This means it will use the generated file from the config option by default, but it lets the user override it by setting configFile directly. A comment explaining that in configFile would be nice then
| ''; | ||
| example = literalExample "/etc/dnscrypt-proxy/dnscrypt-proxy.toml"; | ||
| type = types.nullOr types.path; | ||
| default = null; |
There was a problem hiding this comment.
Then there's also no need to allow null in the type
| description = '' | ||
| Path to TOML config file. See: https://git.io/fxBne | ||
| ''; | ||
| example = literalExample "/etc/dnscrypt-proxy/dnscrypt-proxy.toml"; |
There was a problem hiding this comment.
This shouldn't use literalExample, since you'll want to have this as a string, not a Nix path (which get copied to the store on build time)
| options.services.dnscrypt-proxy2 = { | ||
| enable = mkEnableOption "dnscrypt-proxy2"; | ||
|
|
||
| config = mkOption { |
There was a problem hiding this comment.
This option implements the approach described in NixOS/rfcs#42, and as such it should be renamed to settings instead.
| config = mkOption { | ||
| description = '' | ||
| Attrset that is converted and passed as TOML config file. | ||
| For available params, see: https://git.io/fxBne |
There was a problem hiding this comment.
Use the docbook link syntax <link xlink:href="https://example.com"/>. Also I don't think the url needs to be shortened, a slightly longer link isn't a problem, and lets the user know where they're going
| }; | ||
| } | ||
| ''; | ||
| type = types.attrs; |
There was a problem hiding this comment.
Note: Once #75584 is merged, the type and generator function from there can be used for this, which will then allow proper merging to happen.
|
I have fixed most of the things mentioned in @infinisil's review here, should I create a PR in @ryneeverett's fork or should I create a separate PR in nixpkgs? |
|
I had to rebase and will have to open a separate PR but what would have been the standard procedure in such cases? |
Motivation for this change
Resolves #43298.
I cherry-picked a commit out of #48289 and made a couple changes:
packageoption since that seems to be the reason it wasn't merged.configFileoption.Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)Notify maintainers
cc @waynr @yegortimoshenko