-
Notifications
You must be signed in to change notification settings - Fork 98
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
Plex official #169
Plex official #169
Conversation
@coleberhorst Thanks for the submission. |
Linking to your forum thread relating to this pull request for context: "Plex from official image" where @FroggyFlox and forum user Haioken address some issues with this pr. Thanks again for stepping up to supply an official Plex rock-on. |
Ok got the rockon working without a TZ variable. Passing the system timezone to the -e switch ended up working and I updated my pull request. |
Thanks for the update @coleberhorst,
Have you tried without specifying the TZ at all? As mentioned in the corresponding forum post, Rockstor automatically binds the system's localtime to this end so you may not need to. |
as rockstor binds timezone with install on rockon, passing timezone to -e isn't needed.
ok it does seem to work without the timezone binding. i tried a version without timezone, but maybe some other problem prevented it from loading. Works now, I think it is ready for merge and/or someone else to test it. |
@coleberhorst Thanks for you effort on this one and sorting that timezone bit out. Lets get a review and if all is well this can go 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.
I just suggested a few style changes.
Notably, I upon first install on 3.9.1-16, clicking the webUI button led to a browser error page and I had to follow the instructions listed on the image's docker hub documentation, pasted below:
Running on a headless server with container using host networking
If the claim token is not added during initial configuration you will need to use ssh tunneling to gain access and setup the server for first run. During first run you setup the server to make it available and configurable. However, this setup option will only be triggered if you access it over http://localhost:32400/web, it will not be triggered if you access it over http://ip_of_server:32400/web. If you are setting up PMS on a headless server, you can use a SSH tunnel to link http://localhost:32400/web (on your current computer) to http://localhost:32400/web (on the headless server running PMS):
ssh username@ip_of_server -L 32400:ip_of_server:32400 -N
Based on this, it seems this is not restricted to my install and would affect everybody. If that is the case, this process seems a little too involved for some users. Note that I didn't suffer from this with the Linuxserver.io Plex rock-on.
@FroggyFlox re accessing the Plex rock-on Web-UI; we may have had something similar before with our existing Plex container. See: |
Thanks for the link, @phillxnet! Unfortunately, I get a browser error ("the connection was reset" if I remember correctly) and cannot even connect the Plex "Sign-in" page. I can if I follow the workaround documented on docker hub (use ssh tunneling to link localhost on a user machine to the server's ip). |
style Co-Authored-By: coleberhorst <[email protected]>
style Co-Authored-By: coleberhorst <[email protected]>
style Co-Authored-By: coleberhorst <[email protected]>
Co-Authored-By: coleberhorst <[email protected]>
Co-Authored-By: coleberhorst <[email protected]>
Co-Authored-By: coleberhorst <[email protected]>
Ok I tried to test by deleting my plex-config share and replacing the json with a freshly downloaded one from this pull request. It worked well and had to do the usual server fresh setup, but I was still signed into plex (maybe plex pulls shared cookies from plex.tv signin idk. So not a conclusive test. Tried again by deleting plex-config, rockons share (in case it had any credentials or something), and signed out of plex on my browser and even opened it in icognito mode. I was redirected to a sign on screen from the normal app.plex.tv. So I cannot verify any issue requiring ssh to access the web gui on Chrome- Windows 10. I did experience a weird bug where rockstor only presented me the option to choose a share for /data and not for /config. I pressed update and it requested both. I'm not sure if this is fixable in the .json or just one of the quite a few rockon webui bugs I have experienced. |
Oh some plex installs, I have seen rockstor require several minutes to load plex because the install is quite large compared to other images. Maybe that's it? |
Don't you want to add this? I mean does it login without setting it for rockstor? I'll try to test...if not let's add. |
I tested the rockon and it works but adding the CLAIM would help. I added it and it saves the setup step. Load was quick for this container also. (under environment)
|
@phillxnet is there a way to make this claim token optional? I'm fine with including it, but I'm unsure whether the rockon "philosophy" is require the least amount of input from the user. My personal opinion is the plex.tv redirect page is very clean; however, if you like the claim token or claim token option idea, I'll add it. |
If you leave it blank it won’t set. I guess we just need to say that better in the input if it’s not clear. |
Ok when testing the claim code, leaving it blank causes rockstor to complain that it is required. So I'll just wait for a response to my question about how to make environment vars optional? If there isn't a way to make optional environment vars, I'm tempted to revert because adding a box and link to the seems awkward when clicking the Plex-official UI button brings you to the sign in page. I guess it's more of a rockon philosophy question at that point: is it better to have less required inputs or not? |
It depends lol! I know if I used the rockon I'd want the auto-login and would input the tag (it's only during install and a blank box is accepted). I can't do that if the box isn't there. However, the linuxserver.io image doesn't have it and I seem to use a cookie to just login anyway. I'm fine either way honestly. We can also open an issue and PR this if someone wants it later but I thought it would be good to have now. |
Also is this ready to test? |
Yessir! Test away. |
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.
Tested by @magicalyak and worked great! Good to merge @phillxnet @schakrava
Thanks for testing everyone! My only reservation is that the current version forces the user to enter a claim code (leaving blank doesn't work). If that's ok with you guys, I'll just leave it in. I'd need to remove the language about leaving it blank from the description before it gets merged. Unless there's a way to allow it to be blank / optional? Other option is removing it. Need some feedback from others on this. |
If it won’t work blank remove that part and let’s go without it. Sorry to make this difficult. |
This reverts commit 3d08866.
ok reverted. ready for merge. no worries its worth considering options to offer to the user! |
Co-Authored-By: coleberhorst <[email protected]>
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.
Thanks for reverting the token option. I agree that we should keep things as simple as possible. Maybe we can add an optional element in the future for this kind of thing.
Just reviewing from a json perspective re my suggestion that we have an extraneous comma added in that last change. The web based https://jsonlint.com/ json checker is good for finding these kinds of hiccups.
throws error on jsonlint.com
awesome thanks for catching those. My revert wasn't as clean as I thought. |
@coleberhorst Well done on persevering with this. It's now undergone a goodly number of changes and had a number of reviews so can we have a final test of what this pr represents now and if it works as expected then lets get it in. |
I confirmed the latest json works. |
The only challenge I see with the official image is that it doesn't offer the additional architectures like the linuxserver does (in reference to this discussion issue by @mcbridematt : rockstor/rockstor-core#2191 And finally, one more question for @coleberhorst , on the docker hub there is this blob about the tag (https://hub.docker.com/r/plexinc/pms-docker - section on Tags):
Since in the json you don't specify a version (i.e. translates into "latest") - does that mean that the version does not get updated unless the container is forcibly removed and then newly pulled (as opposed to a stop/restart triggering an update/upgrade)? If that's the case, we might have to mention that in this RockOn |
@FroggyFlox @Hooverdan96 and @coleberhorst I'm thinking now this pr has reached the no-activity threshold and should be closed. My apologies to all concerned but I think we should now shy away from multiple Rock-ons essentially offering a near identical instance of a project, such as Plex in this case. Such customisation is always still available to folks via the locally hosting Rock-on definitions but from now on I think we should really try for only quite different instances of one project and then only if there is real value in having the different/parallel versions. Also we have now gotten a lot of house work to do so having to be a little more clean cut with getting rid of stuff that has not received appropriate attention. Obviously anyone concerned should chime in if they fee this is an overstep but we must now reduce this repo to something more fluid/flowing and reducing it's backlog is a part of who I would like to approach that. |
Agreed. I think we can close this one for now. |
Added a version of plex that uses the official image source and updates more frequently than current.