-
Notifications
You must be signed in to change notification settings - Fork 81
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
Conversation
automatic timezone detection
and updating the containers
also use a previously unused function to clean up code
new .env sample, fixed setup
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 |
You could add something like a "sample" |
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.
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
)?
Just a small note: I actually used the |
@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. |
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 |
Absolutely! No need to apologize, I was just curious. That's absolutely fine, good luck on your exams! |
@Pavlogal do you think you'll get around to it soon, or should I pick it up? |
I'll start work on it later today |
Updated auto-check workflow to also copy docker-compose sample file, updated syntax to docker compose without "-"
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 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. |
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. |
I've addressed all complaints from #49 and some more.
Changes:
main.py
(it just reads from /etc/timezone),.env
anddocker-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.version
fromdocker-compose.yml
so compose v2 doesn't complain about it. Also added a note to README.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.