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

[DOC] ADR 56: Standardisation de l'exécution des scripts #10397

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

bpetetot
Copy link
Contributor

@bpetetot bpetetot commented Oct 22, 2024

TL;DR

Contexte : Nous constatons que nos scripts Node.js partagent un code boilerplate important pour la gestion des paramètres d'entrée, des erreurs et des ressources. De plus, certains scripts "one-shots" restent longtemps dans la codebase, et il y a un manque d'outils de test et de documentation.

Décision proposée : Créer une classe de base pour standardiser l'exécution des scripts Node, incluant des fonctionnalités essentielles. L'accompagner d'une documentation et de bonnes pratiques autour de l'exécution des scripts.

ADR à relire et commenter. Merci 🙏

Mise en place de la solution proposée sur la PR #10273

@bpetetot bpetetot self-assigned this Oct 22, 2024
@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@bpetetot bpetetot added 👀 Tech Review Needed cross-team Toutes les équipes de dev labels Oct 22, 2024
@bpetetot bpetetot force-pushed the tech-adr-56 branch 2 times, most recently from 6c0d291 to 2ed1ebb Compare October 22, 2024 16:58
@bpetetot bpetetot changed the title [DOC] ADR 56: Standardisation de l'éxecution des script [DOC] ADR 56: Standardisation de l'éxecution des scripts Oct 22, 2024

### Exemples d’utilisation

Un exemple simple d’utilisation d’un script via cette classe standardisée et du runner :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bpetetot dans quelle mesure tu penses que c'est possible d'intégrer le concept de cette PR dans le nouveau template de script ?
A savoir le fait d'historiser les exécutions. Je me dis que ta classe de base pouvait implémenter dans le handle générique le fait de logger en BDD les exécutions ?
Je vais la fermer ma PR j'ai pas le temps de l'entretenir

Copy link
Contributor Author

@bpetetot bpetetot Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est tout à fait possible, d'ailleurs @HEYGUL l'a évoqué pendant le tech time. Et je l'ai ajouté dans la section "Fonctionnalités supplémentaires" de l'ADR:

Stocker en base les informations d'exécution : Créer une table en base de données pour enregistrer les informations d'exécution des scripts, telles que le nom du script, l’heure de début et de fin, le statut (succès ou échec), et la progression. Cela fournirait une visibilité historique et permettrait un suivi précis des scripts exécutés.

Si tu regardes la PR où j'implémente cette ADR https://github.com/1024pix/pix/pull/10273/files#diff-677d2fe0b113daf7cb8a94de41dd34413a95d72494f3270d73b69f50cb094e69, il y a la classe ScriptRunner qui a en charge l'exécution du script et la gestion des erreurs. On peut y ajouter des appels en base de données pour l'historisation des exécutions très facilement soucis.

Copy link
Contributor Author

@bpetetot bpetetot Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'ailleurs, même si tu fermes ta PR, je vais la mettre en lien dans l'ADR afin d'en conserver la trace afin qu'on puisse s'en inspirer si on l'implémente ça dans ce nouveau standard.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok super merci 🚀

@yannbertrand
Copy link
Member

Très bonne idée ! Je suis pour expérimenter dès maintenant cet usage quitte à faire évoluer la proposition :)

@bpetetot
Copy link
Contributor Author

Très bonne idée ! Je suis pour expérimenter dès maintenant cet usage quitte à faire évoluer la proposition :)

Ça tombe bien, la PR existe déjà 😉: #10273

@aurelie-crouillebois
Copy link
Contributor

Cette PR d'ADR est top (ainsi que celle d'implem). Je serais bien pour ramener l'historisation dans le scope initial plutôt que dans le "Aller plus loin...".

@bpetetot
Copy link
Contributor Author

Cette PR d'ADR est top (ainsi que celle d'implem). Je serais bien pour ramener l'historisation dans le scope initial plutôt que dans le "Aller plus loin...".

J'ai modifié le chapitre "Aller plus loin..." en "Fonctionnalités supplémentaires", car en effet, ces fonctionnalités font (pour moi) parti du scope. Elles ne seront pas dans la première PR qui met en place la base, mais seront inclus petit à petit dans de prochains tech times ;)

@bpetetot bpetetot changed the title [DOC] ADR 56: Standardisation de l'éxecution des scripts [DOC] ADR 56: Standardisation de l'exécution des scripts Oct 24, 2024
Copy link
Member

@yannbertrand yannbertrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fond pour tester !

@pix-service-auto-merge pix-service-auto-merge merged commit 7f838e4 into dev Oct 25, 2024
7 of 8 checks passed
@pix-service-auto-merge pix-service-auto-merge deleted the tech-adr-56 branch October 25, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants