-
Notifications
You must be signed in to change notification settings - Fork 55
Binary location change, Config location change, mountable sites-enabled folder #11
Binary location change, Config location change, mountable sites-enabled folder #11
Conversation
vovimayhem
commented
Aug 26, 2014
- Binary location change
- Config location change
- Mountable sites-enabled folder
This is the fix for #10. I think this is a great change. My minor concern is that the echo lines for the Should we also add Thoughts? /cc @tianon |
&& apt-get purge -y --auto-remove $buildDeps | ||
|
||
ENV PATH /usr/local/nginx/sbin:$PATH | ||
WORKDIR /usr/local/nginx/html | ||
VOLUME ["/etc/nginx/sites-enabled"] |
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.
One more thought is to just make this VOLUME /etc/nginx
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.
Right now /etc/nginx has some default config files (mime.types, for example) that I think a "regular" user will not be aware of their existence...
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.
That doesn't really affect making all of /etc/nginx a volume in the Dockerfile, though. (Plus, for purposes like this image's use in Plushu, we want to override those files anyway.)
I'm not a fan of calling this "sites-enabled" because that's a delta from upstream. Why not just call it "conf.d" instead, since that's what it is? Also, I think we need to decide whether it would be "more upstream" for us to switch to their packages instead of compiling ourselves, which might also have a bearing on this issue. |
|
I agree with your points. If someone wants to @tianon do we want to change to using packages rather than source? |
Yeah, I think we should switch this to use upstream's packages so we can Also, since this image is used equally well for static websites as it is |
I really like the "/nginx/conf.d" suggestion. BTW, what is stopping this PR from being accepted? Is this imminent? |
Sorry for the delay in responses here. Nginx upstream will now be providing and maintaining the images nginxinc/docker-nginx. Which already includes the '/etc/nginx/` directory for configuration as a volume. (docker-library/official-images#197) So, this repository will be deprecated. |