Skip to content
This repository has been archived by the owner on Mar 24, 2022. It is now read-only.

php: add PHP_INI_DIR variable to container #34

Merged
merged 3 commits into from Jul 22, 2019
Merged

Conversation

glensc
Copy link
Contributor

@glensc glensc commented May 26, 2019

  • remove $DEPS variable from the final image
  • add $PHP_INI_DIR to be symmetrical with official php docker image
# this project
$ docker run --rm -it app:latest sh -c 'ls -ld $PHP_INI_DIR/conf.d'
drwxr-xr-x    2 root     root          4096 May 26 00:52 /etc/php/7.1/conf.d

# official docker php image
$ docker run --rm -it php sh -c 'ls -ld $PHP_INI_DIR/conf.d'
drwxr-sr-x 1 root staff 4096 May  8 02:36 /usr/local/etc/php/conf.d

@glensc
Copy link
Contributor Author

glensc commented May 26, 2019

@petk WDYT? if approved, I'll change other Dockerfiles as well.

@glensc glensc changed the title php: add PHP_INI_DIR php: add PHP_INI_DIR variable to container May 26, 2019
@petk
Copy link
Member

petk commented May 27, 2019

Sure... This would be useful for example for knowing where to install some extension's ini configuration file or something similar?

@glensc
Copy link
Contributor Author

glensc commented May 28, 2019

yes. I use it for that purpose exactly:

  1. from php:7.2 or php:7.3
  2. copy blah.ini $PHP_INI_DIR/conf.d

and I do not need to adjust the path when I change base image ;)

@glensc
Copy link
Contributor Author

glensc commented May 28, 2019

added to all docker files, can be merged now.

for the same reason I'd like to add $PHP_VERSON=7.2 variable. append to this PR, or create new one?

@glensc glensc marked this pull request as ready for review May 28, 2019 10:26
@petk
Copy link
Member

petk commented May 28, 2019

Thanks for the info. I would suggest using this workflow with the ini files (for the readme example):

RUN ... \
    && echo "memory_limit = 512M" >> $PHP_INI_DIR/php.ini \
    && ...

copy-ing entire INI files is not very well organized because the settings might change a lot from version to version.

This ini dir is also basically entire configuration directory here... Let me check a bit, otherwise looks good to me.

@glensc
Copy link
Contributor Author

glensc commented May 28, 2019

I prefer a new file because:

  • separate file has proper syntax hiliting and validation based on the file extension
  • I can synchronize the separate files with different projects more easily.
  • appending to a file creates bigger docker image, due to the storage needed for existing file content before append
  • depending on the order of the commands, the COPY directive could be cached (useful when developing image locally)

I can remove the readme modification from this and you can write your own instructions.

@glensc
Copy link
Contributor Author

glensc commented May 28, 2019

also, my instructions did not suggest copy whole .ini file, but only add project specific settings.

@glensc
Copy link
Contributor Author

glensc commented Jul 6, 2019

ping @petk

@glensc
Copy link
Contributor Author

glensc commented Jul 11, 2019

btw, using this project here: https://github.com/eventum/docker/pull/2/files

@petk petk merged commit 4969bca into phpearth:master Jul 22, 2019
@petk
Copy link
Member

petk commented Jul 22, 2019

If I'm not mistaken, you need to get the location of the ini directory after you install PHP. So basically, with php 7.4 you can also use php-config --ini-dir and php-config ----ini-path I'm not sure why it took so long to get this option in, yes... But now it will be available in PHP directly.

Hopefully here everything works correctly then. Thanks for the pull request.

@glensc
Copy link
Contributor Author

glensc commented Jul 23, 2019

you can't use scripts if you use COPY, see discussion here: docker-library/php#721

@glensc glensc deleted the patch-1 branch July 23, 2019 08:13
@petk
Copy link
Member

petk commented Jul 23, 2019

Yup, that's true. And that's why also echo "ini.directive" >> ... is better and adds less layers to image.

@petk
Copy link
Member

petk commented Jul 23, 2019

Now I remember also why there was a single ENV with new lines defined :D - it adds that many layers less. Another measurement of the image simplicity :D

@glensc
Copy link
Contributor Author

glensc commented Jul 23, 2019

yeah, but echo has other downsides: no suitable version control (adding another echo will likely git blame previous line as well), total mess with syntax (dosini syntax wrapped in shell syntax, including escaping); no layers caching (all commands in single RUN re-run); more difficult to share (copy the file between projects).

typically, single ENV vs multiple ENV add only metadata, not data layers, so not really an issue.

if you're worried about layers data, you could use multi staged build and still be efficient in layers caching:

FROM alpine AS build
....

FROM scratch
COPY --from=build / .

or, to keep alpine layer:

FROM alpine AS base
FROM base AS build
....

FROM base
COPY --from=build / .

there's also docker build --squash which does basically the first example, but that likely can't be automated using dockerhub.

note: this is just a rant, no need real reason to change anything now :)

@petk
Copy link
Member

petk commented Jul 23, 2019

I'll check, thanks for the info. Last time I was checking this, there were some recommendations to keep the layers at minimum for such base images that are used further on...

@glensc
Copy link
Contributor Author

glensc commented Jul 23, 2019

some random thoughts:

I'd keep alpine layer intact. as I might have it cached locally and provides some means to identify layers added by this project. and having multiple layers actually improves download efficiency on first pull, layers are downloaded in parallel, so typically four smaller ones come down faster than one very huge. and of course, this all depends on the downlink.

@glensc
Copy link
Contributor Author

glensc commented Mar 27, 2020

@petk any estimation when the image gets rebuilt?

➔ docker run --rm docker.io/phpearth/php:7.1-cli env|grep PHP_INI_DIR -c
0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants