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

3proxy: init at 0.8.13 #67507

Open
wants to merge 6 commits into
base: master
from

Conversation

@misuzu
Copy link

commented Aug 26, 2019

Motivation for this change

New package

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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
Copy link
Member

left a comment

LGTM!

pkgs/applications/networking/3proxy/default.nix Outdated Show resolved Hide resolved
@volth
volth approved these changes Aug 26, 2019
@balsoft

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

@GrahamcOfBorg build _3proxy

@misuzu misuzu force-pushed the misuzu:package-3proxy branch from d93ef8b to 79f2fb9 Aug 27, 2019
@misuzu misuzu force-pushed the misuzu:package-3proxy branch from 79f2fb9 to bfdcf1e Aug 27, 2019
@mmahut

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

@GrahamcOfBorg build _3proxy

@misuzu

This comment has been minimized.

Copy link
Author

commented Aug 30, 2019

I have a module for 3proxy. Should i push it here or wait for this pull request to by merged?

@misuzu misuzu force-pushed the misuzu:package-3proxy branch from bfdcf1e to 9874ce2 Aug 31, 2019
@misuzu misuzu requested review from mmahut and alexarice Oct 9, 2019
Copy link
Contributor

left a comment

Builds with nix-review

@mmahut

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

@misuzu feel free to include it here, as well as the test for the module.

@GrahamcOfBorg build _3proxy

@mmahut

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

@GrahamcOfBorg test 3proxy

@mmahut mmahut requested a review from aanderse Oct 11, 2019
Copy link
Contributor

left a comment

Impressive first package. You've done a great job here!

Ignore all other 3proxy options and load configuration from this file.
'';
};
users = mkOption {

This comment has been minimized.

Copy link
@aanderse

aanderse Oct 12, 2019

Contributor

I don't like options like this because it promotes bad user behavior and serves as more examples to other nixos developers on why it is ok to store secrets in the nix store (hint: it is not).

You have a usersFile option which covers the required functionality in a relatively simple way. What I'm suggesting is that you remove the users option in favor of usersFile. People who are entirely insistent on storing secrets in the nix store (or just want to quickly test with dummy data) are still free to write out something like:

services._3proxy.usersFile = pkgs.writeText ''
  user aanderse:CL:test
'';

I want to make it very clear this is just my opinion and not required in order to have this PR merged.

This comment has been minimized.

Copy link
@misuzu

misuzu Oct 12, 2019

Author

Removed.

type = types.nullOr types.lines;
default = null;
description = ''
Extra configuration, appended to the 3proxy configuration file.

This comment has been minimized.

Copy link
@aanderse

aanderse Oct 12, 2019

Contributor

Please include link to upstream documentation.

This comment has been minimized.

Copy link
@misuzu

misuzu Oct 12, 2019

Author

Done.

This comment has been minimized.

Copy link
@aanderse

aanderse Oct 12, 2019

Contributor

Please wrap links like so <link xlink:href="https://github.com/z3APA3A/3proxy/wiki/3proxy.cfg"/> for easy clicking.

This comment has been minimized.

Copy link
@misuzu

misuzu Oct 13, 2019

Author

Done.

type = types.nullOr types.str;
default = null;
description = ''
Extra arguments for service.

This comment has been minimized.

Copy link
@aanderse

aanderse Oct 12, 2019

Contributor

As someone who has never used this program I would appreciate an explanation of what you put here, possibly including a link to the relevant section of upstream documentation.

This comment has been minimized.

Copy link
@misuzu

misuzu Oct 12, 2019

Author

Done.

This comment has been minimized.

Copy link
@aanderse

aanderse Oct 12, 2019

Contributor

Please wrap links like so <link xlink:href="https://github.com/z3APA3A/3proxy/wiki/3proxy.cfg"/> for easy clicking.

This comment has been minimized.

Copy link
@misuzu

misuzu Oct 13, 2019

Author

Done.

type = types.nullOr types.lines;
default = null;
description = ''
Extra configuration for service.

This comment has been minimized.

Copy link
@aanderse

aanderse Oct 12, 2019

Contributor

As someone who has never used this program I would appreciate an explanation of what you put here, possibly including a link to the relevant section of upstream documentation.

This comment has been minimized.

Copy link
@misuzu

misuzu Oct 12, 2019

Author

Done.

@misuzu misuzu requested a review from aanderse Oct 12, 2019
Copy link
Contributor

left a comment

Little cleanup

configFile = if cfg.confFile != null then
cfg.confFile
else
(pkgs.writeText "3proxy.conf" ''

This comment has been minimized.

Copy link
@volth

volth Oct 13, 2019

Contributor
-  configFile = if cfg.confFile != null then
-    cfg.confFile
-  else
-    (pkgs.writeText "3proxy.conf" ''
+ services._3proxy.confFile = lib.mkDefaultOption (pkgs.writeText "3proxy.conf" ''

and move it downstairs from the let-block to under config = mkIf cfg.enable {

This comment has been minimized.

Copy link
@misuzu

misuzu Oct 13, 2019

Author

I'm getting error: attribute 'configFile' missing when it's like that.

This comment has been minimized.

Copy link
@volth

This comment has been minimized.

Copy link
@misuzu

misuzu Oct 13, 2019

Author

#67507 (comment)

Yes, i did that, but it's failing anyway.

enable = mkEnableOption "3proxy";
confFile = mkOption {
type = types.nullOr types.path;
default = null;

This comment has been minimized.

Copy link
@volth

volth Oct 13, 2019

Contributor
-     type = types.nullOr types.path;
-     default = null;
+     type = types.path;
serviceConfig = {
DynamicUser = true;
StateDirectory = "3proxy";
ExecStart = "${pkg}/bin/3proxy ${configFile}";

This comment has been minimized.

Copy link
@volth

volth Oct 13, 2019

Contributor
- ExecStart = "${pkg}/bin/3proxy ${configFile}";
+ ExecStart = "${pkg}/bin/3proxy ${cfg.confFile}";
};
bindAddress = mkOption {
type = types.str;
default = "[::]";

This comment has been minimized.

Copy link
@volth

volth Oct 13, 2019

Contributor

I am not sure if [::] will work when IPv6 is disabled, need to check
If not, then default = if config.networking.enableIPv6 then "[::]" else "0.0.0.0"

This comment has been minimized.

Copy link
@misuzu

misuzu Oct 13, 2019

Author

It will. I added networking.enableIPv6 = false; to tests.

};
bindPort = mkOption {
type = types.nullOr types.int;
default = null;

This comment has been minimized.

Copy link
@volth

volth Oct 13, 2019

Contributor
- type = types.nullOr types.int;
- default = null;
+ type = types.int;
+ default = 3128;

This comment has been minimized.

Copy link
@misuzu

misuzu Oct 13, 2019

Author

Every service (except tcppm and udppm) has it's own default port. Take a look at _3proxy.services.type.description

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.