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 support for Flaresolverr and Jackett, fix UIDs, add some useful scripts, update README #49

Closed
wants to merge 10 commits into from

Conversation

Pavlogal
Copy link
Contributor

I had some time on hand and I really like this project so I did some updates and cleaning up.

Changes:

  • I noticed Jackett support had disappeared at some point so I reintroduced it, thus addressing: Added support for Jackett #10 (comment)
  • added flaresolverr thus solving FlareSolverr support #48 (comment)
  • fixed audiobookshelf thus solving Docker-compose missing multiple containers #44 (comment)
  • added automatic timezone detection to main.py (it just reads from /etc/timezone),
  • UIDs were all over the place, unsorted, sometimes missing and mismatched. I fixed it all, but at the cost of some being different which could possibly cause trouble for old users re-running the script to get the new services. That's why I added remove_old_users.sh which removes the old users so that new ones can be created without conflicts. I'm considering making some even more drastic changes in the future to make UID's easily customisable instead of hardcoded everywhere but this is good enough for now.
  • changed sample docker-compose to include all services with correct IDs, changed default root folder to just "/ezzar" which partly addresses Don't let people do a multi user install into their /home/user folder #41 (comment)
  • added an update script
  • added write_docker_compose.py which is essentially main.py but without the permission and folder creation
  • updated README to reflect changes and added a note for NFS users to save them some trouble

@Luctia
Copy link
Owner

Luctia commented Apr 27, 2024

Thanks for the work! I'll try to look at this tomorrow.

@Luctia Luctia self-requested a review April 27, 2024 22:19
@Ulimn
Copy link

Ulimn commented Apr 28, 2024

I came to the GH issues because of the audiobookshelf and overseerr UID conflict. Glad you're already working on it. Appreciate the effort! :)

Copy link

@outcast292 outcast292 left a comment

Choose a reason for hiding this comment

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

Thank you for the good work.

The current user creation script creates users that can be used to log in. Is there a possibility to change the UID to something lower than 1000? This way, the users will be accounted for as "System Users."

Alternatively, we could use the command useradd -r -s /sbin/nologin.

PS: For my system (Debian 12), the UIDs from 200 to 240 were available.

4. Run `setup.sh` as superuser. This will set up your users, a system of directories, ensure
permissions are set correctly and sets some more environment variables for docker compose.
5. Take a look at the `docker-compose.yml` file. If there are services you would like to ignore
(for example, running PleX and Jellyfin at the same time is a bit unusual), you can comment them
out by placing `#` in front of the lines. This ensures they are ignored by Docker compose.
6. Run `docker compose up`.
You also need to replace the timezones and root folders. The default timezone is Europe/Amsterdam
Copy link

Choose a reason for hiding this comment

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

Didn't you change it to Sarajevo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that on accident, good catch

- NFS shares' permissions are mapped by user IDs. If you want to access a file as a client, your user ID needs to match the user ID of the owner (or group) of that file on the NFS server.
Note that if you are a group member (and not the owner), having matching group IDs won't be enough, there also needs to be a corresponding user on the NFS server. The easiest way to make sure
the users and groups are set up on both sides correctly is to run setup.sh on both your NFS server and your client. Preferably run it on the server first.
If you are running this script on the client make sure that you temporarily enable -no-root-squash on your NFS server, as the script needs superuser privileges to run and by deafult on NFS the root
Copy link

Choose a reason for hiding this comment

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

Typo: default

Copy link
Owner

@Luctia Luctia left a comment

Choose a reason for hiding this comment

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

Again, thanks for the work! I've added a few comments, but there's also a more fundamental discussion to be had here. First off, I'm not sure why the re-ordering of user IDs was necessary, especially considering the potential compatibility issues if people use this script and already have permissions set on an existing library. I imagine they don't really want to remove all their old users and groups just to add the new ones in a different order, which (AFAIK) means they need to re-set their folder permissions). If you can make a compelling argument for it, I'm okay with it, but otherwise I prefer just using the old UIDs. Someone else actually mentions system users, and I suppose that might be a nice change, although this, too, is mostly administrative.

The re-ordering of everything also makes it quite difficult to review the code. Not much to be done about this now, but wanted to mention it for PRs you might make in the future. In general, it's a good idea to split changes out into multiple PRs as well.

If you'd like to separate some of these changes (like the addition of the new services) from the changed UIDs while we discuss it, that might lead to that separate PR being merged sooner.

Comment on lines +53 to +55
2. ~~Copy `.env.sample` to a real `.env` by running `$ cp .env.sample .env`.~~
3. ~~Set the environment variables to your liking. Note that `ROOT_DIR` should be the directory you
have cloned this in.~~
Copy link
Owner

Choose a reason for hiding this comment

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

What is the reason for striking this through? Using a .env file seems like less work than finding and replacing all occurrences like you suggest in your addition to step 5 (where you also make no mention of the Plex claim token).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured that this was a remainder of a previous implementation because docker-compose didn't use those variables anywhere, I should've asked you about that first

@@ -92,7 +103,9 @@ official SABnzbd website.
## FAQ

### How to update containers
If you'd like to update containers, you can move to the directory of your `docker-compose.yml` file
There is a `update_containers.sh` script that takes care of this. Simply run it and it updates
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: an update_containers.sh

Copy link
Owner

Choose a reason for hiding this comment

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

@@ -8,10 +8,10 @@ services:
- PUID=13001
- PGID=13000
- UMASK=002
- TZ=Europe/Amsterdam
- TZ=Europe/Sarajevo
Copy link
Owner

Choose a reason for hiding this comment

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

Why the change to Sarajevo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accidental, I generated the file by running the compose builder file while pressing enter as a way of saying yes to everything. It also picked up my timezone from the automatic detection I added 😅

Comment on lines +115 to +123

print('Please enter your timezone (like "Europe/Amsterdam") or press enter to use your PCs configured timezone:', end=' ')
timezone = input()
if len(timezone) == 0:
if (timezone == ''):
timezone = open("/etc/timezone", "r").readline().rstrip('\n')

if len(timezone) == 0: # if user pressed enter and reading timezone from /etc/timezone failed then default to Amsterdam
timezone = 'Europe/Amsterdam'

Copy link
Owner

Choose a reason for hiding this comment

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

Nice!

Copy link
Owner

Choose a reason for hiding this comment

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

I see the use for a separate docker-compose.yml builder, but I would much prefer main.py and this file to work together, like main.py importing this file and using it. Having two copies of code in different places makes it very easy to make changes to one and forgetting/mistyping those changes elsewhere which could lead to compatibility issues. It would also be good to mention this file in the README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree 100%, this was just a quick stop-gap solution and I'll make something much better down the line

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for adding the new services. I don't have experience with them myself and don't plan on using them, have you tried them (in this configuration)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jackett was there previously but disappeared at some point, I just reintroduced it. It does work though. Flaresolverr is the new thing and it also works.

@Pavlogal
Copy link
Contributor Author

Pavlogal commented Apr 28, 2024

Again, thanks for the work! I've added a few comments, but there's also a more fundamental discussion to be had here. First off, I'm not sure why the re-ordering of user IDs was necessary, especially considering the potential compatibility issues if people use this script and already have permissions set on an existing library. I imagine they don't really want to remove all their old users and groups just to add the new ones in a different order, which (AFAIK) means they need to re-set their folder permissions). If you can make a compelling argument for it, I'm okay with it, but otherwise I prefer just using the old UIDs. Someone else actually mentions system users, and I suppose that might be a nice change, although this, too, is mostly administrative.

The re-ordering of everything also makes it quite difficult to review the code. Not much to be done about this now, but wanted to mention it for PRs you might make in the future.
If you'd like to separate some of these changes (like the addition of the new services) from the changed UIDs while we discuss it, that might lead to that separate PR being merged sooner.

This is my 2nd ever PR and first time using git 😅, so some of my commits are a mess and could've been better separated. As for the reordering I do recognize that I shouldn't make these sorts of decisions on my own. There were some conflicts like Audiobookshelf - Overseer and Bazarr - Jellyseer. Jackett that previously used 13008 had disappeared at some point. One of each pair inevitably had to be changed which would inevitably break something on someone's config. I figured I might as well reorder the whole thing so it goes in order of selection in main.py, (which ultimately makes it harder to review now, but easier later on) and then add a script for removing old users, which actually came in handy for me too. I know this is a janky and less than ideal solution but I figured it was better to push some solution until I do it properly later. The python files are altogether pretty repetitive and just a template of what each docker-compose service and useradd/permission should look like. Changing one ID requires changing both helper programs. I plan on possibly completely rewriting it down the line with some more elegant solution like letting the user globally choose their own IDs without editing each of the files.
Of course I do see your side of the argument as well so I'll understand if you reject this change and I'll try to revert it.

@Ulimn
Copy link

Ulimn commented May 2, 2024

There were some conflicts like Audiobookshelf - Overseer and Bazarr - Jellyseer.

Can it be solved by introducing previously unused user ids? 2 of the above would have new ones (maybe Audiobookshelf for example as I assume it's not as widely used as Overseer). If I'm not mistaken, this would have the least impact.

Since they share the UID, one has to be changed, it cannot stay like this.

@Pavlogal
Copy link
Contributor Author

Pavlogal commented May 2, 2024

There were some conflicts like Audiobookshelf - Overseer and Bazarr - Jellyseer.

Can it be solved by introducing previously unused user ids? 2 of the above would have new ones (maybe Audiobookshelf for example as I assume it's not as widely used as Overseer). If I'm not mistaken, this would have the least impact.

Since they share the UID, one has to be changed, it cannot stay like this.

I am currently working on a new PR that addresses this, and all the other complaints. The order of IDs will be kept the same, and conflicts will be resolved as follows: Overseer will remain as is and Audiobookshelf will be moved to the first available ID of 13014. Jackett will use its old ID of 13008.

However I want to consult @Luctia on some things first, so that this can go as smoothly as possible.

  1. Is there any particular reason why every folder is owned by the user? I think it's much more elegant to just give each folder to their respective container and add the user to the mediacenter group instead of vice versa. I actually already changed this in this PR and it works well.
  2. About that .env... I see it was used at some point but right now it does nothing as the provided docker-compose.yml is hardcoded, the only environment variable it uses is $UID (for jellyfin and tautulli). Docker actually throws a warning about it since shell variables need to be exported first (and it's absent from the .env file), so in the end it can't find the variable and just throws a blank string as a default. That's why I crossed out those steps in README. Do we want to reintroduce this functionality? I can easily fix this and maybe I could even make a UID variable for each container so that users can choose their own IDs, thus giving them the power to use whatever order and range they like.
  3. docker-compose v1 vs v2... currently both work but v2 complains about the version line in docker-compose.yml, since that feature has been deprecated and is now used just informatively for backwards compatibility. I think that can seamlessly be bumped up to 3.8 which is the latest version of the file spec that both v1 and v2 support, or it can be omitted completely if we want just v2 support.

@Luctia
Copy link
Owner

Luctia commented May 3, 2024

Great to hear you're working on a new version @Pavlogal! I was thinking about implementing some of your changes myself, but I don't want to invalidate your work.

1. Is there any particular reason why every folder is owned by the user? I think it's much more elegant to just give each folder to their respective container and add the user to the `mediacenter` group instead of vice versa. I actually already changed this in this PR and it works well.

I'm not sure what you mean by giving folders to their containers (this kinda already what is happening; a folder belongs to a user which is linked to the container). The structure of users and folders is based on the servarr Docker guide. They do mention an alternative which might be what you're doing, but I'd need to take another look.

2. About that `.env`... I see it was used at some point but right now it does nothing as the provided `docker-compose.yml` is hardcoded, the only environment variable it uses is $UID (for jellyfin and tautulli). Docker actually throws a warning about it since shell variables need to be exported first (and it's absent from the .env file), so in the end it can't find the variable and just throws a blank string as a default. That's why I crossed out those steps in README. Do we want to reintroduce this functionality? I can easily fix this and maybe I could even make a UID variable for each container so that users can choose their own IDs, thus giving them the power to use whatever order and range they like.

I actually saw that too when looking at your PR, an oversight on my end. I think the .env file is more elegant than changing it everywhere in the Compose file, but I think this should have no effect on the Python flow, just on the "take this compose file, comments some stuff out and throw it up" flow.

3. docker-compose v1 vs v2... currently both work but v2 complains about the version line in `docker-compose.yml`, since that feature has been deprecated and is now used just informatively for backwards compatibility. I think that can seamlessly be bumped up to 3.8 which is the latest version of the file spec that both v1 and v2 support, or it can be omitted completely if we want just v2 support.

If we're going to be changing versions anyway, I would prefer using the latest (stable) version. I think Compose v2 also works with older files, so that's fine, but the difference in command style (docker-compose vs docker compose) should be something we take into account when talking about scripting and documentation.

Good luck with the updated PR and feel free to discuss more! I'll try to answer quickly.

@Pavlogal
Copy link
Contributor Author

Pavlogal commented May 3, 2024

@Luctia thanks for the answer, really helped me understand what page we're on!

  1. Anyway as for the 1st question I was referring to the python file for setting up users and groups. The python file actually does something differently to the script as there is a part that gives ownership of each folder to $(id -u), aka the current user. I changed it to be more like the script so it gives each folder to their respective container and applies a 775 mask. I also added the user to the mediacenter group so that he can still have access to his files for any manual intervention. This change would finally synchronise what CLI and the script do and it should follow the recommended servarr config.

  2. As for the env file, I already made a fix for that, I wrote a brand new docker-compose that utilizes all the env variables. It really is a more elegant solution. I suggest providing that file as a docker-compose.yml.sample, since a CLI generated file would otherwise override it and if someone wanted to switch to a manual installation they would have to git pull again.

  3. Glad we're going with v2, seems to overall just work better. I adjusted the update script to reflect that but we should also make some notes in README so that people know that v2 is recommended.

I've been doing this on a new branch and I'll open a new PR tomorrow, though if you want to take an early peek you can check out my branch oldorder.

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.

4 participants