-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Minidlna: Allow more configuration options #57036
Conversation
a8fca0e
to
3ed89ae
Compare
Fixed mixed tab/spaces characters. |
@infinisil would be real nice to be able to point someone to an RFC regarding module options and config files right now... 😃 |
47878af
to
97fede2
Compare
nixos/tests/minidlna.nix
Outdated
album_art_names=Cover.jpg/cover.jpg/AlbumArtSmall.jpg/albumartsmall.jpg | ||
album_art_names=AlbumArt.jpg/albumart.jpg/Album.jpg/album.jpg | ||
album_art_names=Folder.jpg/folder.jpg/Thumb.jpg/thumb.jpg | ||
notify_interval=60 |
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.
Can you please correct the invento indentation?
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.
yes, on it.
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.
Done btw ;)
97fede2
to
fb51d22
Compare
@ardumont how do you feel about something like this? master...aanderse:minidlna |
That sounds about right and way more sustainable in the long run indeed. Thanks for the pointer, I understand the current limitations. I'm not In any case, one remark in the current context, the mediaDirs option To demonstrate that this specific option can be repeated, i suppose I imagine you kept it for backward compabibility ;) |
I specifically kept mediaDirs because the service won't effectively operate without specifying media directories. Generally speaking any option for which there cannot be a default value and the user must specify for the service to run probably deserves a NixOS option with documentation. I like to see the new The loglevel was kept for backwards compatibility, though. In a release or two it should probably be removed. |
Right, that sounds like sane choices indeed. |
@infinisil do you feel like reviewing master...aanderse:minidlna as an alternative to the code in this PR? I'd like to hear your thoughts on the proposed code because the goal of this code is to spread the word of your new RFC 😄 Bonus points if you write something really cool to replace my horrible toKeyValue implementation 😉 |
}; | ||
|
||
services.minidlna.extraConfig = mkOption { | ||
type = types.str; |
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.
types.lines
?
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.
This seems to be fine with types.lines as well, fixing.
@@ -36,6 +36,36 @@ in | |||
''; | |||
}; | |||
|
|||
services.minidlna.friendlyName = mkOption { | |||
type = types.str; | |||
default = "${config.networking.hostName} MiniDLNA"; |
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.
I think you'll need a defaultText
field as well containing something like
defaultText = "$HOSTNAME MiniDLNA"
otherwise the manual would have to be rebuilt for each host.
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.
ok
'' | ||
Use a different container as the root of the directory tree presented | ||
to clients. The possible values are: | ||
,* "." - standard container |
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.
Will this list be formatted OK?
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.
No idea, i pasted as is from minidlna's documentation.
Will simplify this.
@GrahamcOfBorg test minidlna |
ping (triage) |
pong? ;) |
@ardumont: Can you squash fixup commit(s?) and prefix commits with "nixos/minidlna:"? |
df06b8f
to
202bfeb
Compare
done |
- "P" - "Pictures" | ||
- "V" - "Video" | ||
- Or, you can specify the ObjectID of your desired root container | ||
(eg. 1$F for Music/Playlists)ss |
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.
Is the trailing 'ss' supposed to be here?
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 like typos indeed.
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.
Fixed
{ ... }: | ||
{ | ||
imports = [ ../modules/profiles/minimal.nix ]; | ||
networking.firewall.allowedTCPPorts = [ 8200 ]; |
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.
Perhaps the module could use an "openFirewall" option?
According to https://help.ubuntu.com/community/MiniDLNA, we might want to open UDP port 1900 too.
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.
Personally, i use it solely with the tcp port 8200 and it run fine so far ;)
Also, I checked my configuration, and indeed i opened that port myself.
Then again, that was the current behavior so i kept it.
The proposed patch is about allowing more configuration, not behavior change.
Also, i don't really know what's preventing this to be merged already so i'd rather not change the scope just yet ;)
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.
Ok, fair enough. (Although it wouldn't be behavioural change since openFirewall options (should) default to false in nixpgks.)
This commits allows the user to configure: - more minidlna options - the ones not yet disclosed in nix (extending the existing minimal subset)
202bfeb
to
d228a2d
Compare
Motivation for this change
"As a minidlna user", i'd like to have more setup option choices.
At the moment, we have the choice between:
This PR allows to:
existing minimal subset). This matches what is done for example in the sshd service.
Things done
Tested using sandboxing (nix.useSandbox on NixOS, or option
sandbox
innix.conf
on non-NixOS)Built on platform(s)
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Added test on that service.
I followed the doc about tests [1] though which i think is a better link
https://nixos.org/nixos/manual/index.html#sec-nixos-tests
nix-shell -p nox --run "nox-review wip"
I'm still not sure what that does but it seems to have worked:
nix path-info -S
before and after)Not sure i did this correctly:
Assured whether relevant documentation is up to date
~> every new option opened has the docstring updated as well
Fits CONTRIBUTING.md.
as far as i could tell ;)
Tested execution of all binary files (usually in
./result/bin/
)It's a service, i don't know how to test that without actually
installing it... Any pointer on that is very welcome, thanks. (I
searched quite some time at nixpkgs, nixos, and nix manuals without
quite the answer i hoped for)
So, i settled on testing it with my rpi3 (aarch64), so:
Excerpt:
Cheers,