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

[TECH] Utiliser pnpm #7155

Closed
wants to merge 18 commits into from
Closed

[TECH] Utiliser pnpm #7155

wants to merge 18 commits into from

Conversation

francois2metz
Copy link
Contributor

@francois2metz francois2metz commented Sep 26, 2023

🦄 Problème

Notre mono-repo actuel contient des scripts divers et varié pour tenter de maintenir les dépendances de nos différentes applications a jour.
La plupart de nos applications partagent des dépendances similaires qui sont téléchargé et dupliqué.

🤖 Proposition

Utiliser PNPM https://pnpm.io/
Cela a nécessiter la modification du buildpack nodejs: Scalingo/nodejs-buildpack@master...1024pix:nodejs-buildpack:master
Cela pose des questions de maintenance de ce buildpack.

🌈 Remarques

Ceci est la première étape pour tenter de moderniser les outils du mono-repos.
Une prochaine étape pourrait être d'utiliser circletron qui utilise l'API dynamique de circleci et pnpm pour ne tester que le code qui a réellement changer. https://github.com/circletron/circletron
Cela aurait pour avantager de réduire le temps de build pour un certain nombre de PRs.
Une étape suivante pourrait également de rapatrier l'ensemble des applications qui sont nécessaire pour faire fonctionner Pix qui sont réparties dans différents repos ce qui rend les évolutions et la maintenance plus compliqué.

💯 Pour tester

  1. CI verte
  2. Déploiement vert

@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 :

Copy link
Member

@yaf yaf left a comment

Choose a reason for hiding this comment

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

👏

@yannbertrand
Copy link
Member

J'ai testé un peu la CLI de mon côté.

npm install => pnpm install (alias pnpm i)
Petit plaisir si on a déjà installé les dépendances précédemment, un cache local permet de reset les dépendances en offline. Le install global ne fonctionne pas cependant - d'autres téléchargements sont tentés après les dépendances node. pnpm install --offline évite ce problème.

pnpm run liste les scripts à dispo (je connais pas d'équivalent npm).

Les scripts peuvent être lancé sans run (à la yarn). Ex : pnpm clean à la racine.

On peut utiliser les scripts des sous-dossiers comme pnpm start dans l'API.

J'ai pas compris comment lancer un script dans un package uniquement, ni dans un ensemble de packages (ex : api, mon-pix). Le --filter ne fonctionne pas dans mes essais.

A la racine, pnpm start fail à cause de audit-logger.

Le param --frozen-lockfile est automatiquement utilisé si la var d'env CI est settée.

Les doubles dash -- ne sont pas nécessaires avec pnpm, on peut les supprimer de nos scripts.

Je crois pas que les pnpm exec soient nécessaires dans les cas où les dépendances sont installées (confirmé avec db:migrate qui utilise knex en local). C'est nécessaire pour le déploiement ?


Sinon, j'ai pas trop compris ajustements de versions avec des tags GitHub, mais ça se raccourci en supprimant git+https://github.com/, seuls ${orga}/${repo}#${hash} sont nécessaires.

Dans nos dépendances on a aussi https://cdn.sheetjs.com/xlsx-0.19.3/xlsx-0.19.3.tgz que je trouve chelou, mais j'ai pas la connaissance.

Merci @Tarektouati pour le coup de main sur cette revue.

1d/package.json Outdated Show resolved Hide resolved
@HEYGUL
Copy link
Contributor

HEYGUL commented Oct 7, 2023

J'ai testé un peu la CLI de mon côté.

npm install => pnpm install (alias pnpm i) Petit plaisir si on a déjà installé les dépendances précédemment, un cache local permet de reset les dépendances en offline. Le install global ne fonctionne pas cependant - d'autres téléchargements sont tentés après les dépendances node. pnpm install --offline évite ce problème.

pnpm run liste les scripts à dispo (je connais pas d'équivalent npm).

npm run fait la même chose 👍🏾

Les scripts peuvent être lancé sans run (à la yarn). Ex : pnpm clean à la racine.

Merci pour l'info, je vais simplifier nos scripts.

On peut utiliser les scripts des sous-dossiers comme pnpm start dans l'API.

J'ai pas compris comment lancer un script dans un package uniquement, ni dans un ensemble de packages (ex : api, mon-pix). Le --filter ne fonctionne pas dans mes essais.

Le --filter se base sur le nom de package, donc il faut faire --filter pix-api pour l'api (par exemple).

A la racine, pnpm start fail à cause de audit-logger.

audit-logger étant en typescript, une étape de transpilation est nécessaire. Il faut donc invoquer le build avant le start pour cette application.

Le param --frozen-lockfile est automatiquement utilisé si la var d'env CI est settée.

Effectivement, on peut simplifier.

Les doubles dash -- ne sont pas nécessaires avec pnpm, on peut les supprimer de nos scripts.

Effectivement, on peut simplifier là aussi.

Je crois pas que les pnpm exec soient nécessaires dans les cas où les dépendances sont installées (confirmé avec db:migrate qui utilise knex en local). C'est nécessaire pour le déploiement ?

Sinon, j'ai pas trop compris ajustements de versions avec des tags GitHub, mais ça se raccourci en supprimant git+https://github.com/, seuls ${orga}/${repo}#${hash} sont nécessaires.

Dans nos dépendances on a aussi https://cdn.sheetjs.com/xlsx-0.19.3/xlsx-0.19.3.tgz que je trouve chelou, mais j'ai pas la connaissance.

Merci @Tarektouati pour le coup de main sur cette revue.

@@ -33,7 +33,7 @@ executors:
type: string
docker:
- image: cimg/node:<<parameters.node-version>>
resource_class: small
resource_class: medium+
Copy link
Contributor

Choose a reason for hiding this comment

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

Point d'attention : ici, j'ai du augmenter la capacité du container de la CI, car on avait un OOM lors de l'installation des dépendances.


e2e_test_app:
e2e_test:
Copy link
Contributor

Choose a reason for hiding this comment

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

Point d'attention : j'ai mergé les deux jobs de tests e2e qui faisaient quasiment la même chose. A voir si on veut les garder séparer auquel cas il faut retravailler ce point.


- run:
name: Install Cypress
Copy link
Contributor

Choose a reason for hiding this comment

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

Pour une raison que je n'ai pas réussi à déterminer, j'ai été obligé d'ajouter une étape d'installation de Cypress dans le job.

api/package.json Outdated
Comment on lines 154 to 159
"test:api": "status=0; pnpm test:api:unit || status=1 ; for dir in $(find tests/* -maxdepth 0 -type d -not -path tests/unit -not -path tests/shared -not -path tests/certification) ; do pnpm test:api:path $dir || status=1 ; done ; exit $status",
"test:api:path": "NODE_ENV=test mocha --exit --recursive --reporter=${MOCHA_REPORTER:-dot}",
"test:api:scripts": "npm run test:api:path -- tests/integration/scripts",
"test:api:unit": "TEST_DATABASE_URL=postgres://should.not.reach.db.in.unit.tests REDIS_URL= npm run test:api:path -- tests/unit tests/**/unit",
"test:api:integration": "npm run test:api:path -- tests/integration tests/**/integration",
"test:api:acceptance": "npm run test:api:path -- tests/acceptance tests/**/acceptance",
"test:api:scripts": "pnpm test:api:path tests/integration/scripts",
"test:api:unit": "TEST_DATABASE_URL=postgres://should.not.reach.db.in.unit.tests REDIS_URL= pnpm test:api:path tests/unit tests/**/unit",
"test:api:integration": "pnpm test:api:path tests/integration tests/**/integration",
"test:api:acceptance": "pnpm test:api:path tests/acceptance tests/**/acceptance",
Copy link
Member

Choose a reason for hiding this comment

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

Je pense que c'est en retard avec dev cette partie a bien bougé ces derniers temps

@francois2metz francois2metz force-pushed the pnpm-migration branch 2 times, most recently from 8a86895 to 1bebf4c Compare March 27, 2024 09:33
@francois2metz francois2metz force-pushed the pnpm-migration branch 3 times, most recently from 809e145 to 2e80392 Compare March 27, 2024 15:11
@HEYGUL
Copy link
Contributor

HEYGUL commented Apr 17, 2024

Je n'ai plus l'énergie / envie d'avance ce sujet.
Je clos donc la PR.

@HEYGUL HEYGUL closed this Apr 17, 2024
@HEYGUL HEYGUL deleted the pnpm-migration branch April 28, 2024 09:09
@bpetetot bpetetot restored the pnpm-migration branch December 17, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-team Toutes les équipes de dev 👀 Tech Review Needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants