nixos/sonarr: add settings option #373576
Conversation
github-actions
bot
added
6.topic: nixos
Stunkymonkey
force-pushed
the
exportarr-sonarr-test
branch
from
43fec13 to
6a0cd16
Compare
github-actions
bot
added
10.rebuild-darwin: 0
I wondered what "sonaar" is, then I realized it's a typo :-) |
|
Commit |
Stunkymonkey
changed the title
| settings = lib.mkOption { | ||
| inherit (settingsFormat) type; | ||
| description = "An attribute set containing Sonarr configuration settings."; | ||
| default = { }; |
There was a problem hiding this comment.
AFAICT, changes done in the web UI is still allowed, but will be reverted on the next service start. I think that should be noted in the description field. How about
An attribute set containing Sonarr configuration written to the config.xml file, which will be applied on every service start. IOW, it overwrites imperative changes done in web UI.
I think config.xml covers roughly the stuff in Settings -> General -- should we mention it?
By reading the generated documentation, it seems the default settings are empty. But they're not. Can we have the actual default settings in the option default value?
There was a problem hiding this comment.
so overall you only suggest an documentation change? Or can we only write, when the config changes?
There was a problem hiding this comment.
I suggest to update the description and wonder if we can put the default values right there in default = ....
There was a problem hiding this comment.
I'm not sure what to do about the "this affects some of the applications config" issue -- I was just thinking out loud.
| let | ||
| # add empty ApiKey so it can be replaced afterwards | ||
| configContent = | ||
| settingsCombined // (lib.optionalAttrs (cfg.apiKeyFile != "") { ApiKey = "#APIKEY#"; }); |
There was a problem hiding this comment.
I think the module should assert that cfg.settingsdoes not contain ApiKey due to it being a secret in the nix store.
There was a problem hiding this comment.
added assertions
Stunkymonkey
force-pushed
the
exportarr-sonarr-test
branch
from
6a0cd16 to
7617721
Compare
| settings = lib.mkOption { | ||
| inherit (settingsFormat) type; | ||
| description = "An attribute set containing Sonarr configuration settings."; | ||
| default = { }; |
There was a problem hiding this comment.
| default = { }; | |
| default = settingsDefault; |
| @@ -44,6 +83,15 @@ in | |||
| }; | |||
|
|
|||
| config = lib.mkIf cfg.enable { | |||
| assertions = [ | |||
| { | |||
| assertion = cfg.settings ? ApiKey -> false; | |||
There was a problem hiding this comment.
| assertion = cfg.settings ? ApiKey -> false; | |
| assertion = !(builtins.hasAttr "ApiKey" cfg.settings); |
| { | ||
| assertion = cfg.settings ? ApiKey -> false; | ||
| message = '' | ||
| The `services.sonarr.settings` attribute set must not contain `ApiKey`, as it is a secret and cannot be securely stored in the Nix store. |
There was a problem hiding this comment.
This should point to apiKeyFile in the message, how about:
| The `services.sonarr.settings` attribute set must not contain `ApiKey`, as it is a secret and cannot be securely stored in the Nix store. | |
| The `services.sonarr.settings` attribute set must not contain `ApiKey`, you should instead use `services.sonarr.apiKeyFile` to avoid storing secrets in the Nix store. |
with the new
formats.xmlwe can do great things. it was prepare some time ago.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.