refacto(docker): Improve DX + new Dockerfile with target #1764

mihani · 2025-05-04T13:35:18Z

Dans cette PR, il y la réécriture du Dockerfile pour avoir plusieurs target en fonction de l'environnement. Grâce à ce changement, il y a un ajout de docker-entrypoint pour pouvoir lancer un container avec le user localUser et donc simplifier les commandes du Makefile.

Ajout d'un docker.env(.local) qui permet de gérer les variables d'environnment pour le container de dev pour XDEBUG.

Ajout de plusieurs commandes make pour installer et reset le projet.

Ajout de commande dans package.json pour simplifier les commande node pour ce qui tourne autour des assets.

Concernant la target de prod :

Build - ok
Running - ok

Je propose cette refacto, donc si quelque chose ne vous convient pas ou si vous avez des remarques n'hésitez pas j'ai peut être loupé des informations.

agallou · 2025-05-08T09:32:46Z

.docker/php/docker-entrypoint.sh

+
+    if [ "$APP_ENV" != 'prod' ]; then
+        if [ -d .git ]; then
+          git config --global --add safe.directory /var/www/html


pourquoi y a t-il besoin de faire cela ?

N'étant pas propriétaire du dossier dans le container du coup il y a un conflit de secu entre mon .git le dossier du projet. Pour éviter ce warning qui n'existe qu'en dev je lance cette commande.

agallou · 2025-05-08T09:33:27Z

.editorconfig

@@ -0,0 +1,10 @@
+[*]


est-ce que cela ne serait-il pas à mettre dans place dans une autre PR ?

Oui pas de soucis.

agallou · 2025-05-08T09:34:32Z

.github/workflows/ci.yml

      - name: Tests - Functional
-        run: make test-functional
+        run: |


pourquoi avoir décomposé le lancement des tests ? Pour moi dans le fichier de configuration des actions/de la CI, on devrait avoir le moins de logique de lancement possible. Je pense qu'o devrait conserver le make test-functional.

De mon point de vue, justement la CI doit être claire sur ce quelle fait sans devoir aller voir dans un autre fichier. D'autant plus que si on modifie une commande make pour des besoins en dev ça impact la CI sans le vouloir ou sans s'en rendre compte ce qui complexifie la lecture du projet. En plus ça complexifie aussi la compréhension du make file car il n'y a aucun moyen de savoir quelle commande est à exécuter et où ce qui peut être un frein pour de nouveau contributeur peut importe l'expérience de ce dernier.

agallou · 2025-05-08T09:35:03Z

.github/workflows/ci.yml

      - name: Tests - Integration
-        run: make test-integration-ci
+        run: |


même remarque, on a un outil pour le lancer autant rester sur cette configuration dans le Makefile

De mon point de vue, justement la CI doit être claire sur ce quelle fait sans devoir aller voir dans un autre fichier. D'autant plus que si on modifie une commande make pour des besoins en dev ça impact la CI sans le vouloir ou sans s'en rendre compte ce qui complexifie la lecture du projet. En plus ça complexifie aussi la compréhension du make file car il n'y a aucun moyen de savoir quelle commande est à exécuter et où ce qui peut être un frein pour de nouveau contributeur peut importe l'expérience de ce dernier.

agallou · 2025-05-08T09:37:06Z

Dockerfile

+COPY --link ./composer.* ./
+
+RUN set -eux; \
+    composer install --no-cache --prefer-dist --no-dev --no-autoloader --no-scripts --no-progress


pourquoi mettre le composer install dans le Dockerfile ? ici on est sur une image pour du dev pas de la prod (on utilisera pas docker pour la prod), ça n'est pas son rôle.

Pour la target de prod ça à du sens de composer install pour start des images avec déjà les dépendances d'installer. Mais si il n'y a pas de docker en prod en effet cette target ne sert à rien. Je l'enlève.

agallou · 2025-05-08T09:37:56Z

Dockerfile

+
+RUN set -eux; \
+    npm install --legacy-peer-deps; \
+    npm run build;


même remarque pour les assets ? sur une image pour de la prod ça peux avoir du sens, ici ce n'est pas le but de l'image. On doit pouvoir rebuilder les assets sans devoir rebuilder l'image de dev, autant le faire en dehors du build de l'image docker.

J'enlève la target de prod vu qu'elle ne sert à rien.

agallou · 2025-05-08T09:39:31Z

Makefile

+##@ Setup 📜
+### Installer le projet from scratch
+install:
+	cp -n .env.dist .env && cp -n docker.env docker.env.local && cp -n .docker/data/history.dist .docker/data/history && cp -n compose.override.yml-dist compose.override.yml


ici je pense qu'il vaux mieux continuer à tirer les bénéfices de Make. D'avoir des targets en fonction du fichier, qui sont en dépendance d'autres tagets.
Par exemple on avait :

compose.override.yml: cp compose.override.yml-dist compose.override.yml

cela permet de ne faire la copie que si le fichier n'existe pas. Autant rester sur du standard de Make / qu'on peux facilement mettre en dépendance de différentes targets.

Je sais que c'est le but premier du Makefile, mais cette stratégie complique la lecture et la compréhension du Makefile, faire des aller-retours de haut en bas du Makefile pour bien comprendre ce que fait un commande ce qui n'aide pas à la compréhension du projet dès l'install. Et du coup créer une target make juste pour cette fonctionnalité alors que le cp n'est utilisé qu'à un seul endroit et qu'il y a une option dans cp pour faire la même chose, ça augmente la complexité de compréhension pour pas grand chose.

agallou · 2025-05-08T09:42:34Z

Makefile


-## Targets cachés
-
-var/logs/.docker-build: compose.yml compose.override.yml $(shell find docker -type f)


on pers la partie cache sur le docker up up faisant cela (même sil y a le cache docker), ça me semble dommage de perdre cela.

Désolé je ne comprends pas ce qu'apporte cette commande que n'apporte pas docker et buildx directement ? Surtout que var/logs/.docker-build est vide du coup je comprends encore moins 🤔

agallou · 2025-05-08T09:43:57Z

Makefile

-node_modules:
-	npm install --legacy-peer-deps
-
-init-db:


pourquoi avoir supprimé ces différentes méthodes permettant de décomposer l'installation ?

Je ne comprends pas trop l'interet de décomposer autant, quand une commande n'est utilisé qu'à un seul moment à l'installation du projet. Pour les commandes qui peuvent être utilisé pendant le processus de dev c'est normale mais pour les commandes d'installation je ne trouve pas ça pertinent et ça complexifie la compréhension pour rien, de mon point de vue externe.

agallou · 2025-05-08T09:45:02Z

compose.yml

+    build:
+      context: .
+      target: afup_web_dev
+    container_name: afup-web-apachephp


par curiosité à quoi sert le container name ? (on a déjà un nom similaire qui est généré)

Pour justement ne pas laisser docker le généré comme il veut (souvent avec des chiffres dedans) c'est juste pour être constant sur le nommage des containers. Utile quand on a plusieurs projets d'allumer sur le même réseau pour différencier 2 services php ou nginx par exemple.

agallou · 2025-05-08T09:46:32Z

compose.yml

+    image: dockage/mailcatcher:0.9.0
+    container_name: afup-web-mailcatcher
+
+  node:


comment va se passer le build des assets en ayant ce container dédié ? ce n'est pas plus simple d'avoir un container de cli toutes les dépendances nécéssaires ?

En dev, le container va s'allumer au moment de faire la commande de build puis s'éteindra à la fin (sauf pour le watch) et vu que le mapping de volume est le même que les autres services les fichiers vont se synchroniser avec les autres container de manière transparente.

agallou · 2025-05-08T09:48:05Z

doc/dev-setup.md

@@ -36,8 +36,7 @@ Ce document contient des instructions sur la mise en place d'une instance de dé
 git clone https://github.com/my-account/afup-web.git
 ```

-3. Lancer `make docker-up` pour lancer le projet
-4. Lancer `make init` pour le setup initial (config, dépendances, base de données)
+3. Lancer `make install` pour lancer le projet


ça me semble pertinent de conserver deux étapes, le up et init pour le lancement. Pourquoi avoir tout rassemblé ? (ça permettrais de généralement faire le up, la première fois de faire l'init ou le faire si on veux remettre à zéro son env de dev).

Je trouve plus facile d'inclure de nouvelle personne dans un projet ne leur disant que tu n'as qu'une commande pour installer (si tu es curieux tu vas voir ce qu'elle fait) et de juste utiliser les commande docker de base que tu pourrais faire peut importe le projet dockerisé. Quand à remettre le projet de zéro c'est pour ça que je créait une commande reset pour vraiment enlevé les fichiers générés par le projet pour simuler un projet vierge comme si on venait de clone le projet.

agallou · 2025-05-08T09:50:04Z

var/cache/.gitkeep

pourquoi avoir supprimé ces fichiers (surtout pour le cache et log) ? si on le fait autant le faire dans une autre PR en ayant validé aussi l'impact en prod et dev.

Il se sont supprimé avec mes premiers tests de docker, je les remets.

agallou · 2025-05-08T09:52:08Z

.docker/config/bash_aliases

+# Global
+alias ll='ls -alh --color'
+alias clean-test-deprecated-log='rm -f /var/www/html/var/logs/test.deprecations.log'
+alias group-deprecation-log="touch /var/www/html/var/logs/test.deprecations_grouped.log && cat /var/www/html/var/logs/test.deprecations.log | cut -d \"]\" -f 2 | awk '{\$1=\$1};1' | sort | uniq -c | sort -nr > /var/www/html/var/logs/test.deprecations_grouped.log"


avant on avait des targets Make, ici on a la logique qui est reproduite dans l'alias et dans test-functional Pourquoi ne pas conserver les targets permettant de factoriser cela ?

ça permet de bien séparé les commandes qui se lance depuis le container (donc qui sont dans bash_aliases) et ceux qui se lance depuis l'extérieur de container (Makefile)

agallou · 2025-05-08T09:54:33Z

docker/dockerfiles/apachephp/Dockerfile

@@ -1,56 +0,0 @@
-FROM php:8.2-apache


pourquoi remonter ce container à la racine ? (du coup on a un container à la racine et ceux de MySQL / des ressources statiques dans docker/dockerfiles. On ne pourrait pas le laisser dans le dossier dockerfiles ? ça me semblerais plus cohérent de tous les y retrouver.

Vu qu'il y avait l'utilisation d'un container node ce n'était plus pertinant de le mettre dans le dossier apachephp mais je remet dans le dossier docker.

ternel · 2025-05-30T08:55:19Z

J'ai regardé vite fait, et ça m'a l'air d'être un super première étape pour rendre les dockers / makefile plus au gout du jour et plus simple que ce que c'est actuellement :)

Je dis "première étape", parce que j'ai l'impression qu'on pourra encore améliorer la DX et simplifier tout ça !

Bravo @mihani !

…dd node container + Add new npm command to simplify Makefile

…d doc to config xdebug and use breakpoint

…akefile (test part) + Add bash alias in container

…ix prod php ini

mihani force-pushed the refacto/docker-to-improve-dx branch 3 times, most recently from f960142 to c5c53dc Compare May 4, 2025 18:12

mihani marked this pull request as draft May 4, 2025 18:20

mihani force-pushed the refacto/docker-to-improve-dx branch 9 times, most recently from aa43be8 to 5560cd1 Compare May 7, 2025 19:00

mihani marked this pull request as ready for review May 7, 2025 19:10

agallou reviewed May 8, 2025

View reviewed changes

mihani force-pushed the refacto/docker-to-improve-dx branch from 51b4008 to 4630b65 Compare May 24, 2025 08:10

mihani force-pushed the refacto/docker-to-improve-dx branch 2 times, most recently from 02ac88e to 8f1d084 Compare May 24, 2025 10:43

mihani requested a review from agallou May 24, 2025 11:04

mihani force-pushed the refacto/docker-to-improve-dx branch 5 times, most recently from a9579b8 to a02c44f Compare May 24, 2025 12:41

mihani added 5 commits June 9, 2025 10:45

refacto(docker): Add docker php config file + Init new Dockerfile + A…

c246292

…dd node container + Add new npm command to simplify Makefile

update: dev doc + Makefile (only development part not test part) + Ad…

97fb858

…d doc to config xdebug and use breakpoint

refacto(docker): Add new apache dockerfile for test stack + Refacto m…

c0ef5cd

…akefile (test part) + Add bash alias in container

refacto(docker): Init .dockerignore + Fix the version of composer + f…

df01f36

…ix prod php ini

fix(ci): Fix CI with new Dockerfile

b1af0b6

mihani force-pushed the refacto/docker-to-improve-dx branch 6 times, most recently from ea16663 to f93d409 Compare June 9, 2025 17:41

refacto: Move some file to match with comment

41913bb

mihani force-pushed the refacto/docker-to-improve-dx branch from f93d409 to 41913bb Compare June 9, 2025 19:26


		## Targets cachés

		var/logs/.docker-build: compose.yml compose.override.yml $(shell find docker -type f)

refacto(docker): Improve DX + new Dockerfile with target #1764

Are you sure you want to change the base?

refacto(docker): Improve DX + new Dockerfile with target #1764

Uh oh!

Conversation

mihani commented May 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mihani May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ternel commented May 30, 2025

Uh oh!

Uh oh!

mihani May 9, 2025 •

edited

Loading