feat: Add Jellystat configuration module#137
Conversation
There was a problem hiding this comment.
First off, thank you so much for the PR. I greatly appreciate it.
I have a lot of comments.
This is my first time looking into Jellystat properly. And I have a couple of concerns. It seems to me like they really expect you to go through a setup wizard and don't have a lot of support for an automated workflow. Based on what I am seeing in the README. I don't see jellyfin integration in there, which implies it is only supported through UI (I hope this is wrong, please let me know if so). Even their JS_USER and JS_PASSWORD variables say "Master Override User in case username or password used during setup is forgotten". Which tells me that they aren't really expecting users to use those variables as their "only" method of configuration.
It seems like they have an API. So, hopefully we can just figure it out that way. Push comes to shove, we can look at the API calls of the setup wizard, and make a systemd service that replicates it. I had to do something similar for Jellyfin.
|
When we get closer to feeling great about the implementation, we will also need to add tests and update the documentation. I will help you figure those out when we get closer to it. |
- Add auto-generated POSTGRES_PASSWORD and JWT_SECRET if not provided - Add jellystat-setup-db service to configure PostgreSQL password - Change jellyfin dependency to jellyfin-setup-wizard.service - Add NS_USER/NS_PASSWORD options for master override - Add configurable timezone option (defaults to time.timeZone) - Fix wait-for-db to use TCP/IP instead of Unix socket - Fix database name default to "jfstat" (matches Jellystat) - Fix description punctuation per PR review - Always set EnvironmentFile when service is enabled
|
Thanks for the detailed review! I've addressed all the feedback. Here's a summary of the changes: All Comments AddressedDescription fixes: All option descriptions now properly end with periods. Timezone option: Added Jellyfin URL: Redesigned to use API Key: Already uses VPN description: Fixed markdown formatting with proper line breaks between sentences. Jellyfin dependency: Changed from Environment variables: Added full support for:
wait-for-db: Fixed to use TCP/IP connection ( Database name: Changed default from "jellystat" to "jfstat" to match Jellystat's Docker default. The commit is |
Replies to Individual CommentsOn ExecStart suggestion: Fixed - now properly uses the package path. On timezone default: Thanks @takac! I've added On mkSecretOption: Already using mkSecretOption for apiKey. Also added mkSecretOption for On jellystat-env service: Already implemented - the env file is created at On description punctuation: All fixed - descriptions now properly end with periods. On VPN description: Fixed markdown formatting with proper blank lines between sentences. On jellyfin URL options: Redesigned to use hostname/port/useSsl/urlBase which auto-construct the URL with sensible defaults. On jellyfin-setup-wizard dependency: Changed from On API key auto-detection: Currently auto-detects from On environment variables (JF_USE_WEBSOCKETS etc): These were actually missing! I've now added all required environment variables including On unnecessary code: Removed the conditional optionalAttrs that was commented out. On NS_USER/NS_PASSWORD: Added On POSTGRES_PASSWORD: Added auto-generated password support and |
kiriwalawren
left a comment
There was a problem hiding this comment.
This is coming along nicely. I can't thank you enough for this.
I have a few more comments, though.
|
I am closing this PR because there is no way to override the listen port. There is an open PR that has been open for over a year for this issue. CyferShepard/Jellystat#346 However, the maintainer announced a rewrite of the project. And that jellystat is entering maintenance mode. So, it seems unlikely that this will be fixed. |
Summary
Resolves #61
Adds Jellystat statistics dashboard configuration module to nixflix.
Features
nixflix.jellystat.enable = trueUsage Example
Files Changed
modules/default.nix- Added jellystat importmodules/globals.nix- Added jellystat UID/GIDmodules/jellystat/default.nix- New module implementation