-
Notifications
You must be signed in to change notification settings - Fork 51
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
[TECH] Ajouter une valeur par défaut pour LOG_ENABLED #8845
Conversation
Une fois les applications déployées, elles seront accessibles via les liens suivants : Les variables d'environnement seront accessibles via les liens suivants : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je propose le changement suite au KO test func (voir commentaires)
@@ -21,7 +21,7 @@ const schema = Joi.object({ | |||
DOMAIN_PIX_ORGA: Joi.string().optional(), | |||
LCMS_API_KEY: Joi.string().required(), | |||
LCMS_API_URL: Joi.string().uri().required(), | |||
LOG_ENABLED: Joi.string().optional().valid('true', 'false'), | |||
LOG_ENABLED: Joi.string().required().valid('true', 'false'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Il faudra bien communiquer ce changement pour éviter des incompréhensions ! (surtout en cette forte période d'absence ne pas hésitez à en reparler)
da8046d
to
687aa9b
Compare
687aa9b
to
434ec6c
Compare
434ec6c
to
906ebf0
Compare
docker/sample.env
Outdated
@@ -340,7 +340,7 @@ LCMS_API_URL=https://lcms.minimal.pix.fr/api | |||
# presence: optional | |||
# type: Boolean | |||
# default: `true` | |||
# LOG_ENABLED=true | |||
LOG_ENABLED=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alignement de la propriété faite, mais on a un gros doute sur le fait que ce fichier soit utilisé par quoi / qui que ce soit (en tout cas pas par les tests e2e)
ddf7b7f
to
b5263ee
Compare
Il manque quelque chose pour merger ? |
Non je ne pense pas. @Steph0 ? |
b5263ee
to
a8f0cfc
Compare
@yannbertrand vu avec @Steph0 LGTM 🚀 |
a8f0cfc
to
2ed32ad
Compare
Co-authored-by: Steph0 <[email protected]>
2ed32ad
to
9cc5c7f
Compare
🦄 Problème
Lors de cette PR, une régression a été introduite côté API dans la configuration des variables d'environnements. Ainsi, en local, nous n'avions plus de logs pour l'API, car la variable par défaut LOG_ENABLED était commentée dans le sample.env (et donc dans les .env locaux).
🤖 Proposition
Par mesure de sécurité, nous avons passé
LOG_ENABLED
enrequired
dans le fichierapi/lib/infrastructure/validate-environment-variables.js
et nous l'avons dé-commenté dusample.env
🌈 Remarques
Le fichier config.js n'est pas testé et on constate qu'il peut y avoir des régressions.
Peut-être faudrait-il tester au cas par cas lorsqu'on trouve une régression (par exemple en commençant par le cas présent) ?
Cela peut être un sujet d'aprèm tech.
💯 Pour tester
LOG_ENABLED
dans le.env
true