-
Notifications
You must be signed in to change notification settings - Fork 54
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] Standardisation des scripts nodejs #10273
base: dev
Are you sure you want to change the base?
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 : |
86968d8
to
bb89b45
Compare
bb89b45
to
db4da2c
Compare
960ee56
to
24bed6c
Compare
6d7a723
to
9f569e0
Compare
9f569e0
to
b6caf5a
Compare
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.
Pourrait-on profiter de cette PR pour déplacer ce script dans le bon contexte DDD, c'est à dire concrètement déplacer ce fichier dans src/organizational-entities/scripts
si je ne me trompe pas.
Ainsi les personnes qui consulteront cette PR pour bénéficier du cadre standardisé d'exécution de script auront aussi une incitation à en profiter pour ranger le script dans le bon contexte DDD, ce qui participe à l'amélioration de la maintenance car cela documente à quel contexte appartient le script.
/** | ||
* Base class for scripts with meta information and execution handling. | ||
*/ | ||
export class BaseScript { |
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 trouve que BaseScript
est trop générique, ça ne fait pas penser que c'est LA classe de base du code de Pix pour les scripts à exécuter pour faire des tâches particulières, en effet il y a des scripts pour faire tout et n'importe quoi … Je propose un autre nom ManagedScript
, mais pourquoi pas BaseManagedScript
ou si vous avez d'autres idées :
export class BaseScript { | |
export class ManagedScript { |
|
||
import { logger as defaultLogger } from '../../infrastructure/utils/logger.js'; | ||
|
||
const META_INFO_SCHEMA = Joi.object({ |
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 suggère plutôt METAINFO_SCHEMA
. D'ailleurs dans le constructeur l'argument est nommé metaInfo
, sans underscore.
const META_INFO_SCHEMA = Joi.object({ | |
const METAINFO_SCHEMA = Joi.object({ |
import { learningContentCache } from '../../infrastructure/caches/learning-content-cache.js'; | ||
import { logger as defaultLogger } from '../../infrastructure/utils/logger.js'; | ||
|
||
function isRunningFromCLI(scriptFileUrl) { |
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 préconise d’utiliser toujours le camelCase pour les acronymes et les sigles. D'ailleurs on a scriptFileUr
l et non scriptFileUrl
:
function isRunningFromCLI(scriptFileUrl) { | |
function isRunningFromCli(scriptFileUrl) { |
En effet utiliser du camelCase pour les acronymes et les sigles permet d'avoir une règle de nommage systématiquement cohérente et automatisable.
/** | ||
* A utility class for running scripts from the command line. | ||
*/ | ||
export class ScriptRunner { |
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.
Adapter le nom de la classe du Script Runner suivant la suggestion de modifier le nom de la classe de base pour les scripts :
export class ScriptRunner { | |
export class ManagedScriptRunner { |
/** | ||
* Create a parser for a CSV file with the given columns schema | ||
* @param {Record<string, Joi.Schema>} columnsSchema | ||
* @returns {Promise<Array<Record<string, any>>>} parsed data | ||
*/ | ||
export function csvFileParser(columnsSchema = []) { | ||
return async (filePath) => { | ||
const columnNames = columnsSchema.map((column) => column.name); | ||
|
||
await checkCsvHeader({ filePath, requiredFieldNames: columnNames }); | ||
|
||
return parseCsvWithHeader(filePath, { | ||
header: true, | ||
skipEmptyLines: true, | ||
transform: (value, columnName) => { | ||
const column = columnsSchema.find((column) => column.name === columnName); |
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.
Nommage correct en anglais :
/** | |
* Create a parser for a CSV file with the given columns schema | |
* @param {Record<string, Joi.Schema>} columnsSchema | |
* @returns {Promise<Array<Record<string, any>>>} parsed data | |
*/ | |
export function csvFileParser(columnsSchema = []) { | |
return async (filePath) => { | |
const columnNames = columnsSchema.map((column) => column.name); | |
await checkCsvHeader({ filePath, requiredFieldNames: columnNames }); | |
return parseCsvWithHeader(filePath, { | |
header: true, | |
skipEmptyLines: true, | |
transform: (value, columnName) => { | |
const column = columnsSchema.find((column) => column.name === columnName); | |
/** | |
* Create a parser for a CSV file with the given column schema | |
* @param {Record<string, Joi.Schema>} columnSchema | |
* @returns {Promise<Array<Record<string, any>>>} parsed data | |
*/ | |
export function csvFileParser(columnSchema = []) { | |
return async (filePath) => { | |
const columnNames = columnSchema.map((column) => column.name); | |
await checkCsvHeader({ filePath, requiredFieldNames: columnNames }); | |
return parseCsvWithHeader(filePath, { | |
header: true, | |
skipEmptyLines: true, | |
transform: (value, columnName) => { | |
const column = columnSchema.find((column) => column.name === columnName); |
{ name: 'email', schema: Joi.string().trim().replace(' ', '').lowercase().email().optional().default(null) }, | ||
]; | ||
|
||
export class AddOrUpdateCertificationCentersDpoInfosScript extends BaseScript { |
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.
Dans le code en anglais on écrit plutôt info
au singulier :
export class AddOrUpdateCertificationCentersDpoInfosScript extends BaseScript { | |
export class AddOrUpdateCertificationCentersDpoInfoScript extends BaseScript { |
* @param {typeof BaseScript} ScriptClass - The script class to be instantiated and executed. | ||
* @param {object} [dependencies] - The script runner dependencies (logger, isRunningFromCLI) | ||
*/ | ||
static async execute(scriptFileUrl, ScriptClass, dependencies = { logger: defaultLogger, isRunningFromCLI }) { |
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.
Adapter suivant la suggestion de modifier le nom de la classe de base pour les scripts :
static async execute(scriptFileUrl, ScriptClass, dependencies = { logger: defaultLogger, isRunningFromCLI }) { | |
static async execute(scriptFileUrl, ManagedScriptClass, dependencies = { logger: defaultLogger, isRunningFromCLI }) { |
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.
✅ Lu et testé fonctionnellement avec succès
Néanmoins j'ai effectué des essais en ne mettant pas toutes les valeurs dans les colonnes du CSV et cela n'a pas causé d'erreur car le usecase updateCertificationCenterDataProtectionOfficerInformation
et la tables SQL data-protection-officers
acceptent qu'on ne spécifie pas toutes les infos. Aussi si on veut s'assurer dans cette PR que le csvFileParser
est bien adapté à tous les cas, il faudrait ajouter un autre script pour lequel toutes les colonnes de son fichier en argument sont obligatoires. Mais cela pourrait être réalisé dans une autre PR, pas besoin que tout soit dans cette PR-ci.
🦄 Problème
Première implémentation de l'ADR 56 : #10397
🤖 Proposition
Création des composants nécessaires à la standardisation des script:
BaseScript
: classe générique permettant d'instancier un nouveau script et d'apporter un ensemble de fonctionnalités de base dans l'implémentation de scripts.ScriptRunner
: classe utilitaire permettant d'exécuter un script incluant des logs de suivi, une gestion des erreurs et la gestion de fermeture des resources.parsers
: un ensemble de parsers pour les données d'entrées des scripts (csvFileParser
,commaSeparatedStringsParser
,commaSeparatedNumbersParser
)Important
Mise en place de ce standard sur un premier script:
api/scripts/add-or-update-certification-centers-dpo-infos.js
💯 Pour tester
Exécuter le script
scripts/add-or-update-certification-centers-dpo-infos.js
avec le fichiercertification-centers-dpo-info.csv
suivant :Commandes possibles à exécuter dans
/api
Afficher l'aide :
Exécuter le script :