Single Dockerfile using multi-stages for production and dev image #751

AlMaVizca · 2023-11-19T14:31:42Z

This proposal is reorganizing the instructions to get advantage of the Docker cache, and handle a single file for images.

Previous size:

~/R/A/farmOS ❯❯❯ docker images | /usr/bin/grep farmos
farmos/farmos                     3.x-dev              979ee3ee7cbe   12 days ago         1.36GB
farmos/farmos                     3.x                  601095e643d1   12 days ago         966MB

New image size:

~/R/A/farmOS ❯❯❯ docker images | /usr/bin/grep farmos
farmos/farmos                     3.x-dev              c6a35b8434fe   6 minutes ago       835MB
farmos/farmos                     3.x                  6c93b1019a43   2 hours ago         796MB

mstenta · 2023-11-27T19:38:47Z

Thanks for starting this @AlMaVizca!

I haven't had a chance to dig in yet, but at first glance it looks like a large number of changes in a small number of commits, and not a lot of explanation or reasoning is included in the commit messages. This makes it harder to review, because it means maintainers need to keep a lot of considerations in their head, and make guesses at your intentions or reasoning. It would be great if you could break this up so that each change can be reviewed in smaller bites, with each commit focused on a single change.

The first commit is pretty good as-is ("Define baseimage with arguments required in multiple stages"), but the second and third commits ("Production image" and "Development image", respectively) are a lot more opaque and intimidating to review. They also seem to be making a few new additions (eg: introducing COMPOSER_ALLOW_SUPERUSER=1), which should have their own commits with summaries of why they are being added. This may mean adding more "preparation" commits before the actual changes you want to make.

The end-goal of this "many small commits" approach is that we have a more granular history of changes, and the reasoning behind them, in the Git history. I use the "blame" features of Git a lot when I'm trying to remember why a change was made.

To illustrate, compare these two blames:

https://github.com/farmOS/farmOS/blame/845ee7f6cb79f65525c88e85e69ddb133dba35a0/docker/Dockerfile

https://github.com/AlMaVizca/farmOS/blame/1d905ca31150eb5294d465753a1a9643f12608b6/docker/Dockerfile

In the latter, large chunks of the history just say "Production image" or "Development image". Ideally there would be more granular changes with more descriptive messages in that blame.

Hope that all makes sense! If you have the time/interest in reformatting please do! Otherwise I will get around to picking this apart myself in more detail sometime in the future. :-)

AlMaVizca · 2023-12-14T15:36:02Z

I am not sure if it could be split a bit further. I hope this works.

I'm seeing that it failed the check of MariaDB due to connectivity issue:

<ul><li>Failed to connect to your database server. The server reports the following message: 
<em class="placeholder">
SQLSTATE[HY000] [2002] Connection refused [Tip: This message normally means that 
there is no MySQL server running on the system or that you are using an incorrect host name 
or port number when trying to connect to the server. You should also check that the TCP/IP port 
you are using has not been blocked by a firewall or port blocking service.] 
</em>.
<ul><li>Is the database server running?</li><li>Does the database exist or does the database user have 
sufficient privileges to create the database?</li>
<li>Have you entered the correct database name?</li><li>Have you entered the correct username and password?</li><li>Have you entered the correct database hostname and port number?</li></ul></li></ul>

The execution of the same actions on my account worked fine:
Merge development image into a single Dockerfile

Is there a way to trigger this workflow again?

wotnak · 2023-12-21T19:43:28Z

docker/Dockerfile

+
+# Set the farmOS and composer project repository URLs and versions.
+ARG FARMOS_REPO=https://github.com/farmOS/farmOS.git
+ARG FARMOS_VERSION=3.x


Moving all build args to the first stage on which all others depend will mean that if any of their values will change, the entire build cache will be invalidated. This is especially important for FARMOS_VERSION and FARMOS_REPO since these can be different for each pr which will prevent using build cache to speedup github actions workflow.

Also, if you want to make the build args available in all build stages they can just be placed before first FROM, no need to create a separate stage for them.

So to allow for efficient use of build cache I would move them to as late stage as possible, so that when they change they invalidate as little build cache as possible.

Nice catch, I'm moving the arguments to the compose-file stage.
The idea of the baseimage stage is to facilitate the upgrade of Drupal version.

docker/Dockerfile

+# Set the COMPOSER environment variables.
+ENV COMPOSER_HOME=tmp \
+    # Set the COMPOSER_MEMORY_LIMIT environment variable to unlimited.
+    COMPOSER_MEMORY_LIMIT=-1 \
+    # Allow root to install plugins.
+    COMPOSER_ALLOW_SUPERUSER=1


docker/Dockerfile

-ARG PROJECT_REPO=https://github.com/farmOS/composer-project.git
-ARG PROJECT_VERSION=3.x
+# Set the COMPOSER environment variables.
+ENV COMPOSER_HOME=tmp \


wotnak · 2023-12-21T21:46:02Z

docker/Dockerfile

+RUN composer install
+
+# Set the version in farm.info.yml
+RUN sed -i "s|version: 3.x|version: ${FARMOS_VERSION}|g" ${FARMOS_PATH}/web/profiles/farm/farm.info.yml


farmos-dev-sources is almost the same as farmos-sources, maybe it could be based on farmos-sources. This would allow avoiding duplication of setting version in farm.info.yml and would speed up composer installation since prod dependencies would be installed only once in farmos-sources instead of in both stages separately.

It could look something like:

## # Create layer with farmOS sources. FROM composer-file as farmos-sources # Install php dependencies. RUN composer install --no-dev --no-interaction # Set the version in farm.info.yml. RUN sed -i "s|version: 3.x|version: ${FARMOS_VERSION}|g" ${FARMOS_PATH}/web/profiles/farm/farm.info.yml ## # Create layer with farmOS dev sources. FROM farmos-sources as farmos-dev-sources # Install php dev dependencies. RUN composer install --dev --no-interaction

Ideally, that would be the case.
I tried that approach before, and I forgot the reason, yet I was able to replicate it again.

If you take a look at the build-farmOS.sh script, the installation of the sources is made using composer project and some manual modifications. Therefore, trying to reuse this stage is throwing and error with this message:
This usually happens when composer files are incorrectly merged or the composer.json file is manually edited.

You can take a look related to these manual modifications in this thread

Also, while the Drupal image is still using composer 2.6.5, the --dev argument is being deprecated on Composer 3.

It can be done using composer update instead of composer install. Since composer install --no-dev in farmos-sources stage already installed the latest versions of production dependencies, composer update in farmos-dev-sources won't do anything with them, but it will install missing dev dependencies and will update composer.lock to include them.
dda987c
https://github.com/wotnak/farmOS/actions/runs/7307749361/job/19913727994

docker/Dockerfile

+FROM composer-file as farmos-dev-sources
+
+# Install sources.
+RUN composer install


wotnak · 2023-12-21T22:17:34Z

docker/Dockerfile

+# Install apt dependencies.
+RUN apt-get update && apt-get install -y \
+    # Install git and unzip (needed by Composer).
+    git unzip


git and unzip are currently installed and available only in stages related to building farmOS. In my opinion that they should also be available at least in the final dev version of the image but probably also in the final prod image since they are required to install additional modules using composer which is a workflow available in current image versions, so removing it here would be a breaking change.

Also, when installing them in the stage that will be included in the final image, it would be good to add --no-install-recommends option to the apt-get install command to skip installing additional packages that are not really needed and only take up space in the image. It is a good practice to use this option in all instances of apt-get in docker images.

git and unzip are currently installed and available only in stages related to building farmOS. In my opinion that they should also be available at least in the final dev version of the image but probably also in the final prod image since they are required to install additional modules using composer which is a workflow available in current image versions, so removing it here would be a breaking change.

Can you, please, share a module that requires some of those packages?
I've tried with a default installation, also adding modules that are displayed during setup, and later on /setup/modules and they were installed without errors.

I understand that those packages might be useful for development image. Yet, for production, I think that adding any module without creating a new image for distribution, or without custom shared volumes, might create a scenario where if the container is replaced, it will lose functionality.
Since there is no documentation about shared volumes to persist modules information, I think, it is not a case to resolve in this PR.

Also, when installing them in the stage that will be included in the final image, it would be good to add --no-install-recommends option to the apt-get install command to skip installing additional packages that are not really needed and only take up space in the image. It is a good practice to use this option in all instances of apt-get in docker images.

I've tried to minimize the number of changes that has to do with packages installations, and propose a different structural organization.
I took the time to evaluate which packages are installed by recommendations, and only git and unzip are adding extra packages. Would it be enough to add them only to the dev image? Is it worth it?

I was thinking of community modules, not included in the farmOS itself. For example https://github.com/symbioquine/farmOS_asset_link or https://www.drupal.org/project/farm_map_custom_layers.

On the Building farmOS with Composer documentation page, there is described approach for adding additional community modules to the composer.json and building docker image based on official production one with these customizations. Currently, in instructions for building a custom image when it comes to installing composer dependencies only composer install is provided, without git and unzip in the prod image these instructions will no longer work and will need to be updated to also include installation of git and unzip. So not including git and uzip in production image may break some existing setups when the new farmOS base image version will be pulled.

I would include git and unzip in both dev and prod image to don't break existing documented functionality.

Ok, although, I think that it would be more appropriate to add the information on the documentation, so the image is kept as slim as possible, It's the right thing to keep it working as it was.
Thanks for the explanation.

docker/build-farmOS.sh

-# Remove the Composer cache directory.
-rm -rf "$COMPOSER_HOME"
+# Create folder for composer installations.
+mkdir -p ${FARMOS_PATH}/web/sites/simpletest/browser_output


docs/development/environment/docker.md

+To build the development image, you will have to define the target farmos-dev,
+for example:
+
+`docker build --build-arg WWW_DATA_ID=$(id -u)  -t farmos/farmos:3.x-dev --target farmos-dev docker`


mstenta · 2023-12-22T20:45:41Z

Thanks for reviewing @wotnak!

I'm seeing that it failed the check of MariaDB due to connectivity issue

@AlMaVizca if you rebase your branch onto latest 3.x and force-push then this issue should be fixed. @wotnak fixed it recently in #758.

wotnak · 2023-12-23T10:21:26Z

docker/Dockerfile

+# Install apt dependencies.
+RUN apt-get update && apt-get install -y \
+    # Install git and unzip (needed by Composer).
+    git unzip


I was thinking of community modules, not included in the farmOS itself. For example https://github.com/symbioquine/farmOS_asset_link or https://www.drupal.org/project/farm_map_custom_layers.

On the Building farmOS with Composer documentation page, there is described approach for adding additional community modules to the composer.json and building docker image based on official production one with these customizations. Currently, in instructions for building a custom image when it comes to installing composer dependencies only composer install is provided, without git and unzip in the prod image these instructions will no longer work and will need to be updated to also include installation of git and unzip. So not including git and uzip in production image may break some existing setups when the new farmOS base image version will be pulled.

I would include git and unzip in both dev and prod image to don't break existing documented functionality.

wotnak · 2023-12-23T10:50:55Z

docker/Dockerfile

+RUN composer install
+
+# Set the version in farm.info.yml
+RUN sed -i "s|version: 3.x|version: ${FARMOS_VERSION}|g" ${FARMOS_PATH}/web/profiles/farm/farm.info.yml


It can be done using composer update instead of composer install. Since composer install --no-dev in farmos-sources stage already installed the latest versions of production dependencies, composer update in farmos-dev-sources won't do anything with them, but it will install missing dev dependencies and will update composer.lock to include them.
dda987c
https://github.com/wotnak/farmOS/actions/runs/7307749361/job/19913727994

wotnak · 2023-12-28T14:22:31Z

docs/development/environment/docker.md

@@ -26,3 +28,8 @@ The `3.x-dev` image also provides the following:
   container. If your user ID is not `1000`, build the image with:
   `--build-arg WWW_DATA_ID=$(id -u)`
    - Default: `1000`
+
+To build the development image, you will have to define the target farmos-dev,


Suggested change

To build the development image, you will have to define the target farmos-dev,

To build the development image, you will have to define the target dev,

wotnak · 2023-12-28T14:43:26Z

docker/Dockerfile

+RUN usermod -u ${WWW_DATA_ID} www-data && groupmod -g ${WWW_DATA_ID} www-data
+
+# Add farmOS sources
+COPY --from=composer-file --chown=www-data:www-data ${FARMOS_PATH} ${FARMOS_PATH}


This line can be removed, few lines below dev sources are added that already include the composer.json file generated in composer-file stage.

wotnak

Looks good.
Merging this will allow configuring docker image build cache in GitHub actions, which will improve their execution time.

mstenta · 2024-01-21T14:24:30Z

Thanks @AlMaVizca @wotnak! I haven't forgotten about this... I've just had some deadlines this month that needed my attention. I hope to get back to this and review it again soon. In the meantime I'll tag a few other folks in case they have some time/interest in reviewing.

Change versions, add common paths Keep drupal path Fix comment

Use common path on entrypoint

Use docker stages to install dependencies and simplify the building script. Prepare production image while keeping compatibility with the development image. Fix typo Relocate arguments to improve cache Make single line for each env definition Remove composer home path Avoid installing recommended packages

Changes based on PR review

AlMaVizca · 2024-04-26T11:02:23Z

I've tested the code at 3.x branch and the same error is happening.
Please let us know if we can help with anything else.

AlMaVizca mentioned this pull request Nov 19, 2023

Single Dockerfile using multi-stages for production and dev image #748

Closed

AlMaVizca force-pushed the 3.x-docker-multi-stage branch 2 times, most recently from 4048eb7 to 1d905ca Compare November 20, 2023 09:36

AlMaVizca force-pushed the 3.x-docker-multi-stage branch from 1d905ca to f1997a6 Compare December 14, 2023 15:04

wotnak mentioned this pull request Dec 15, 2023

Add healthcheck to MariaDB container to ensure it is ready to accept external connections before reporting as ready #758

Merged

wotnak suggested changes Dec 21, 2023

View reviewed changes

AlMaVizca force-pushed the 3.x-docker-multi-stage branch from f1997a6 to e5caa98 Compare December 23, 2023 07:54

wotnak suggested changes Dec 23, 2023

View reviewed changes

AlMaVizca force-pushed the 3.x-docker-multi-stage branch from e5caa98 to 777fe4a Compare December 24, 2023 04:05

wotnak suggested changes Dec 28, 2023

View reviewed changes

AlMaVizca force-pushed the 3.x-docker-multi-stage branch from 777fe4a to 3774afe Compare December 29, 2023 16:15

wotnak approved these changes Jan 1, 2024

View reviewed changes

wotnak mentioned this pull request Jan 1, 2024

Use docker image build cache in github actions #764

Draft

mstenta requested review from paul121 and symbioquine January 21, 2024 14:24

AlMaVizca added 5 commits April 26, 2024 17:26

Define baseimage with arguments required in multiple stages

8fe46ef

Change versions, add common paths Keep drupal path Fix comment

Avoid repeating common path

ee9f49b

Use common path on entrypoint

Relocate php unit configuration

682fdbb

Merge development image into a single Dockerfile

8320482

Changes based on PR review

AlMaVizca force-pushed the 3.x-docker-multi-stage branch from 3774afe to 8320482 Compare April 26, 2024 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Single Dockerfile using multi-stages for production and dev image #751

Single Dockerfile using multi-stages for production and dev image #751

AlMaVizca commented Nov 19, 2023 •

edited

mstenta commented Nov 27, 2023 •

edited

AlMaVizca commented Dec 14, 2023 •

edited

wotnak Dec 21, 2023

AlMaVizca Dec 22, 2023

This comment was marked as resolved.

This comment was marked as resolved.

wotnak Dec 21, 2023

AlMaVizca Dec 23, 2023

wotnak Dec 23, 2023

This comment was marked as resolved.

wotnak Dec 21, 2023

wotnak Dec 21, 2023

AlMaVizca Dec 22, 2023

AlMaVizca Dec 22, 2023

wotnak Dec 23, 2023

AlMaVizca Dec 24, 2023 •

edited

This comment was marked as resolved.

This comment was marked as resolved.

mstenta commented Dec 22, 2023

wotnak Dec 23, 2023

wotnak Dec 23, 2023

wotnak Dec 28, 2023

wotnak Dec 28, 2023

wotnak left a comment

mstenta commented Jan 21, 2024

AlMaVizca commented Apr 26, 2024

	To build the development image, you will have to define the target farmos-dev,
	To build the development image, you will have to define the target dev,

Single Dockerfile using multi-stages for production and dev image #751

Are you sure you want to change the base?

Single Dockerfile using multi-stages for production and dev image #751

Conversation

AlMaVizca commented Nov 19, 2023 • edited

mstenta commented Nov 27, 2023 • edited

AlMaVizca commented Dec 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlMaVizca Dec 24, 2023 • edited

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

mstenta commented Dec 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wotnak left a comment

Choose a reason for hiding this comment

mstenta commented Jan 21, 2024

AlMaVizca commented Apr 26, 2024

AlMaVizca commented Nov 19, 2023 •

edited

mstenta commented Nov 27, 2023 •

edited

AlMaVizca commented Dec 14, 2023 •

edited

AlMaVizca Dec 24, 2023 •

edited