-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Config] Add custom config location #2097
Conversation
Dockerfile
Outdated
COPY --chown=www-data:www-data ./ /app/ | ||
|
||
CMD ["/app/start-rssbridge"] |
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.
Missing newline. Also wouldn't it be better to rename this script to something like docker-entrypoint.sh
, to indicate that it's related to Docker image?
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.
sure, no problem
start-rssbridge
Outdated
for f in `find /config/ -type f`; do | ||
fname=${f##*/} | ||
case $fname in | ||
*Bridge.php) yes | cp $f /app/bridges/ ; |
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.
While bridges' names don't contain spaces, it's better to quote these anyway.
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.
Also, wouldn't it be better if these files were soft-linked?
*Bridge.php) yes | cp $f /app/bridges/ ; | |
*Bridge.php) ln -s "$f" /app/bridges/ ; |
Something like this should do.
|
Wouldn't softlinking the file make it possible to change the content during runtime? I dont know if that has implications on the stability. If you remove a bridge from the config folder while the app is running, it will break the rss-bridge. I see no downside of copying the content to the /app/bridges path since that is in the container and not mapped anyways. So it will always be container-default at the start. Also, what happens if you customize one of the already-present bridges? Pretty sure linking a file to an existing file will cause an error (which could be solved by just overwriting it). So about the quoting: I would do a different change: Check the filename for spaces and log out an error message before the case.
Because copying the file (or symlinking) wont help the user at all, since it wont show up. So how about I add the check and not do the change about linking? |
I haven't considered that. Symlinking seemed more elegant to me.
Yeah, I have added
My point was, that it is a good idea to quote every variable in Bash to prevent accidental splitting. Adding that additional check might be a good idea, though. |
How about now? |
Co-authored-by: Evo <28950897+verahawk@users.noreply.github.com>
Seems good. Although you might want to run shellcheck on it, as it still complains about some stuff. Also it seems, that they require max. 80 chars per line, so it would be good to wrap those comments. |
There we go. I actually misread your comment as "spellcheck", not "shellcheck" as I didnt know about that :D All the findings are okay for me, I wouldn't add another commit for it. |
Haha. Yup, those aren't that important. Well done. Now we can only wait. |
Thanks a lot for the thorough review! @em92 any more wishes? |
I just tested this and noticed that the rename stripped the script of the executable flag. It now works again |
Great.. there seem to be more issues. @em92 wait with the merge. I will do more testing and then do a new PR, doing all the commits in one, as this is already 6 commits deep for a feature. I will reference this pr in the new one. |
I assume this is going to cause some discussion but I wanted to present my solution to the problem with mounting local configs for whitelist, config.ini and custom bridges. This should solve all three issues.
What this does:
What this accomplishes:
will work the same as
All the logic checks and merges of rss-bridge still apply, as this basically just adds files to paths where rss-bridge would expect the files to be anyways.
Impact on existing installations: None!
If you haven't mounted the /config folder, nothing will change. If you have mounted the whitelist.txt directly, nothing will change (as long as you dont try to do both at the same time, but that would mean you are aware of the change and you mount the config folder...).
Useability impact on new users:
I have tested this with multiple bridges, non-valid files and folders and so on and I wasn't able to break it, so please, test if you are able to.
I will tag all the previous issues and discussions about this topic to get more brains on this.
Greetings