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

Add jellyfin-linuxserver RockOn #227

Merged
merged 5 commits into from
Nov 11, 2020

Conversation

StephenBrown2
Copy link
Contributor

General information on project

This pull request proposes to add a new rock-on for the following project:

  • name: Jellyfin media server
  • website: https://jellyfin.media
  • description: A fork of Emby, Jellyfin works exactly the same way, allowing for media serving from shares in Rockstor

Information on docker image

Checklist

  • Passes JSONlint validation
  • Entry added to root.json in alphabetical order (for new rock-on only)
  • "description" object lists and links to the docker image used
  • "description" object provides information on the image's particularities (advantage over another existing rock-on for the same project, for instance)
  • "website" object links to project's main website

@phillxnet phillxnet added the needs review Test install, function, on / off behaviour, all links / info. label Aug 22, 2020
@FroggyFlox
Copy link
Member

Hi @StephenBrown2 ,

Thanks for another contribution, and again, please accept our apologies in not getting to it earlier.

Thanks a lot for this rock-on, this is a great project for which to have a rock-on and I believe it would be a great addition.

I tested it and it all works as intended... with one major drawback, however. It's not related to your work at all, @StephenBrown2 , but more inherent to the fact that Jellyfin is a direct fork from Emby combined with the fact that they both require host networking for their DLNA and HDHomerun features to work. Indeed, using host networking, docker ignores the published ports settings (expected) which cause some incompatibility with how we are dealing with that.

Indeed, Rockstor automatically detect whether a port setting is already used by another rock-on and increment it by one until it finds an available one. That is a great feature that usually prevents the user from unknowningly set an unavailable port and finds one for you, but in this case it backfires. This is due to the fact that we use the host mapped port number to generate the "webUI link", which means that we are in this case linking to an unused and incorrect port. For instance, the webUI port for Emby is 8096. This is also the case for Jellyfin. During Jellyfin install, Rockstor detects that Emby is already reserving port 8096 (even if not installed) and thus automatically suggests the next available port (8105 in my case). After install, this result in the webUI link pointing to 8105, which is wrong as this port is not actually used (Jellyfin UI is actually accessible at 8096 due to its host networking setting).

Now, that actually highlights a shortcoming in how Rockstor deals with host-networking, but timing is actually quite good as I just submitted some work that, in part, will detect whether a rock-on is using host networking (rockstor/rockstor-core#2207). We should then be able to adapt accordingly how we generate the webUI link and improve the ports section of the rock-on installation wizard, for instance.

In the meantime, the user can still enter 8096 in the "webUI port" field, which will then lead to a proper webUI link. We should thus try to emphasize that requirement in the description field, for instance, and also mention/explain this requirement in the "more info" section.

@phillxnet , do you think this is an acceptable alternative? That's the only option I can think of at the moment without having some work done in the rockstor-core repo to improve how we deal with host-networking rock-ons.

@FroggyFlox FroggyFlox removed the needs review Test install, function, on / off behaviour, all links / info. label Aug 25, 2020
@phillxnet
Copy link
Member

@FroggyFlox Re:

@phillxnet , do you think this is an acceptable alternative?

That seems just fine. I know it's clunky but it at least buys us some time to get your big Rock-on network update in place and then tend to this shortcoming in due course. We should probably, in time, flag all host networking Rock-ons as such and have some kind of generic warning. Plus keep them to a bare minimum where we can of course. But as stated some stuff just expects it, or fails to work without it. Again all stuff we can iterate on as we go.

@FroggyFlox
Copy link
Member

Thanks a lot, @phillxnet, I've created an issue in rockstor-core to keep track of this need:
rockstor/rockstor-core#2209 (comment)

@StephenBrown2 , I understand I might be too late, but let us know if you would be ok with including those small changes designed to help with this:

  • state in webUI port description that it must be 8096.
  • state that this rock-on cannot currently be run alongside the Emby rock-on.
  • mention the requirement for the webUI port to be 8096 in the "more_info" field in case some folks have issues with accessing its webUI because of that.

Once we have this, I believe it will be ready to be merged.

@StephenBrown2
Copy link
Contributor Author

You would think that it wouldn't take me 2 and a half months to update a couple text fields, but here we are, and I've finally made those changes.

Hoping for networking improvements aplenty in due time!

jellyfin-linuxserver.json Outdated Show resolved Hide resolved
@FroggyFlox
Copy link
Member

@StephenBrown2 , thanks a lot for the changes, that looks perfect!

I only added a minor suggestion for you to include, as we now would like to explicitly mention the architectures supported. In your case, both x86_64 and aarch64 are supported (which is great!) so I added a few words to your description accordingly. Sorry for requested yet another small change like that.

Everything else works as intended for me, so once the little addition on the supported architectures are in, I'll go ahead with a final review and merge it.

Thanks a lot!

@FroggyFlox FroggyFlox added the change requested A change has been requested label Nov 9, 2020
@StephenBrown2
Copy link
Contributor Author

@FroggyFlox Changes committed, thanks!

@FroggyFlox FroggyFlox removed the change requested A change has been requested label Nov 11, 2020
@FroggyFlox
Copy link
Member

Thanks a lot for all the updates, @StephenBrown2 !

It all works as intended on my side, on both x86_64 and aarch64 machines, so I'm going to merge it now. Thanks a lot!

@FroggyFlox FroggyFlox merged commit ca88fca into rockstor:master Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants