Documentation should call out DLNA and auto-discovery ports as optonal

[wolflarson]

Add optional parameters letting users know about Service Discovery and Auto Discovery

---

We welcome all PR’s though this doesn’t guarantee it will be accepted.

Description:

Update README to include additional ports available in jellyfin

Benefits of this PR and context:

https://github.com/linuxserver/docker-jellyfin/issues/59

This has come up in support channels and linuxserver's own documentation does not make it clear that this is available.

How Has This Been Tested?

Users report DLNA is now working

Source / References:

https://jellyfin.org/docs/general/networking/index.html

[Roxedus]

Please note this text from the PR template:

<!--- We maintain a changelog of major revisions to the container at the end of readme-vars.yml in the root of this repository, please add your changes there if appropriate -->

[LinuxServer-CI]

I am a bot, here are the test results for this PR: https://lsio-ci.ams3.digitaloceanspaces.com/lspipepr/jellyfin/10.6.4-1-pkg-71b3504c-pr-60/index.html https://lsio-ci.ams3.digitaloceanspaces.com/lspipepr/jellyfin/10.6.4-1-pkg-71b3504c-pr-60/shellcheck-result.xml

[wolflarson]

Given that this is a minor documentation change I did not think it matches the "major revisions" qualification? Those seemed like functional changes.

[Roxedus]

The readme is generated using the variables in said file.

[LinuxServer-CI]

I am a bot, here are the test results for this PR: https://lsio-ci.ams3.digitaloceanspaces.com/lspipepr/jellyfin/10.6.4-1-pkg-71b3504c-pr-60/index.html https://lsio-ci.ams3.digitaloceanspaces.com/lspipepr/jellyfin/10.6.4-1-pkg-71b3504c-pr-60/shellcheck-result.xml

[ironicbadger]

@Roxedus I wouldn't say that this constitutes a major change, as @wolflarson said it's just a minor documentation change thus putting an entry in the changelog feels a little overkill though you could argue it either way.

See my above comments with regards to the overall placement of the suggested changes though. @wolflarson you may as well revert your changes to readme.md as they will be overwritten by the pipeline CI automation.

[ironicbadger]

I noticed that the extra field optional_parameters was added to readme-vars.yml. This is not supported by the readme generation system LSIO has - I've made a new PR #61 which incorporates my suggestions and puts the ports in valid locations supported by the CI pipeline.

I would have merged with your PR but I don't have permissions - sorry for the PR hijack and thanks for the contribution.

[wolflarson]

I got optional_parameters from https://github.com/linuxserver/docker-plex/blob/master/readme-vars.yml#L38.

Should I close this PR then and you will move forward with #61 @IronicBadger ?

[ironicbadger]

Oh my mistake on the optional_parameters bit, it's been a while since I looked at this stuff. The Plex container referenced has a lot of these params.

I think on this occasion it fits better under app setup because of the way the rest of the docs are already laid out. It's more a question of what are the LSIO stylistic guidelines for putting stuff where - it's a little unclear.

As for whether you should close this PR or not, that's up to LSIO not me 👍

[wolflarson]

Adding it to parameters directly was discussed in Discord last week. Several people expressed that parameters should only include ports required to get the app working. Given that I opted to put it under optional_parameters.

[ironicbadger]

That makes perfect sense, thanks for the clarification. I'll re-review our two PRs tomorrow and close mine if it makes more sense to do so. 👍

[ironicbadger]

@wolflarson following up with you. The PR as it stands doesn't build properly due to some YAML whitespace issues, you can test putting the optional_parameters section through the builder with

docker run --rm   -v $(pwd):/tmp   -e LOCAL=true   -e PUID=$(id -u) -e PGID=$(id -g)   linuxserver/jenkins-builder:latest && rm -f "$(basename $PWD).md"

I've rolled these changes into #61 now and incorporated the comment of "Several people expressed that parameters should only include ports required to get the app working." into that PR now as well via this commit.

It would therefore be my suggestion that this PR is closed in favour of #61 but you can take all the credit as I've really just copy / pasta'd your changes! :smile:

[nemchik]

Closed via https://github.com/linuxserver/docker-jellyfin/pull/61

Thanks for putting this together @wolflarson and @IronicBadger