Skip to content
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 data dir options #233617

Closed
wants to merge 1 commit into from
Closed

nixos/jellyfin: add data dir options #233617

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented May 23, 2023

Description of changes

add dataDir and cacheDir as options to the module for jellyfin
dataDir is useful for jellyfin as theres no current way to manage jellyfin config with nix, the management of its state should be easy.

this should close #141367

cacheDir is included because it was mentioned in the relavent issue.
they both default to existing behaviour so this should not impact current users.

systemd tmpfile rules are used to ensure the paths exist with the right permissions so the previously related entries in the service config are removed

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Copy link
Member

@tomodachi94 tomodachi94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a cursory glance, this looks fine. However, I'm unable to test this.

Tentatively approving, this looks like a simple change.

Comment on lines +38 to +46
Jellyfin data directory.
'';
};

cacheDir = mkOption {
type = types.path;
default = "/var/cache/jellyfin";
description = lib.mdDoc ''
Jellyfin cache data directory.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The service name is duplicated with the path and the description is rather none descriptive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to keep these explicitly set here with the hardcoded 'jellyfin' in the name. We don't want the defaults to accidentally change as that will break existing setups. Most other services I see (eg postgresql) use the repeated names rather than abstracting it out to another variable

Comment on lines +107 to +122
"~@clock"
"~@aio"
"~@chown"
"~@cpu-emulation"
"~@debug"
"~@keyring"
"~@memlock"
"~@module"
"~@mount"
"~@obsolete"
"~@privileged"
"~@raw-io"
"~@reboot"
"~@setuid"
"~@swap"
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"~@clock"
"~@aio"
"~@chown"
"~@cpu-emulation"
"~@debug"
"~@keyring"
"~@memlock"
"~@module"
"~@mount"
"~@obsolete"
"~@privileged"
"~@raw-io"
"~@reboot"
"~@setuid"
"~@swap"
];
"~@clock @aio @chown @cpu-emulation @debug @keyring @memlock @module @mount" @obsolete @privileged @raw-io @reboot @setuid @swap"
];

this should be the same but in one line of the service file instead of ~15

@gianmarcogg03
Copy link
Contributor

Is this getting merged? Hopefully in time for NixOS 23.11.

@kraftnix
Copy link

I have tested this and am currently running it on two Jellyfin setups. One on bare metal and one inside a NixOS Container. Everything has worked as expected.

@SuperSandro2000
Copy link
Member

Closing in favor of #277220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jellyfin: add dataDir option
6 participants