Restructure la configuration CircleCI #1650

HAEKADI · 2021-08-18T08:37:31Z

Amélioration technique.
Périodes concernées : toutes.
Zones impactées : .circleci/config.yml.
Détails :
- Sépare les jobs test-python et test-yaml du build.
- Regroupe les deux jobs lint_python_files et lint_yaml_files dans un seul job.
- Rend le job check-version-and-changelog dépendant de tous les autres jobs.

HAEKADI · 2021-08-18T08:39:51Z

Je pense qu'il faudra aussi changer les status-checks du merge vers master.

HAEKADI · 2021-08-18T14:43:35Z

Suite à un échange avec @MattiSG, je sépare le job install du job build.
Auparavant, l'installation des dépendances se faisait au niveau du job build avec le make build au lieu du make install.
Désormais, le job install se chargera du make install et le job build du make build.
Le linting dépendra de l'install et les tests dépendront du build.
Par contre, dans ce cas nous exécutons le pip install openfisca-core[web-api]deux fois.
@MattiSG doit-on remplacer le cache sauvegardé à l' install par celui du build ou sauvegarder les deux?

MattiSG · 2021-08-18T15:39:35Z

Par contre, dans ce cas nous exécutons le pip install openfisca-core[web-api]deux fois.

Je ne comprends pas pourquoi make build comprend pip install openfisca-core[web-api]. Éventuellement, je peux admettre que make build déclenche make install, mais ce doublon me gêne.
De même, je ne comprends pas pourquoi make build déclenche make deps, ni pourquoi make deps n'installe en réalité que des dépendances utiles pour le packaging.
@maukoquiroga as-tu des éléments de compréhension sur le sujet ?

En l'état, je suggèrerais de laisser tel quel pour ne pas modifier le comportement de make build sans le comprendre, et d'ouvrir une autre PR pour supprimer pip install openfisca-core[web-api] de make build.

MattiSG

A priori pas encore prêt pour une revue au vu du dernier commentaire, merci de m'assigner pour la revue quand ce sera prêt :)

.circleci/config.yml

HAEKADI · 2021-08-19T08:37:44Z

@MattiSG Donc pour le moment, je ne sépare pas le job install du build?

MattiSG · 2021-08-19T15:06:25Z

Donc pour le moment, je ne sépare pas le job install du build?

Pardon, je me suis mal exprimé. Je pense que cette tâche est toujours pertinente. Je parlais simplement d'éclater dans une autre PR la modification du makefile pour retirer le doublon d'installation des dépendances 🙂

HAEKADI · 2021-08-20T13:52:42Z

@MattiSG Je ne sais pas s'il faut re-sauvegarder un cache des dépendances (déjà sauvgardées à l'install) après le build ?

MattiSG · 2021-08-24T10:11:05Z

Je ne sais pas s'il faut re-sauvegarder un cache des dépendances (déjà sauvegardées à l'install) après le build ?

Vu que make build déclenche des appels à pip install, il me semble pertinent de re-sauvegarder le cache en effet !

MattiSG

Waiting for the PR to not be ready for review for a deeper review 🙂

.circleci/lint-changed-python-files.sh

.circleci/lint-changed-yaml-tests.sh

.circleci/lint-changed-python-files.sh

.circleci/lint-changed-yaml-tests.sh

MattiSG

On y est presque ! 😃

(désolé pour les commentaires en anglais, j'ai oublié qu'on était sur France)

.circleci/config.yml

MattiSG · 2021-08-24T14:14:53Z

.circleci/config.yml

+            - test_python
+            - test_yaml
+            - test_api
+            - lint_files
      - deploy:
          requires:
            - check_version_and_changelog


Move this downwards to align dependencies order with reading order.

I would be at ease with minimising the number of dependencies, knowing they are transitive: if check_version_and_changelog passes and it requires all the other jobs, we maybe don't need to list all of them explicitly. Do whatever feels safer to you though 🙂

Je trouve aussi que c'est redondant. Je les avais gardées pour des raisons de lisibilité du fichier de config. Mais vu que tu penses aussi qu'il y a une redondance, je les ai supprimées.

Donc pour le deploy, je ne garde que le check_version_and_changelog 🤔

J'ai l'impression… ça fait un peu peur, non ? 😅

Un peu 😬 Mais le DAG montre clairement les dépendances, donc ça ira je pense 😅

MattiSG

C'est si beau 😭
Merci @HAEKADI !

.circleci/config.yml

MattiSG · 2021-08-24T16:33:50Z

J'ai mis à jour les règles de protection de branche. Toutes les PR ouvertes devront donc faire un rebase.

HAEKADI · 2021-08-24T18:14:41Z

@MattiSG Je ne merge pas vu que test_python ne passe pas.
Je ne comprends pas du tout la source de l'erreur, sachant que je n'ai rien changé dans la config depuis cette exécution, sauf le bump de la version et le rebase, et que le test en question tests/test_metadata.pypasse en local sans aucun problème.
Ce qui m'amène à croire que l'erreur provient peut-être du cache 🤔

HAEKADI · 2021-08-25T14:36:52Z

@sandcha a restitué les règles de protection de la master, pour ne pas bloquer les autres PRs, en attendant la résolution de ce bug.

HAEKADI · 2021-08-25T17:01:38Z

Le problème provenait effectivement du cache.

En re-exécutant le job test_python avec SSH, après le commit du bump de la version, je ne retrouve plus Openfisca-France dans l'environnement, d'où l'échec du test test_metadata.py.

L'exécution de make install dans le job test_python résound le problème, ce qui confirme davantage que le cache ne fonctionne pas pour ce job.

La solution que j'ai pu retrouver est de sauvegarder le build dans un cache v2, séparé du cache de v1 l'install.

Je pense que ceci crée effectivement des redondances, mais pour le moment c'est la seule solution que j'ai pu trouver.
La restructuration du Makefile peut potentiellement remédier à ce problème.

J'ai lancé les tests plusieurs fois sur la CI pour être sûre.

CHANGELOG.md

HAEKADI · 2021-09-01T08:56:30Z

@MattiSG J'ai fait le rebase, mis à jour le changelog et le setup, et les tests passent. Est-ce que tu peux mettre à jour les règles de protection pour faire le merge, please?

MattiSG · 2021-09-01T10:43:51Z

J'ai désactivé l'obligation de passer les anciens tests lint, mais pas encore activé l'obligation de passer les nouveaux tests, afin de ne pas bloquer l'intégration de branches déjà validées et en attente d'intégration. La protection actuelle me semble adéquate car check_version_and_changelog est toujours obligatoire, et qu'une fois cette branche intégrée, cela impliquera que tous les tests passent.
On pourra toujours activer des obligations supplémentaires pour plus de lisibilité plus tard si le besoin s'en fait sentir 🙂

bonjourmauko · 2021-09-01T23:46:39Z

Par contre, dans ce cas nous exécutons le pip install openfisca-core[web-api]deux fois.

Je ne comprends pas pourquoi make build comprend pip install openfisca-core[web-api]. Éventuellement, je peux admettre que make build déclenche make install, mais ce doublon me gêne.
De même, je ne comprends pas pourquoi make build déclenche make deps, ni pourquoi make deps n'installe en réalité que des dépendances utiles pour le packaging.
@maukoquiroga as-tu des éléments de compréhension sur le sujet ?

En l'état, je suggèrerais de laisser tel quel pour ne pas modifier le comportement de make build sans le comprendre, et d'ouvrir une autre PR pour supprimer pip install openfisca-core[web-api] de make build.

Hello @HAEKADI , si encore utile : je pense que make est particulièrement utile pour les tâches de test et de lint, et que pour la gestion de dépendances nous avons d'autres outils plus pertinentes, comme setuptools et poetry.

J'ai fait une proposition dans ce sens sur openfisca/openfisca-core#1015. Je te propose qu'on itère dessus, et qu'une fois qu'on arrive à un consensus on essaie de propager ça :)

HAEKADI self-assigned this Aug 18, 2021

MattiSG reviewed Aug 18, 2021

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

MattiSG mentioned this pull request Aug 19, 2021

Switch from CircleCI to GitHub Actions openfisca/openfisca-core#1030

Closed

HAEKADI marked this pull request as draft August 20, 2021 12:10

HAEKADI force-pushed the config-ci branch from cac1b15 to ae4e201 Compare August 20, 2021 12:24

HAEKADI force-pushed the config-ci branch 2 times, most recently from 0d30e27 to 293df2a Compare August 20, 2021 14:40

MattiSG reviewed Aug 24, 2021

View reviewed changes

HAEKADI marked this pull request as ready for review August 24, 2021 13:27

HAEKADI requested a review from MattiSG August 24, 2021 13:28

MattiSG requested changes Aug 24, 2021

View reviewed changes

HAEKADI requested a review from MattiSG August 24, 2021 15:45

MattiSG approved these changes Aug 24, 2021

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

HAEKADI force-pushed the config-ci branch 2 times, most recently from 247d0e5 to 6a0747d Compare August 24, 2021 17:33

HAEKADI force-pushed the config-ci branch from af98c3b to ed5497e Compare August 25, 2021 14:34

HAEKADI requested a review from MattiSG August 25, 2021 17:02

MattiSG approved these changes Sep 1, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

Sépare le job test-python du build

3e8c175

HAEKADI added 21 commits September 1, 2021 10:39

Sauvegarde le cache du build

510f8b3

Nomme le job build

ab3137d

Ajoute la dépendance test_api au check_version

b6942b9

Améliore la formulation

6a502af

Sauvegarde le cache après le build

f5dfc4d

Supprime la redondance de l'utilisation de OpenFisca Core

a5e4b26

Supprime le split des tests Python

5c1f067

Restructure la config

2cb7b28

Supprime les redondances des dépendances dans deploy

f039ea3

Change l'ordre des déclarations

781d9e1

Teste sans le cache du build

c937ebd

Teste avec le cache du build

2f60c8c

Change la dépendance des tests Python

8c0e1d0

Ajoute revision cache aux tests Python

54acb66

Supprime le job install

16d6925

Ajoute le job install

623fdbc

Ajoute au job test_python

206e9f0

Ajoute deuxième cache pour le build

c35aaed

Change le cache utilisé dans les jobs

346cd07

Metà jour le CHANGELOG

79f5a52

Met à jour le setup

511f6bd

HAEKADI force-pushed the config-ci branch from 46bb08e to 511f6bd Compare September 1, 2021 08:41

HAEKADI merged commit ceb6647 into master Sep 1, 2021

HAEKADI deleted the config-ci branch September 1, 2021 10:51

This was referenced Sep 1, 2021

Passage de CircleCI vers GitHub Actions #1663

Merged

Lint YAML and Python files on CI openfisca/country-template#114

Closed

Document Makefile openfisca/openfisca-core#1036

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure la configuration CircleCI #1650

Restructure la configuration CircleCI #1650

HAEKADI commented Aug 18, 2021 •

edited

Loading

HAEKADI commented Aug 18, 2021 •

edited

Loading

HAEKADI commented Aug 18, 2021

MattiSG commented Aug 18, 2021

MattiSG left a comment

HAEKADI commented Aug 19, 2021 •

edited

Loading

MattiSG commented Aug 19, 2021

HAEKADI commented Aug 20, 2021 •

edited

Loading

MattiSG commented Aug 24, 2021

MattiSG left a comment

MattiSG left a comment

MattiSG Aug 24, 2021

HAEKADI Aug 24, 2021

HAEKADI Aug 24, 2021 •

edited

Loading

MattiSG Aug 24, 2021

HAEKADI Aug 24, 2021

MattiSG left a comment

MattiSG commented Aug 24, 2021

HAEKADI commented Aug 24, 2021 •

edited

Loading

HAEKADI commented Aug 25, 2021

HAEKADI commented Aug 25, 2021 •

edited

Loading

HAEKADI commented Sep 1, 2021

MattiSG commented Sep 1, 2021

bonjourmauko commented Sep 1, 2021

Restructure la configuration CircleCI #1650

Restructure la configuration CircleCI #1650

Conversation

HAEKADI commented Aug 18, 2021 • edited Loading

HAEKADI commented Aug 18, 2021 • edited Loading

HAEKADI commented Aug 18, 2021

MattiSG commented Aug 18, 2021

MattiSG left a comment

Choose a reason for hiding this comment

HAEKADI commented Aug 19, 2021 • edited Loading

MattiSG commented Aug 19, 2021

HAEKADI commented Aug 20, 2021 • edited Loading

MattiSG commented Aug 24, 2021

MattiSG left a comment

Choose a reason for hiding this comment

MattiSG left a comment

Choose a reason for hiding this comment

MattiSG Aug 24, 2021

Choose a reason for hiding this comment

HAEKADI Aug 24, 2021

Choose a reason for hiding this comment

HAEKADI Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

MattiSG Aug 24, 2021

Choose a reason for hiding this comment

HAEKADI Aug 24, 2021

Choose a reason for hiding this comment

MattiSG left a comment

Choose a reason for hiding this comment

MattiSG commented Aug 24, 2021

HAEKADI commented Aug 24, 2021 • edited Loading

HAEKADI commented Aug 25, 2021

HAEKADI commented Aug 25, 2021 • edited Loading

HAEKADI commented Sep 1, 2021

MattiSG commented Sep 1, 2021

bonjourmauko commented Sep 1, 2021

HAEKADI commented Aug 18, 2021 •

edited

Loading

HAEKADI commented Aug 18, 2021 •

edited

Loading

HAEKADI commented Aug 19, 2021 •

edited

Loading

HAEKADI commented Aug 20, 2021 •

edited

Loading

HAEKADI Aug 24, 2021 •

edited

Loading

HAEKADI commented Aug 24, 2021 •

edited

Loading

HAEKADI commented Aug 25, 2021 •

edited

Loading