[project-base] adding php.ini to image is now done only in dockerfiles #497
Conversation
MattCzerner
commented
Sep 21, 2018
•
MattCzerner
commented
| Q | A |
|---|---|
| Description, reason for the PR | Mounting of php.ini using volumes does not apply the changes without manually restarting the php-fpm process. Because of that, the mounting is not used anymore and instead it is added using COPY during Docker image build. That way, if anything changes in php.ini, it is added to the image during build and loaded when the container starts. |
| New feature | No |
| BC breaks | No |
| Fixes issues | ... |
| Standards and tests pass | Yes |
| Have you read and signed our License Agreement for contributions? | Yes |
|
Php.ini is mounted there but it does not apply changes if you change something. I dont think that php.ini configuration isnt something that should be mounted, it is changed really rarely and it could improve performance for docker-sync. In kubernetes, there is not any reason to make it volume. |
|
I agree, that there is no reason to mount the config files in Kubernetes manifests. I would remove it from there, just as you suggest. I would argue that the config files should behave consistently in Docker Compose during development - look at https://github.com/shopsys/shopsys/blob/v7.0.0-beta1/docker/conf/docker-compose.yml.dist and notice that there are Some of these mounts are necessary because official Docker images are used (and we don't want to create our own Dockerfile atm). Some of these mounts are redundant - they overwrite already copied files during build. Mounting of Personally, I think that from the point of view of a developer using monorepo, all config files should behave the same, or at least it should be clear what happens when. This is a valuable insight for me in context of our planned mutli-stage Docker image building - we should probably make this behavior consistent and documented. Maybe use mounts for local development (with both the source codes and the configs NOT copied into the image), no mounting on our CI (as they are copied into the image) and no mounting to the product images? We might even need to build our own Docker images of services such as nginx, just because of having more control over tagging and having the config files copied inside them - or is the config-mounting in Kubernetes OK for production images? |
|
Okay, ur right in case of consistency. I dont see really the problem in having 2 scenarios. 1st - our dockerimages, we add it using dockerfile and not mount it at all, i think that there is no point, even if it is mounted it is not automatic to load this configuration into running php process. 2nd - 3rd party dockerimages, it would be wierd to create dockerfile just to add configurations. But i guess thats different problem then just removing volumes. Should i just delete mounts of kubernetes and mount of configs in docker-compose let be, since it will be decided in multistage US? |
|
I would be perfectly fine with both solutions: either just remove the redundant Kubernetes mounts, or remove all config mounts from our docker images and add the COPY instruction to both our |
There was a problem hiding this comment.
You still mount the .ini files in project-base's docker-compose.ymls, eg. here, remove it there as well please.
Should we update the UPGRADE.md? Users will need to rebuild their php-fpm container for it to contain, but only if they manually remove the config mounts, so it should be backwards-compatible; the upgrade step might be a bit confusing for some...
How shall we test it?
| @@ -35,8 +35,7 @@ services: | |||
| volumes: | |||
| - shopsys-framework-sync:/var/www/html | |||
| - shopsys-framework-vendor-sync:/var/www/html/vendor | |||
| - shopsys-framework-web-sync:/var/www/html/project-base/web | |||
| - ./project-base/docker/php-fpm/php-ini-overrides.ini:/usr/local/etc/php/php.ini:delegated | |||
| - shopsys-framework-web-sync:/var/www/html/project-base/web:delegated | |||
There was a problem hiding this comment.
why delegated? probably typo
| # allow configuring the GitHub OAuth token | ||
| ARG github_oauth_token | ||
| RUN composer config -g github-oauth.github.com $github_oauth_token | ||
|
|
||
| # hirak/prestissimo makes the install of Composer dependencies faster by parallel downloading | ||
| RUN composer global require hirak/prestissimo | ||
|
|
||
|
|
|
Okay i deleted mounting of php-ini of php-fpm container too. Now it is only possible to reload php.ini configuration by build of dockerimage. |
| @@ -35,8 +35,7 @@ services: | |||
| volumes: | |||
| - shopsys-framework-sync:/var/www/html | |||
| - shopsys-framework-vendor-sync:/var/www/html/vendor | |||
| - shopsys-framework-web-sync:/var/www/html/project-base/web | |||
| - ./project-base/docker/php-fpm/php-ini-overrides.ini:/usr/local/etc/php/php.ini:delegated | |||
| - shopsys-framework-web-sync:/var/www/html/project-base/web:delegated | |||
There was a problem hiding this comment.
the delegated is probably a typo, please fix it or explain it
| @@ -4,7 +4,7 @@ services: | |||
| image: postgres:10.5-alpine | |||
| container_name: shopsys-framework-postgres | |||
| volumes: | |||
| - ./project-base/docker/postgres/postgres.conf:/var/lib/postgresql/data/postgresql.conf:delegated | |||
| - ./project-base/docker/postgres/postgres.conf:/var/lib/postgresql/data/postgresql.conf | |||
There was a problem hiding this comment.
the removal of delegated is probably a mistake
| @@ -4,7 +4,7 @@ services: | |||
| image: postgres:10.5-alpine | |||
| container_name: shopsys-framework-postgres | |||
| volumes: | |||
| - ./docker/postgres/postgres.conf:/var/lib/postgresql/data/postgresql.conf:delegated | |||
| - ./docker/postgres/postgres.conf:/var/lib/postgresql/data/postgresql.conf | |||
There was a problem hiding this comment.
this makes me think there is a reason for the changes - why are you doing this?
| @@ -60,6 +60,9 @@ ENV LC_ALL=en_US.utf8 LANG=en_US.utf8 LANGUAGE=en_US.utf8 | |||
| # overwrite the original entry-point from the PHP Docker image with our own | |||
| COPY docker-php-entrypoint /usr/local/bin/ | |||
|
|
|||
| # copy php.ini configuration | |||
| COPY project-base/docker/php-fpm/php-ini-overrides.ini /usr/local/etc/php/php.ini | |||
There was a problem hiding this comment.
this assumes that the Docker build context is the root of the monorepo, right? But the COPY above assumes the actual context is the docker directory. Both can't be true.
|
Please, rename the PR to reflect the new changes and our rules about package-tagging. |
MattCzerner
changed the title
- all php.ini configurations are added to image during dockerimage build
MattCzerner
force-pushed
the
mc-bad-volume-mount
branch
from
4946ad0
to
bfa515d
Compare
|
I did it by global find and replace function and i didnt see the delegated options. I rebased it and squashed it into one lucid commit. Also I got a question about upgrade notes. Because you should copy new version of So do we write it into |
There was a problem hiding this comment.
In the UPGRADE.md there should an explanation that we changed the way php.ini config files are used in the containers - that they are copied during the image build instead of mounting it. I would inform the users that they can remove all the volumes mentioning php-ini-overrides.ini (just as in the original template), but they will have to rebuild the image for any changes to take place from now on.
Please, change the name of the PR to fully reflect this change. The word unnecessary should definitely not be there, as it was necessary until you've added the COPY instruction in php-fpm Dockerfile.
Please, provide reasons for this change in the description - without them we can see a new PR in a few months named "Deleted unnecessary copying of php.ini configuration" with a reason "configuration is added there using volume mounts". Please, clarify why is this approach better.
MattCzerner
changed the title
|
@PetrHeinz I updated name of PR and description, also i have added record to |
MattCzerner
changed the title
