-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
matrix-appservice-irc: init at 0.17.1 #89106
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
Conversation
''; | ||
|
||
meta = with lib; { | ||
description = "A Matrix <--> IRC bridge"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description = "A Matrix <--> IRC bridge"; | |
description = "Node.js IRC bridge for Matrix"; |
Also please add homepage
|
||
passwordKeyLength = mkOption { | ||
type = ints.unsigned; | ||
description = "Length of te key to encrypt IRC passwords"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
description = "Length of te key to encrypt IRC passwords"; | |
description = "Length of the key to encrypt IRC passwords"; |
Also what kind of encryption scheme is this? If RSA wouldn't 4096 be more appropriate these days?
|
||
needBindingCap = mkOption { | ||
type = bool; | ||
description = "Whether the daemon needs to bind to ports less than 1024 (e.g. for the ident service)"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either
description = "Whether the daemon needs to bind to ports less than 1024 (e.g. for the ident service)"; | |
description = "Whether the daemon needs to bind to ports below 1024 (e.g. for the ident service)"; |
or
description = "Whether the daemon needs to bind to ports less than 1024 (e.g. for the ident service)"; | |
description = "Whether the daemon needs to bind to privileged ports (e.g. for the ident service)"; |
wantedBy = [ "multi-user.target" ]; | ||
|
||
preStart = '' | ||
mask="$(umask)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for this assignment if you're not using it below?
StateDirectory = "matrix-appservice-irc"; | ||
StateDirectoryMode = "755"; | ||
|
||
User = "appservice-matrix-irc"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be reversed?
User = "appservice-matrix-irc"; | |
User = "matrix-appservice-irc"; |
Same for group.
@@ -0,0 +1,27 @@ | |||
# TODO: This has a bunch of duplication with matrix-appservice-slack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly is problematic about this?
The module is also missing in |
One missing optional dependency that sound quite important:
[...]
|
needBindingCap = mkOption { | ||
type = bool; | ||
description = "Whether the daemon needs to bind to ports less than 1024 (e.g. for the ident service)"; | ||
default = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the default depend on the selected port? default = cfg.port < 1024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because the option is unrelated to cfg.port
. It's required when matrix-appservice-irc is supposed to run an identd server on 113/tcp. I wonder if we can get rid of this option however by parsing the configuration.
https://github.com/matrix-org/matrix-appservice-irc/blob/master/config.sample.yaml#L367-L384
Superseded by #95854. |
Motivation for this change
Another attempt at #62864.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)