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

dnscrypt-proxy2 service: init #72815

Closed
wants to merge 3 commits into from

Conversation

ryneeverett
Copy link
Contributor

Motivation for this change

Resolves #43298.

I cherry-picked a commit out of #48289 and made a couple changes:

  • Removed package option since that seems to be the reason it wasn't merged.
  • Added a configFile option.
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @waynr @yegortimoshenko

lukateras and others added 2 commits November 5, 2019 03:10
- 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.
@infinisil
Copy link
Member

Do we need to keep around a NixOS module for both versions?

@ryneeverett
Copy link
Contributor Author

@infinisil No, I added a commit removing the original dnscrypt-proxy module. I don't believe it's been functional for some time now.

@yupi2
Copy link

yupi2 commented Dec 29, 2019

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
Copy link
Member

Choose a reason for hiding this comment

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

Add an entry in nixos/modules/rename.nix to mark this module as removed

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that be done via an import in dnscrypt-proxy2.nix?

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 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'.
Copy link
Member

Choose a reason for hiding this comment

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

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;
Copy link
Member

Choose a reason for hiding this comment

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

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";
Copy link
Member

Choose a reason for hiding this comment

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

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 {
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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;
Copy link
Member

Choose a reason for hiding this comment

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

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.

@Atemu
Copy link
Member

Atemu commented Jan 25, 2020

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?

@Atemu
Copy link
Member

Atemu commented Jan 26, 2020

I had to rebase and will have to open a separate PR but what would have been the standard procedure in such cases?

@Atemu Atemu mentioned this pull request Jan 26, 2020
10 tasks
@ryneeverett
Copy link
Contributor Author

@Atemu IMO it's a judgement call based on whether the person making the original PR has a lot invested in the work and/or expertise you want input from. In this case you made the right call as I have neither and am happy to hand it off. Closing in favor of #78543.

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.

Port nixos/dnscrypt-proxy to dnscrypt-proxy2
5 participants