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.

Sign up for GitHub

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

Jump to bottom

Add working dockerfile and update readme #77

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add working dockerfile and update readme #77

wants to merge 1 commit into from

Conversation

ctkcoding
Copy link

@ctkcoding ctkcoding commented Jan 6, 2024 •
edited
Loading

Add Dockerfile to allow build to docker image and update readme's docker install instructions

@ctkcoding ctkcoding force-pushed the main branch 2 times, most recently from 0377906 to 5bdc781 Compare January 6, 2024 21:36
@ctkcoding
Copy link
Author

Built the referenced docker container and pushed to https://hub.docker.com/repository/docker/ctkcoding/dir2cast

@ctkcoding
Copy link
Author

@ben-xo not seeing the option to request a review like I'm used to, so tagging you in the comments here instead

@ben-xo
Copy link
Owner

ben-xo commented Jan 24, 2024

Hey, thanks for this.

I have some feedback, which I'll leave as a review. However it's worth noting that dir2cast already has a docker example (using nginx and php-fpm) - checkout the docker-compose.yml file already in the repo. I guess it doesn't really work the same way, and there are pros and cons - and personal preferences - to both approaches. Also I guess the current example isn't actually documented in the README.

e.g do you want dir2cast and config on your filesystem or hidden within the container? are you more of an Apache guy or a Nginx fan? (i'm the latter, but I am not a php-fpm fan so it kind of cuts both ways).

Anyway i'll leave you a proper review next.

@ben-xo ben-xo self-requested a review January 24, 2024 22:25
ben-xo
ben-xo reviewed Jan 24, 2024
View reviewed changes
Copy link
Owner

@ben-xo ben-xo left a comment

Choose a reason for hiding this comment

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

Some things I spotted.

Dockerfile Outdated
Comment on lines 14 to 27
# add any dir2cast.ini options as env vars here
# todo - making this reusable for all env vars
RUN echo '; review options in dir2cast.ini source' > /var/www/html/dir2cast.ini
RUN echo "ITEM_COUNT = ${ITEM_COUNT}" >> /var/www/html/dir2cast.ini
RUN echo "MP3_DIR = ${MP3_DIR}" >> /var/www/html/dir2cast.ini
RUN if [[ $TITLE ]]; then echo "TITLE = ${TITLE}" >> /var/www/html/dir2cast.ini; fi;
RUN if [[ $MP3_URL ]]; then echo "MP3_URL = ${MP3_URL}" >> /var/www/html/dir2cast.ini; fi;

RUN if [[ $IMAGE ]]; then echo "IMAGE = ${IMAGE}" >> /var/www/html/dir2cast.ini; fi;
RUN if [[ $ITUNES_EXPLICIT ]]; then echo "ITUNES_EXPLICIT = ${ITUNES_EXPLICIT}" >> /var/www/html/dir2cast.ini; fi;
RUN if [[ $DESCRIPTION ]]; then echo "DESCRIPTION = ${DESCRIPTION}" >> /var/www/html/dir2cast.ini; fi;
RUN if [[ $ITUNES_AUTHOR ]]; then echo "ITUNES_AUTHOR = ${ITUNES_AUTHOR}" >> /var/www/html/dir2cast.ini; fi;
RUN if [[ $RECURSIVE_DIRECTORY_ITERATOR ]]; then echo "RECURSIVE_DIRECTORY_ITERATOR = ${RECURSIVE_DIRECTORY_ITERATOR}" >> /var/www/html/dir2cast.ini; fi;
RUN if [[ $AUTO_SAVE_COVER_ART ]]; then echo "AUTO_SAVE_COVER_ART = ${AUTO_SAVE_COVER_ART}" >> /var/www/html/dir2cast.ini; fi;
Copy link
Owner

Choose a reason for hiding this comment

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

so, I assume what you intended was for someone to be able to do e.g. docker run -d -p 8080:80 -v podcast_volume:/var/www/html/episodes -e IMAGE=... -e ITUNES_EXPLICIT=... ctkcoding/dir2cast:v0.1 etc etc. but, these RUN lines run at build time, not run time.

There are two standard approaches to deal with this.

  1. the user should edit dir2cast.ini and then mount it at run time with -v dir2cast.ini:/var/www/html/dir2cast.ini
  2. use envsubst to replace placeholders in the baked in dir2cast.ini at run time, so you can do -e THING=whatever like i imagine you intended. (You will need to set a default for every single one i think). This way is harder but nicer for the user

Copy link
Author

Choose a reason for hiding this comment

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

Updated to support ini creation through environment variables at container run time rather than image build time

dir2cast.php Outdated Show resolved Hide resolved
dir2cast.php Show resolved Hide resolved
Dockerfile Outdated
Comment on lines 32 to 41
# COPY ./dir2cast.ini /var/www/html/
COPY ./getID3/ /var/www/html/getID3/
Copy link
Owner

Choose a reason for hiding this comment

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

so, the code for getID3 and also the dir2cast.ini file will be downloadable, unless you add some .htaccess to block them. There's instructions for that in the README.

Copy link
Author

Choose a reason for hiding this comment

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

This type of server isn't in my expertise so I might be missing something, but it looks like the ini file has to be in the same relative position to the php file the way things are now so it seems like admins would need to set up .htaccess to block whether its running from my docker image or any of the ways listed in the readme

Copy link
Author

Choose a reason for hiding this comment

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

Updated with .htaccess to block any .php or .ini requests, and seeing a 403 when I try /getID3/ as the path.

@ctkcoding
Copy link
Author

Thanks for the feedback - will clean up & address over the weekend! Also re why I'm not into docker compose - I run all my docker apps using images from docker hub and it's more convenient accessing it through a gui on my laptop than sshing in to the server itself to manage things. Re apache vs nginx - my real preference is dealing with a single port on my host machine because it's all abstracted by docker

@ben-xo
Copy link
Owner

ben-xo commented Jan 25, 2024

Thanks for the feedback - will clean up & address over the weekend! Also re why I'm not into docker compose - I run all my docker apps using images from docker hub and it's more convenient accessing it through a gui on my laptop than sshing in to the server itself to manage things. Re apache vs nginx - my real preference is dealing with a single port on my host machine because it's all abstracted by docker

docker-compose is supported by both OrbStack and the official Docker desktop app, as far as i know!

@ctkcoding
Copy link
Author

ctkcoding commented Mar 9, 2024 •
edited
Loading

@ben-xo updated to address feedback

  • run-time environment variables that generate new dir2cast.ini file
  • reverted php code change erroneously pulled from other branch
  • blocked getID3 and dir2cast.ini access from path manipulation

@ctkcoding
Dockerfile for image build and update README
10c36ba 
Dockerfile for image build and update README

Add working dockerfile and update readme

enable docker image build

clean logs

code cleanup
@ctkcoding ctkcoding force-pushed the main branch 5 times, most recently from 4e9cf3f to 10c36ba Compare March 16, 2024 17:16
@ctkcoding
Copy link
Author

@ben-xo any comments/feedback on the latest version or does this look good to merge?

@ctkcoding ctkcoding requested a review from ben-xo March 28, 2024 02:50
@ctkcoding
Copy link
Author

@ben-xo ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ben-xo ben-xo Awaiting requested review from ben-xo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ctkcoding @ben-xo