slskd: init module #233648
slskd: init module #233648
Conversation
github-actions
bot
added
6.topic: nixos
|
ping @turlando @SuperSandro2000 @NickCao who already worked on slskd :) |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/2262 |
ofborg
bot
added
10.rebuild-darwin: 0
ppom0
force-pushed
the
slskd-module
branch
2 times, most recently
from
ce93d8d to
29919f8
Compare
|
please fix the manual build erros |
|
fixed! |
|
damn 😍 |
| }; | ||
|
|
||
| # Reverse proxy configuration | ||
| services.nginx.enable = true; |
There was a problem hiding this comment.
The nginx configuration here seems very problematic: even if services.slskd.nginx.enable is false, the vhost will be created and an ACME cert attempted! These vhost settings should probably be mkDefault or maybe mkForce so that users can override them, and ACME support should definitely be an option disabled by default.
As is I cannot enable this module without unintentionally generating a new unwanted certificate for a vhost I am not using.
There was a problem hiding this comment.
dolibarr has a nice way of handling nginx options that could probably fit here. values like forceSSL and enableACME can also be set to true by default, similar to the current implementation - just with a better way of changing these options
There was a problem hiding this comment.
Oh yeah, that's problematic indeed, didn't think of it. I'll look for a fix, thanks for linking.
| wantedBy = [ "multi-user.target" ]; | ||
| serviceConfig = { | ||
| Type = "simple"; | ||
| User = "slskd"; |
There was a problem hiding this comment.
Having configurable user/group support would be helpful for managing permissions. A good example (IMO) is netadata: https://github.com/NixOS/nixpkgs/blob/nixos-unstable/nixos/modules/services/monitoring/netdata.nix#L344-L353
There was a problem hiding this comment.
I didn't implement this, because I figured someone who'd need it would do it.
Anyway, this netdata's way of doing is very straight forward, we can include this in the next PR.
| StateDirectory = "slskd"; | ||
| ExecStart = "${cfg.package}/bin/slskd --app-dir /var/lib/slskd --config ${configurationYaml}"; | ||
| Restart = "on-failure"; | ||
| ReadOnlyPaths = map (d: builtins.elemAt (builtins.split "[^/]*(/.+)" d) 1) cfg.settings.shares.directories; |
There was a problem hiding this comment.
Missing ReadWritePaths for downloads and incomplete folders, e.g.:
ReadWritePaths =
(lib.optional (cfg.settings.directories.incomplete != null) cfg.settings.directories.incomplete) ++
(lib.optional (cfg.settings.directories.downloads != null) cfg.settings.directories.downloads);
| }; | ||
| }; | ||
|
|
||
| web = { |
There was a problem hiding this comment.
Missing frontend authentication options? (authentication.username and authentication.password)
They can be manually entered by the users but really should be added.
There was a problem hiding this comment.
As it's considered bad practice to save credentials into the nix store, the module provides an environmentFile option for secrets.
Can be for example
SLSKD_USERNAME=foo
SLSKD_PASSWORD=barI'd say if they are added, that would mainly be to document what to put in the environmentFile instead. what do you think?
| description = lib.mdDoc '' | ||
| Configuration for slskd, see | ||
| [available options](https://github.com/slskd/slskd/blob/master/docs/config.md) | ||
| `APP_DIR` is set to /var/lib/slskd, where default download & incomplete directories, |
There was a problem hiding this comment.
Is there a reason the application directory isn't configurable?
There was a problem hiding this comment.
As for slskd user, I preferred to do things incrementally, and let someone with particular needs like you make a new PR. I encourage you to do so :)
On my setup, which serves a lot of files, APP_DIR/data weights ~250 MB. I didn't think of which use cases would need this somewhere else, so I let this not configurable. However I encourage you to add more flexibility to the module.
I don't have time to address your issues in the following days. I will may be address them in ~10 days, but I'll welcome a PR instead.
There was a problem hiding this comment.
That's alright, it's not a particular issue for me. I just wondered why it was referred like a variable when it wasn't (yet). I might make a PR for these fixes this week, it was just easiest for me to share my thoughts here beforehand.
There was a problem hiding this comment.
Cool. Oh, right, APP_DIR is an environment variable read by slskd, but it's hardcoded, not "variabilized", in the nix module.
It's nice to express your thoughts before making a PR, thanks. If you PR and want to add yourself to the maintainers list, you're welcome to do so.
Description of changes
Add module for the slskd web-app. Following of #227059
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)