-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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/jellyfin: add directory options #277220
Conversation
8727c80
to
3456727
Compare
4485454
to
0f1ec08
Compare
ffec5b1
to
d943091
Compare
Welp I struggled with that more than I'd like to have been shown lol. Lesson learnt, test locally always 😵💫 also TIL you can merge suggestions right from within GitHubs website. unrelated question, can you preserve the commit messages of commits which must be squashed? the last few force pushes now feel silly without any immediate context. |
d943091
to
5ca2a74
Compare
somewhere in this process I've managed to break the service.
persists when state is removed and is a problem with another user trying to use this PR. this was working before my first push today. |
removing the workingDir line moves the issue to some permissions faults with the log directory. converting to a draft till I can find the issue here, any insight is greatly appreciated. |
5ca2a74
to
46a2193
Compare
In that latest force-push I removed I also added more links to jellyfin documentation using some markdown after finding out that was available. |
46a2193
to
f413456
Compare
f413456
to
2253842
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/3264 |
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.
PrivateUsers makes other users (not root
or jellyfin
) invisible to this unit. Jellyfin should work with it enabled, so this is a (minor) regression and should be fixed, imo.
2253842
to
1feaec1
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 great
given the PR this replaces had some users preemptively using the changes I'll leave this here as reference for how I've been testing / making use of the changes myself. small update to this, cant recommend using the jellyfin(package) ver from this on unstable since some changes have borked hw transcodes with it, will be fine once merged of course. |
31f884d
to
475b760
Compare
latest force push was simply to ensure exact permissions lineup with whats current. |
I thought using |
Correct, since d36f950 (#237557) which was merged in June 2023: Lines 384 to 387 in 0a25418
Any uses of |
475b760
to
42d8301
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.
LGTM
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.
LGTM
Description of changes
add
dataDir
configDir
cacheDir
andlogDir
optionsideally a lot of config and data dirs would be declared in some way, but the ability to easily define their locations is nicer than nothing.
the
webdir
option explained here is as far as i know not relevant given its in the store.this should replace PR #233617 ( I've tried to address the unresolved comments here )
it should also close #141367 as a result.
removed
with lib;
to address #208242 for this module.add
meta.mainProgram
to the jellyfin server package so thatgetExe
can be used appropriately.remove use of the now pointless
lib.mdDoc
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.