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 new features, fix .env setup, fix permissions and UID conflicts #50

Merged
merged 14 commits into from
Jul 3, 2024

Conversation

Pavlogal
Copy link
Contributor

@Pavlogal Pavlogal commented May 4, 2024

I've addressed all complaints from #49 and some more.

Changes:

  • Re-added support for Jackett, 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 feature to main.py (it just reads from /etc/timezone),
  • The python script can now act as a compose file generator, as it asks the user if they actually want to set up folders and permissions.
  • Resolved UID conflicts, while keeping the others the same as before
  • Fixed manual setup by making new .env and docker-compose.yml samples. This also gives the end user the ability to customize container UIDs for manual installs. I also changed the provided compose file to a .sample, so it's more like the .env sample and the user will always have a copy of the original even if overwritten by the python script.
  • Since we're endorsing compose v2 I removed version from docker-compose.yml so compose v2 doesn't complain about it. Also added a note to README.
  • Added a script to remove old users and an update script
  • Updated README to reflect changes and added a note for NFS users to save them some trouble

I tried to keep my commit history as clean as possible so I hope reviewing this will be easy, although I did make quite a lot of changes. I did some testing and everything should work perfectly now.

@Pavlogal
Copy link
Contributor Author

Pavlogal commented May 4, 2024

It failed the docker-compose check because there is no longer a docker-compose file but just a sample. It wouldn't work anyway now since docker-compose.yml now depends on .env for manual installs. Checks should be rewritten or I can simply add a python-generated compose file which is hardcoded and should work in the current workflow.

@Ulimn
Copy link

Ulimn commented May 4, 2024

It failed the docker-compose check because there is no longer a docker-compose file but just a sample. It wouldn't work anyway now since docker-compose.yml now depends on .env for manual installs. Checks should be rewritten or I can simply add a python-generated compose file which is hardcoded and should work in the current workflow.

You could add something like a "sample" ci-compose.yml and use it in the CI with the -f flag.

@Luctia Luctia self-requested a review May 13, 2024 08:28
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.

Sorry for taking so long! Life got in the way. Thanks again for your effort, I hope my comments don't demotivate you, they're all pretty small :)

FYI: comments marked by "nit" are nitpicks that you can ignore, but would probably be nice if changed.

Just out of curiosity: did you test the new scripts (remove_old_users.sh and the new setup.sh)?

.env.sample Outdated Show resolved Hide resolved
.env.sample Outdated Show resolved Hide resolved
container_configs.py Show resolved Hide resolved
container_configs.py Show resolved Hide resolved
update_containers.sh Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
setup.sh Show resolved Hide resolved
setup.sh Outdated Show resolved Hide resolved
setup.sh Outdated Show resolved Hide resolved
users_groups_setup.py Show resolved Hide resolved
@Ulimn
Copy link

Ulimn commented Jun 2, 2024

Just a small note: I actually used the remove_old_users.sh script to clean up the mess left by the original that failed. It worked for me.

@Luctia
Copy link
Owner

Luctia commented Jun 17, 2024

@Pavlogal I really like your work apart from my comments, and would like to merge it. Could you let me know when you think you have time to work on the changes? If I don't hear anything before the end of the week, I think I'll merge it anyway and just immediately apply the changes myself.

@Pavlogal
Copy link
Contributor Author

Sorry for inactivity, I'm in the middle of college exam period and will have a little bit of time on the 21st/22nd of june. Let me know if that's okay for you

@Luctia
Copy link
Owner

Luctia commented Jun 17, 2024

Absolutely! No need to apologize, I was just curious. That's absolutely fine, good luck on your exams!

@Luctia
Copy link
Owner

Luctia commented Jul 2, 2024

@Pavlogal do you think you'll get around to it soon, or should I pick it up?

@Pavlogal
Copy link
Contributor Author

Pavlogal commented Jul 2, 2024

I'll start work on it later today

Pavlogal and others added 2 commits July 2, 2024 21:34
Updated auto-check workflow to also copy docker-compose sample file, updated syntax to docker compose without "-"
@Pavlogal
Copy link
Contributor Author

Pavlogal commented Jul 2, 2024

Sorry for the delays, college can be a bitch... I've addressed all the nitpicks. As for audiobookshelf, iirc I was looking into their docs and I couldn't find any mention of AUDIOBOOKSHELF_UID and AUDIOBOOKSHELF_GID like it was in your config. It really annoyed me how poorly documented this was. Their official examples actually always use either a user environment variable in the format UID:GID or a user section in docker compose with the same format:
https://github.com/advplyr/audiobookshelf/blob/master/docker-compose.yml
https://www.audiobookshelf.org/guides/docker-install/
I chose the first option and it seems to work.

I copied the folder structure directly from their docs so it has room for everything it needs, moreover audiobookshelf doesn't have the feature to look for, download and hardlink audiobooks. It's more like a book management service.

I did test the new user scripts back when I opened this PR and they worked really well. You just need to reboot sometimes for changes to fully take effect.

@Luctia Luctia self-requested a review July 3, 2024 09:39
@Luctia
Copy link
Owner

Luctia commented Jul 3, 2024

Big thanks to @Pavlogal for these changes! As far as I can see, there's no more issues and it solves quite a few inconsistencies.

@Luctia Luctia merged commit 1e8220b into Luctia:main Jul 3, 2024
1 check passed
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