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

RFC: use upstream PHP base image #348

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

RFC: use upstream PHP base image #348

wants to merge 3 commits into from

Conversation

J0WI
Copy link
Contributor

@J0WI J0WI commented Jul 6, 2019

I propose to use the PHP image of the docker library: https://hub.docker.com/_/php

As a first step, this PR keeps the current concept with nginx and supervisor running the same container. I'd also like to introduce some best practices for writing Dockerfiles.
My goal is to get rid of supervisor completely and providing a pure FPM variant and a variant using php-apache as an alternative. This concept is inspired by other well known PHP images like Wordpress, Drupal, Joomla, Nextcloud, Matomo etc. They all run and scale pretty well in production.

You can find more information about the docker library and how to become an "official image" yourself here:

Please let me know how you think about it and if I should continue with this. Feel free to reach out if you have any questions.

@J0WI J0WI requested a review from a team July 6, 2019 23:01
@djdefi
Copy link
Contributor

djdefi commented Jul 7, 2019

Thanks for opening this and getting it started. I have had a similar idea for quite some time now to attempt to simplify / use more "off the shelf" components, so this is a welcome proposal.

I am interested to see if there are any comments from the greater CachetHQ community on this one, I'm not sure if there are any strong preferences either way :)

@J0WI
Copy link
Contributor Author

J0WI commented Jul 23, 2019

No further comments so far. How should I proceed with this?

@djdefi
Copy link
Contributor

djdefi commented Dec 4, 2019

@J0WI Sorry for the long delay in responding.

It seems like this would be a welcome change and refresh, if you are still interested. As you may have noticed, the myself and the other current maintainers of this docker image are not super active here.

@almereyda
Copy link

almereyda commented Mar 30, 2020

For me it appears useful to ditch nginx and supervisor from this image completely, and rather integrate nginx with a dedicated nginx.conf through docker compose.

This also allows to keep up to date with newer Nginx versions more easily.

Examples can be taken from

Then the examples could also note how to configure the queue, a suiting database (Redis, Memcached, ...) and how to run workers in a separate container.

@J0WI
Copy link
Contributor Author

J0WI commented Mar 30, 2020

Agreed. But this would result in a whole rewrite of the Dockerfile and break everything. I wanted to start with this part first. If you prefer to do the rewrite in a single step I can push all changes to this PR.

@djdefi
Copy link
Contributor

djdefi commented Apr 1, 2020

I believe the current test failure is due to a missing mysql-client package for the MySQL DB tests.

@J0WI
Copy link
Contributor Author

J0WI commented Apr 7, 2020

@djdefi thanks for the hint, all tests are now passing.

Copy link
Contributor

@djdefi djdefi left a comment

Choose a reason for hiding this comment

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

Giving this a 👍 for the initial pass.

I do think that simplifying to a pattern with a single entrypoint process more similar to the wordpress container as @almereyda mentioned should be an eventual goal.

@@ -13,7 +13,7 @@ services:
build:
context: .
args:
- cachet_ver=2.4
- cachet_ver=2.3.18
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity does 2.4 pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't build, because there is no downloadable release.

Copy link
Contributor

@djdefi djdefi Apr 7, 2020

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't work for releases anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are other breaking changes like app:install vs cachet:install

WORKDIR /var/www/html/
USER 1001
ARG cachet_ver=2.3.18
ARG archive_url=https://github.com/CachetHQ/Cachet/archive/v${cachet_ver}.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ARG archive_url=https://github.com/CachetHQ/Cachet/archive/v${cachet_ver}.tar.gz
ARG archive_url=https://github.com/CachetHQ/Cachet/archive/${cachet_ver}.tar.gz

related to convo in #348 (comment)

@J0WI J0WI requested a review from djdefi April 16, 2020 17:47
Base automatically changed from master to main January 15, 2021 06:23
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.

3 participants