-
-
Notifications
You must be signed in to change notification settings - Fork 15k
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
nixos/radarr,lidarr,readarr,whisparr,prowlarr: add settings option #384052
base: master
Are you sure you want to change the base?
Conversation
1004d6a
to
cf2273f
Compare
cf2273f
to
2126e64
Compare
2126e64
to
6e9264e
Compare
No |
@ambroisie we could. Not sure, because it looked quiet complex to me for inserting just a string. Should I add it? |
I think I'd care mainly if there was a way to set the various client API keys declaratively in Prowlarr, but that's not really doable from what I understand (except maybe if I hacked it in with some custom SQLite invocations...). As it is, it feels like a small inconsistency with Sonarr's module. |
I have no particular knowledge of these services and it seems like there are many other interested reviewers. If you need someone to merge once everything is reviewed and touched up feel free to give me a ping, but until then I will step out. |
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.
With this servarr directory, I feel it makes more sense to move the module files into it so they're all together.
As an added bonus, I'd suggest you could do a fairly simple nixos test which sets the API key and tries to hit the API with curl. I did something very similar in the autobrr PR I did a few days ago.
It is if we script the API! I have a WIP implemention of that in my private config, I can push it tomorrow if you're interested. Something like configarr or recyclarr might work here too. All in all it's about a 40-line python script though, and most of it is just niceties. It's really not complex. However I'd say we really shouldn't block this PR on that. I can make a PR later with that if I can get it working well enough. |
f021959
to
6dc393f
Compare
6dc393f
to
4fc4911
Compare
Great work! Any reason not to turn the |
4fc4911
to
18f1605
Compare
surely we could. please have a look |
f064b55
to
4618d53
Compare
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.
Looks good to me, if you can confirm it works!
I would still suggest the test though, I think it would be a fairly simple thing that would add some confidence to any future changes in these modules.
follow-up: #373576
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-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.