-
Notifications
You must be signed in to change notification settings - Fork 30
Some QoL changes for the build process #16
base: master
Are you sure you want to change the base?
Conversation
…in tuning of PORTS in the docker-compose setup as it was colliding with my own dockers and ports locally
fix potentially missing /run/php-fpm directory
@@ -3,3 +3,11 @@ MYSQL_USER=contenta | |||
MYSQL_PASSWORD=contenta | |||
MYSQL_ALLOW_EMPTY_PASSWORD=yes | |||
MYSQL_ROOT_PASSWORD=root | |||
|
|||
HOSTNAME=contenta.local |
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.
I think that for decoupled development, it's maybe good having localhost as the host so things like service workers and so on don't get blocked?
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's a really good idea, as long as its on a port I guess. We really don't want to clobber other localhost apps that might be running on the host.
@@ -3,3 +3,11 @@ MYSQL_USER=contenta | |||
MYSQL_PASSWORD=contenta | |||
MYSQL_ALLOW_EMPTY_PASSWORD=yes | |||
MYSQL_ROOT_PASSWORD=root | |||
|
|||
HOSTNAME=contenta.local | |||
HOSTIP=192.168.1.1 |
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.
I think the final goal would be to have a network set up instead of fixing an IP?
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.
Ya, that would likely be a better idea. I was just putting this in as I didn't want it in the docker-compose.yml. Again, the more generic, the better, I suppose.
|
||
HOST_MYSQL_PORT=3336 | ||
HOST_PHP_PORT=9009 | ||
HOST_HTTP_PORT=8888 |
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.
Ports maybe good to have by default but why don't use the default ports?
I'm guessing adding a .env.defaults with all this would be better instead and add .env to the gitignore
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.
Ya, the defaults would be a good idea to use as a common base, I put those in as I wanted to show an example of slightly modified ports. But I'm fine with whatever.
@@ -68,8 +68,9 @@ RUN mkdir -p /var/www && \ | |||
chmod +x /usr/local/bin/docker-entrypoint && \ | |||
chmod +x /usr/local/bin/init-drupal | |||
|
|||
ARG CMS_VERSION=8.x-1.x |
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.
CONTENTA_VERSION?
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.
Ya, that's the contenta_version. Obviously, the base here. I added the CMS_VERSION as this is how it was identified somewhere in the contenta build. So, just took a cue off them.
@@ -4,4 +4,6 @@ ep /etc/php.ini | |||
ep /etc/php-fpm.conf | |||
ep /etc/php-fpm.d/* | |||
|
|||
[ ! -e /run/php-fpm ] && mkdir -p /run/php-fpm |
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.
Why is this needed?
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.
I noticed in one of the forks, this was a problem for some, where the fpm workers weren't starting due to a missing path. I put it in just as a precaution. I've also noticed this setup in docker4drupal, so it must be a common fix.
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.
this solves the issue i run into, where its missing that folder and errors out thus php container never runs, seems like this issue was reported here: #17
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.
👍
I think maybe you could break this PR in smaller ones and we can get in some of the changes you're proposing? Many thanks for working on this! 💪 |
I had a really good time working with the build, but I can into some challenges with my local setup. I found I needed to manage some ports and versions of things here and there. I offer the following changes to improve the build process and the instructions for putting this awesome project together. I hope you find them as useful as I did.