nzbget: Fix script for copying default config file template #51235
Conversation
|
added some comments, we can use |
|
@WhittlesJr can you rebase to master? |
|
@flokli, should I also squash? |
|
fine, I squash & merge'd. |
|
Thanks for the PR and followups! |
|
Thank you! Sorry for the lag and the poor commit management on my end. |
|
No worries :-) |
There was a problem hiding this comment.
@WhittlesJr I know it seems weird to submit a review after this has been merged... but I tried enabling this service which I have never used before and it did not run. I took a look at the module and noticed these problems.
ping @flokli
| sed -i -e 's/MainDir=.*/MainDir=\/tmp/g' $configfile | ||
| } | ||
| echo "Ensuring proper ownership of $datadir (${cfg.user}:${cfg.group})." | ||
| chown -R ${cfg.user}:${cfg.group} $datadir |
There was a problem hiding this comment.
The systemd service has PermissionsStartOnly = "true"; which means that everything executed in the preStart is run as root, including the install call. Since you have taken out the call to chown everything would be owned by root.
| @@ -4,6 +4,7 @@ with lib; | |||
|
|
|||
| let | |||
| cfg = config.services.nzbget; | |||
| dataDir = builtins.dirOf cfg.configFile; | |||
There was a problem hiding this comment.
Now the dataDir option is no longer being used, but remains in the module. If the option is really to be removed you should use mkRemovedOptionModule and possibly mention in release notes.
There was a problem hiding this comment.
@aanderse you're right - Seems I missed some bits here - added my comments, too.
@WhittlesJr when looking at reducing complexity anyhow:
Is it common to run nzbget as a regular user, or is running as a service more common? In that case, we could use DynamicUser=true, and avoid allocating (and asking for) users and groups alltogether.
| @@ -93,6 +89,8 @@ in { | |||
| ''; | |||
|
|
|||
| serviceConfig = { | |||
| StateDirectory = dataDir; | |||
There was a problem hiding this comment.
StateDirectory needs to be passed relative to /var/lib/. I'd just hardcode this to nzbget.
| @@ -41,6 +42,12 @@ in { | |||
| default = "nzbget"; | |||
| description = "Group under which NZBGet runs"; | |||
| }; | |||
|
|
|||
| configFile = mkOption { | |||
There was a problem hiding this comment.
this can probably be dropped too, and just be hardcoded to /var/lib/nzbget/nzbget.conf.
|
@flokli nzbget I would probably suggest keeping the separate nzbget user, adding something like |
|
I don't personally use this service, and I won't have time to apply these changes any time soon. Could one of you start a new PR? |
|
@flokli I'll take this one as I would have eventually got around to it as part of #56265 anyways. Do my Thanks for the heads up @WhittlesJr |
|
@aanderse the comments about |
|
I'm having an issue with the NzbGet service not creating the parent directory before trying to copy the default config. |
|
@Jumblemuddle Can you please post relevant output from journald? Are you running off of master from after #60019 was merged? |
I don't think I have the changes from #60019, since I got the |
Motivation for this change
Fixes #51234
I feel like the options should be more robust and actually allow you to configure the service through nix, but this at least gets it working out of the box.
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)