Make docroot and server name configurable. #13
Conversation
There was a problem hiding this comment.
I like sed-based approach. Have some doubts though about naming convention and entrypoint usage. Added some inline comments to place it in the context.
Just created pull request to your fork-branch with my suggestions. Curious to hear your opinion :)
Thank you for your involvement and pushing this long-standing issue forward.
| @@ -11,3 +11,6 @@ RUN addgroup -g 82 -S www-data \ | |||
|
|
|||
| COPY nginx.conf /etc/nginx/nginx.conf | |||
| COPY drupal.conf /etc/nginx/conf.d/default.conf | |||
|
|
|||
| COPY entrypoint.sh /usr/local/bin/ | |||
| CMD "entrypoint.sh" | |||
There was a problem hiding this comment.
Should use ENTRYPOINT instruction
| @@ -1,5 +1,5 @@ | |||
| server { | |||
| server_name _; | |||
| server_name SERVER_NAME; | |||
| @@ -1,5 +1,5 @@ | |||
| server { | |||
| server_name _; | |||
| server_name SERVER_NAME; | |||
| root /var/www/html; | |||
There was a problem hiding this comment.
IMHO we should tokenize root as well. I suggest name nginx independent so I could implement it the same way in drupal-docker/apache (maybe DOCROOT).
root DOCROOT
Probably even better to use before/after special char, something like %SERVER_NAME% and %DOCROOT%
| set -e | ||
|
|
||
| # Configure docroot. | ||
| if [ -n "$NGINX_DOCROOT" ]; then |
There was a problem hiding this comment.
As I have mentioned before, this variable should be nginx independent. Maybe just DOCROOT?
| sed -i 's/SERVER_NAME/'"${NGINX_SERVER_NAME}"'/' /etc/nginx/conf.d/*.conf | ||
| echo "Configured nginx server name to ${NGINX_SERVER_NAME}." | ||
|
|
||
| exec nginx -g "daemon off;" |
There was a problem hiding this comment.
This script is suppose to be ENTRYPOINT so it should accept command argument and execute it. See https://github.com/docker-library/mysql/blob/master/5.5/docker-entrypoint.sh#L151 or https://github.com/docker-library/redis/blob/master/3.0/docker-entrypoint.sh#L16
| @@ -11,3 +11,6 @@ RUN addgroup -g 82 -S www-data \ | |||
|
|
|||
| COPY nginx.conf /etc/nginx/nginx.conf | |||
| COPY drupal.conf /etc/nginx/conf.d/default.conf | |||
|
|
|||
There was a problem hiding this comment.
Environment variables should be declared in Dockerfile.
|
Makes sense. It your PR you didn't use replacement tokens with special characters as you mentioned in one of the comments. Should we do that? Thank you for helping. I am constantly learning when contributing to this project. |
|
Yes, I think we should. As we use Still not sure why you suggested changing default server_name from |
Done
Not really sure about that. Reverted back to "_" while still offering to override it. |
Based on discussion in drupal-docker/php#46.
Fixes #6.