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 .env file for database configuration #2887

Open
wants to merge 1 commit into
base: alpha
Choose a base branch
from

Conversation

kwizer15
Copy link
Contributor

@kwizer15 kwizer15 commented Sep 8, 2024

Déplace la configuration de la base de donnée dans un fichier .env (surchargeable par .env.local).

Description

Cette MR est rétrocompatible, on priorise le nouveau système mais accepte le fichier /core/config/common.config.php en renvoyant une deprecated si le fichier .env est absent ou ne contient pas la configuration base de donnée.

Le fichier .env peut contenire 2 variable d’environnements :

DEBUG=0
DATABASE_DSN=mysql://user:password@localhost:3306/database_name

en pratique le fichier généré par l’installation sera le suivant

DATABASE_DSN=mysql://jeedom:password@localhost:3306/jeedom

Suggested changelog entry

Move database configuration in /.env file.

Types of changes

  • Bug fix (non-breaking change which fixes)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

@kwizer15 kwizer15 force-pushed the feat/add-env-file branch 4 times, most recently from 39190c2 to 03a8b91 Compare September 8, 2024 16:21
@zoic21
Copy link
Contributor

zoic21 commented Sep 8, 2024

Bonjour,
Je comprends pas trop le but la ? Et j'avoue que les dernier PR sur la partie docker ont a chaque fois tout cassé...

@kwizer15
Copy link
Contributor Author

kwizer15 commented Sep 8, 2024

Le .env est un standard.
Ca permet de surcharger ta config facilement si tu veux avoir plusieurs bdd par exemple. C'est dispo a la racine et pas au milieu du code.
La PR ne casse strictement rien pour l'existant.
L'idée dernière c'est de pour faire un environnement de test auto (qui sont laissé a l'abandon).
Le seul moyen d'avoir une appli sans trop de regression ce sont les tests, mais je veux pas faire péter les bases de dev a chaque lancement.
Voili voilou...

@zoic21
Copy link
Contributor

zoic21 commented Sep 9, 2024

Je vois pas trop l'interet on peut deja passer tout le parametrage en variable. C'est d'ailleurs ce qu'on fait nous chez jeedom pour tester le core.

@kwizer15
Copy link
Contributor Author

kwizer15 commented Sep 9, 2024

Pardon mais j’ai du mal a comprendre ta réponse. Je parle d’avoir un environnement de dev dans lequel on peut lancer les tests phpunit en utilisant un bdd différente que celle du dev.

D’ailleurs si c’est possible, ça serait intéressant de documenter comment vous faites vos environnements de dev parce que la gestion des droits sur les fichiers est ingérable chez moi.

@zoic21
Copy link
Contributor

zoic21 commented Sep 9, 2024

Actuellement on a une action github pour le core qui s'occupe de checker que tout va bien en buildant le docker et en lancer le sick.php. C'est très léger mais ca suffit en général.

Pour les plugins il n'y a rien si ce n'est une validation de la syntaxe.

Après oui il y aurait un gros boulot pour mettre des vrais tests c'est sur mais malheureusement on a pas les moyens humain.

Pour ta modification je suis mitigé je vois pas le but au final. C'est pour changer de BDD en changeant juste un fichier .env ?

@kwizer15
Copy link
Contributor Author

kwizer15 commented Sep 9, 2024

La configuration de base de donnée est une configuration d’environnement. Ce fichier permet de définir les variables d’environnement de ton application.
L’idée est donc de remplacer le fichier de config original common.config.php par ce fichier .env.
On peut rajouter un fichier .env.local qui permet de surchargé le .env (par exemple si tu veux mettre le DEBUG à 1, sans modifier ta conf original qui reste dans .env)
Ce que j’ai prévu dans une prochaine PR, c’est de rajouter un fichier .env.test qui ne sera pris en compte QUE par phpunit (via une variable d’environnement forcée dans la config de phpunit).
Ça veut dire que quand tu lanceras les tests unitaire, tu n’auras strictement aucun impact sur ta bdd de dev.

Je te renvoi vers cet article https://www.armandphilippot.com/article/dotenv-variables-environnement si tu veux en savoir plus, mais c’est aujourd’hui plus que courament utilisé dans le dev, et pas que en PHP. C’est même directement pris en compte par docker-compose.

@edgd1er
Copy link
Contributor

edgd1er commented Sep 9, 2024

Je parlerais que de l'image docker.

Actuellement, le common.config.php est alimenté par les variables d'environnements, le docker compose quand il lit les variables d'environnements, les crée et les valorise dans le conteneur en execution. Ce qui permet justement de "brancher" le conteneur docker si differentes BDD, pour tester par exemple.

Donc concernant l'image docker que les infos viennent du .env ou de la configuration de lancement de l'image c'est la meme.
La confusion pourrait arriver si, lors du build, en copiant/clonant le jeedom/core dans l'image docker un fichier .env existe dans le projet, et que le php lise en priorirté ce .env et non plus les variables d'environnement. Pour éviter cet écueil, il est possible d'aller réécrire le .env comme le commong.config.php est réécrit lors de l'execution du init.sh.

@kwizer15
Copy link
Contributor Author

kwizer15 commented Sep 9, 2024

Actuellement, le common.config.php est alimenté par les variables d'environnements, le docker compose quand il lit les variables d'environnements, les crée et les valorise dans le conteneur en execution. Ce qui permet justement de "brancher" le conteneur docker si differentes BDD, pour tester par exemple.

Le build crée le fichier common.config.php avec des paramèters par défaut et un mot de passe généré aléatoirement (il en est de même pour le moment avec le .env sur ma PR). Ça ne permet pas de modifier ces variables à la volée sans rebuild l’image, seulement en modifiant le fichier.
Si vous parlez bien des variables suivantes de l’image docker

ENV DB_USERNAME=jeedom
ENV DB_NAME=jeedom
ENV DB_PORT=3306
ENV DB_HOST=localhost

Je ne les ai vu utilisée à aucun endroit. Ni dans le common.config.(sample.).php, ni dans l’install.sh. Si j’ai manqué quelque chose, je veux bien les infos.

La confusion pourrait arriver si, lors du build, en copiant/clonant le jeedom/core dans l'image docker un fichier .env existe dans le projet, et que le php lise en priorirté ce .env et non plus les variables d'environnement. Pour éviter cet écueil, il est possible d'aller réécrire le .env comme le commong.config.php est réécrit lors de l'execution du init.sh.

C’est ce qui est fait (en tout cas j’ai gardé le même comportement par soucis de rétrocompatibilité) et actuellement les 2 fichiers co-existent, le commong.config.php reprenant la main en cas de suppression du .env

@edgd1er
Copy link
Contributor

edgd1er commented Sep 9, 2024

Désolé, je fais court: le init.sh de l'image prend en charge la valorisation du fichier de conf bdd lors du lancement.

cp ${WEBSERVER_HOME}/core/config/common.config.sample.php ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#PASSWORD#/${MYSQL_JEEDOM_PASSWD}/g" ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#DBNAME#/jeedom/g" ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#USERNAME#/jeedom/g" ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#PORT#/3306/g" ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#HOST#/localhost/g" ${WEBSERVER_HOME}/core/config/common.config.php

Avec une erreur pour le mot de passe dans la master , mais avec une PR deja envoyé pour corriger l'ano: fix #

@kwizer15
Copy link
Contributor Author

kwizer15 commented Sep 9, 2024

Désolé, je fais court: le init.sh de l'image prend en charge la valorisation du fichier de conf bdd lors du lancement.

cp ${WEBSERVER_HOME}/core/config/common.config.sample.php ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#PASSWORD#/${MYSQL_JEEDOM_PASSWD}/g" ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#DBNAME#/jeedom/g" ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#USERNAME#/jeedom/g" ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#PORT#/3306/g" ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#HOST#/localhost/g" ${WEBSERVER_HOME}/core/config/common.config.php

Avec une erreur pour le mot de passe dans la master , mais avec une PR deja envoyé pour corriger l'ano: fix #

On est d’accord que a part le mot de passe (préalablement généré), toute la config est en dur ?

@edgd1er
Copy link
Contributor

edgd1er commented Sep 9, 2024

On est d’accord que a part le mot de passe (préalablement généré), toute la config est en dur ?

le common.config.php.sample contient des valeurs en dur qui sont changées par les sed pour obtenir un fichier de configuration. common.config.php
l'image docker est un cas particulier puisque lors du lancement, il faut s'assurer que le fichier de config soit correctement valorisé a chaque lancement donc le init.sh recré le fichier common.config.php.

Dans le cas du bare-metal ou vm, l'étape 9 de l'installation (install.sh) prend en charge la valorisation du fichier commong.config.php: step_9_jeedom_configuration :

core/install/install.sh

Lines 302 to 323 in 5de9f9c

step_9_jeedom_configuration() {
echo "---------------------------------------------------------------------"
echo "${YELLOW}Starting step 9 - Jeedom configuration${NORMAL}"
if [ "${INSTALLATION_TYPE}" != "docker" ];then
echo "DROP USER 'jeedom'@'localhost';" | mariadb -uroot > /dev/null 2>&1
mariadb_sql "CREATE USER 'jeedom'@'localhost' IDENTIFIED BY '${MARIADB_JEEDOM_PASSWD}';"
mariadb_sql "DROP DATABASE IF EXISTS jeedom;"
mariadb_sql "CREATE DATABASE jeedom;"
mariadb_sql "GRANT ALL PRIVILEGES ON jeedom.* TO 'jeedom'@'localhost';"
cp ${WEBSERVER_HOME}/core/config/common.config.sample.php ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#PASSWORD#/${MARIADB_JEEDOM_PASSWD}/g" ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#DBNAME#/jeedom/g" ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#USERNAME#/jeedom/g" ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#PORT#/3306/g" ${WEBSERVER_HOME}/core/config/common.config.php
sed -i "s/#HOST#/localhost/g" ${WEBSERVER_HOME}/core/config/common.config.php
fi
chmod 775 -R ${WEBSERVER_HOME}
chown -R www-data:www-data ${WEBSERVER_HOME}
echo "${GREEN}Step 9 - Jeedom configuration done${NORMAL}"
}

Cependant, Je comprends le besoin simple de lancer des tests via des BDD de tests avec des jeux de tests différentiés et si le .env aide, cela va dans le bon sens.
il faut s'assurer que cela ne vienne pas interférer avec les mécanismes existants d'installation ou de dockerisation. :)

@kwizer15
Copy link
Contributor Author

@zoic21 est-ce que ce qui te gène c'est juste la partie docker (que je peux supprimer ca n'aura pas d'impact) ou la MR en général ?

@zoic21
Copy link
Contributor

zoic21 commented Sep 12, 2024

J'ai juste pas encore eu le temps de tester, la charge est assez importante en ce moment et docker est vraiment critique (plus qu'un bug dans le core) donc je dois passer des heures a tester les moindres changements sur docker.

@kwizer15
Copy link
Contributor Author

Si ca peut te faire gagner du temps je peux virer la partie docker sans problème, c'était juste pour être plus clean. Je le répète, le common.config.php est toujours pris en compte tant qu'on surcharge pas avec un .env . J'ai bien fais attention a ne jamais rien casser pendant mon dev.

@zoic21
Copy link
Contributor

zoic21 commented Sep 12, 2024

Faut juste je test ton PR je peux pas le passer comme ca et la je suis complètement sous l'eau donc pas avant plusieurs semaines malheureusement, j'ai pris trop de truc dernièrement et je m'en sors plus.

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