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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)