Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for entrypoint scripts #162

Merged
merged 2 commits into from
Mar 16, 2023
Merged

Conversation

andrewnicols
Copy link
Contributor

This commit adds support for run-time configuration which is executed as part of the startup of the container.

Two options are supported:

  • shell scripts; and
  • .ini files for PHP configuration.

These can be placed into a new directory, located at /docker-entrypoint-initdb.d and files are executed in lexical order returned by a bash glob.

@andrewnicols andrewnicols self-assigned this Mar 15, 2023
@andrewnicols andrewnicols requested a review from stronk7 March 15, 2023 02:32
@andrewnicols andrewnicols force-pushed the entrypoint branch 2 times, most recently from 02d5e14 to 4cf8237 Compare March 15, 2023 13:34
@andrewnicols
Copy link
Contributor Author

Note: If you want to land #163 first, then this same patch built on that branch is here: https://github.com/andrewnicols/moodle-php-apache/pull/new/streamline_entrypoint

@andrewnicols
Copy link
Contributor Author

Also, earlier this evening I encountered this bug: docker/for-mac#5509

I've patched it some hours ago in the script but I've now also found the bug report so adding it to this issue for completeness.

@stronk7
Copy link
Member

stronk7 commented Mar 15, 2023

Hi Andrew,

nice one. I just have an overall (related to this and other PRs that come after this) comment and a couple of observations about this one:

  1. Overall comment: Aren't we adding "too many" overlapping ways to do the same? I mean, shouldn't we just stick with one of them? Specially for the php.ini settings, or we allow them via parameters or we allow them via this (entrypoint init).
  2. About this PR:
    1. Why "entrypoint.initdb.d" ? Shouldn't it be "entrypoint.php.d" or, simply "entrypoint.d" and done? I've seen the "initdb" ones are basically standard in databases (mysql, postgres, sqlsrv, solr...). But in my brain it sounds strange when applied to php/apache.
    2. Maybe we could add a test with a couple of simple entrypoints, easily verificable?

I'll continue with #163 while discuss about the points above... ciao :-)

This change makes it much faster to make smaller changes to the image
by:
- moving the ADD commands to more targeted additions in order of build
  size
- breaking out the Oracle and sqlsrv builds into their own ADD and RUN
  sections in the Dockerfile
- moving the addition of non build-related files to right at the end of
  the Dockerfile

These changes mean that it is possible to more easily developer the
image, for example:
- you can now make changes to files within the /usr directory without
  recompiling all PHP extensions
- you can now iterate on the Oracle and/or sqlsrv extensions without
  recompiling all PHP extensinos
- you can now iterate on the sqlsrv extension, without recompiling all
  PHP extensions

Whist this has little effect on the end image, or the build process
within CI systems, local development is substantially improved (unless
you're making changes to the php-extensions.sh script).
@andrewnicols andrewnicols force-pushed the entrypoint branch 3 times, most recently from 0a88c65 to ec74197 Compare March 16, 2023 02:03
@andrewnicols
Copy link
Contributor Author

  1. Overall comment: Aren't we adding "too many" overlapping ways to do the same? I mean, shouldn't we just stick with one of them? Specially for the php.ini settings, or we allow them via parameters or we allow them via this (entrypoint init).

I can understand this feeling, but at the same time I don't think that we are.

Having the ability to override the standard entrypoint (provided by the upstream php image) is a requirement for adding #167 in some sense, but using an entrypoint to change a single ini file is quite a heavy-handed approach.

These options are not required, but they're also not mutually exclusive. You may wish to have a standard set of shell scripts and ini files that you wish to use, but also have a need to tweak the ini setting as a one-off change. Or you may want to only change one ini file, and it's significantly easier to do so by specifying a single environment variable than by creating a folder, putting a .ini file in it, and mounting it.

  1. Why "entrypoint.initdb.d" ? Shouldn't it be "entrypoint.php.d" or, simply "entrypoint.d" and done? I've seen the "initdb" ones are basically standard in databases (mysql, postgres, sqlsrv, solr...). But in my brain it sounds strange when applied to php/apache.

You know, it had never occurred to me that initdb was referring to the initialisation of the DB -- I had always assumed that the folder was considered to be an initialisation database /o.

I've renamed this to /docker-entrypoint.d

  1. Maybe we could add a test with a couple of simple entrypoints, easily verificable?
    Hadn't even considered it - your wish is my command. I have added:
  • a test for the .ini in both CLI and Web
  • a test for a sourced .sh file
  • a test for an executed .sh file

@andrewnicols andrewnicols force-pushed the entrypoint branch 2 times, most recently from 8a4f26d to 7c90b9c Compare March 16, 2023 02:32
This commit adds support for run-time configuration which is executed as
part of the startup of the container.

Two options are supported:
* shell scripts; and
* .ini files for PHP configuration.

These can be placed into a new directory, located at
/docker-entrypoint-initdb.d and files are executed in lexical order
returned by a bash glob.
@andrewnicols
Copy link
Contributor Author

I was just thinking about this some more and have some further use-cases for either entrypoint and/or simplified INI:

  • we should generate and install xdebug, but not enable it. This will allow people to enable it via either an entrypoint, a shipped entrypoint, or an INI var
  • ditto tideways

Copy link
Member

@stronk7 stronk7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks!

Will merge this soon (once the previous master multi-arch images are already published).

And then will backport this to as many php versions as possible. Surely 7.4 and up (like it's being done for #163). That should be enough.

@stronk7 stronk7 merged commit 43f4add into moodlehq:master Mar 16, 2023
@stronk7
Copy link
Member

stronk7 commented Mar 16, 2023

(done, all >= 7.4 images now support the entrypoints, yay!)

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

Successfully merging this pull request may close these issues.

2 participants