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

Passage de CircleCI vers GitHub Actions #1663

Merged
merged 93 commits into from
Sep 15, 2021
Merged

Passage de CircleCI vers GitHub Actions #1663

merged 93 commits into from
Sep 15, 2021

Conversation

HAEKADI
Copy link
Contributor

@HAEKADI HAEKADI commented Sep 1, 2021

Related to #1030
Related to #1027

GitHub Actions

  • Amélioration technique.
  • Zones impactées : configuration de l'intégration continue.
  • Détails :
    • Switche de CircleCI vers GitHub Actions pour améliorer la performance.

J'ai dupliqué le même schéma du workflow que celui-ci #1650.

J'ai testé le workflow sur plusieurs versions de Python la 3.7, 3.8 et la 3.9. Tous les jobs passent à l'exception du test-api pour Python 3.9 qui tourne sans arrêt.


Ce qu'il reste à faire:

  • Choisir les versions Python à garder
  • Ajouter PYPI_PASSWORD à GitHub Secrets
  • Tester le deploy
  • Supprimer la config CircleCI une fois la PR validée
  • Mettre à jour les règles de protection des merges après la validation de la PR
  • Gérer les notifications

Screenshot 2021-09-06 at 15 58 51

@HAEKADI HAEKADI changed the title Passage de CircleCI vers GitHub actions Passage de CircleCI vers GitHub Actions Sep 1, 2021
@HAEKADI HAEKADI self-assigned this Sep 1, 2021
@MattiSG
Copy link
Member

MattiSG commented Sep 1, 2021

J'ai corrigé le point suivant en éditant la description : cette PR ne va pas fermer openfisca/openfisca-core#1030, car elle vient simplement traiter le cas d'OpenFisca-France, pas de Core. On pourra considérer que openfisca/openfisca-core#1030 est fermée quand Core, Country-Template et Extension-Template auront migré 🙂

Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

C'est un super début, merci @HAEKADI ! 😃

Il faudra supprimer toute la configuration CircleCI, et privilégier le dossier .github pour les scripts, plutôt que de rendre visible le dossier ci-scripts, car la majorité des contributeurs n'ont pas connaissance de cette machinerie.

Il pourrait être utile qu'on ait un échange synchrone pour déterminer la stratégie : il m'aurait semblé pertinent de commencer par le Country Template 🙂

.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
Comment on lines 5 to 11
if ! changes=$(git diff-index --name-only --diff-filter=ACMR --exit-code $last_tagged_commit -- "*.py")
then
echo "Linting the following Python files:"
echo $changes
flake8 $changes
else echo "No changed Python files to lint"
fi
Copy link
Member

Choose a reason for hiding this comment

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

Est-ce qu'on ne pourrait pas migrer cette logique (qui au final n'est que du passage de paramètre à flake8) dans le Makefile, sur la base du travail fait par @maukoquiroga dans openfisca/openfisca-core#1036 ?

Je trouverais bienvenu de consolider l'exécution du lint dans une seule commande, avec seulement un passage de paramètre différent en CI, où l'on va se focaliser sur les fichiers modifiés.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

En fait au sujet du lint j'ai envisagé l'utilisation de super-linter directement 🤔

ci-scripts/lint-changed-yaml-tests.sh Outdated Show resolved Hide resolved
ci-scripts/split-tests.sh Outdated Show resolved Hide resolved
fi
done

openfisca test $final_list
Copy link
Member

Choose a reason for hiding this comment

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

Si les tests sont exécutés, alors le script fait plus que split-tests 🙂

Je crois préférable qu'il se contente de donner une liste de tests à exécuter, et que openfisca test soit appelé indépendamment.

Copy link
Member

Choose a reason for hiding this comment

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

Hello ! Il existe pytest-split qui facilite le split de tests par durations, et l'auteur donne un example avec Github Actions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello! Merci @maukoquiroga :) Le problème est que j'utilise openfisca test tests/**/*.py pour exécuter les tests Python et non pas la commande pytest. D'après ce que j'ai compris il n'y a pas de consensus sur les commandes à utiliser sur la CI 🤔

.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
@HAEKADI
Copy link
Contributor Author

HAEKADI commented Sep 1, 2021

@MattiSG J'avais effectivement au début mis tous les scripts dans ./github, mais en cherchant les bonnes pratiques, j'ai retrouvé ceci, ce qui me semblait logique. Tu penses que je dois remettre les fichiers dans le même dossier (./github) ?

@MattiSG
Copy link
Member

MattiSG commented Sep 2, 2021

L'idée que les fichiers dans .github pourraient être en conflit de nommage futur est effectivement pertinente, mais je crois que la probabilité d'une collision est très faible. On l'a fait sans souci pendant des années avec CircleCI.
Ok si tu préfères ne pas prendre ce risque, mais je persiste dans l'idée que si ces scripts n'ont vocation à être utilisés qu'en CI, il est préférable de les masquer à la majorité des utilisateurs : soit on les met dans un dossier invisible (préfixé par un point), soit on considère qu'ils peuvent être exécutés localement aussi et on les met dans un dossier découvrable 🙂

@HAEKADI
Copy link
Contributor Author

HAEKADI commented Sep 6, 2021

@MattiSG Suite à notre dernier échange, j'ai supprimé le job install au profit du job build.

@HAEKADI
Copy link
Contributor Author

HAEKADI commented Sep 8, 2021

I have been experimenting with Act for local execution, and it is unstable to say the least.
I have not once succeeded in running the entire workflow.

Here are the details:

One problem, that I encountered early on, was a cache problem. As it is not possible to save the build cache. One workaround was to add a make build step for each job in case the cache was not found locally.

The errors are random for the same code base and config file. The individual jobs succeed "sometimes" except for test-yaml which fails every time. Since the test-yaml job is technically ten jobs (matrix strategy to divide tests into ten chunks), some jobs succeed and others fail at different levels, sometimes in the make build stage and other times in the run yaml tests stage.

Sometimes the builds failed with a no such file or directory error, and sometimes with an operation not permitted error, again with the same code base. Other times the build succeeded and soon after the processus was killed while running the tests ( See 1 , 2 & 3).

I have experimented with lowering the number of parallelism (and by consequence of containers), and it did not help. There is always a sub test-yaml job that fails, whether it's 2 containers, 3 or 10.

Looking at community discussions, some people have encountered similar issues but no solution, that I could find, was suggested.

I tested with @sandcha, and she encountered similar issues as well.

Also, one of the biggest issues, is how slow and time consuming it actually is. It takes a simple build job, that usually takes 1 minute if ran from scratch (a simple make build locally and the build job ran on the CI), about 30 min to run with Act which defies the purpose of using it.

Giving the average run time of 2 min on GitHub Actions, I find it more reasonable and time effective to debug using the CI rather then doing it locally with Act.

I am not certain about the origin of the issue (Docker or Act), but the more I test it, the more unstable Act seems in the case of a large testing suite like OpenFisca France.

Screenshots referenced above:
Screenshot 2021-09-08 at 10 40 16
Screenshot 2021-09-08 at 17 35 25
Screenshot 2021-09-08 at 16 21 09

Capture d’écran 2021-09-08 à 16 19 33

Capture d’écran 2021-09-08 à 17 09 24

Capture d’écran 2021-09-08 à 17 09 53

Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

🚀 😃

@HAEKADI
Copy link
Contributor Author

HAEKADI commented Sep 14, 2021

J'ai testé la publication du package du job deploy sur cette branche avec deux versions pre-release.
Screenshot 2021-09-14 at 13 40 01

J'avais deux options pour sauvegarder le répértoire dist du build: soit en artifactqui dure jusqu'à 90 jours et qui est téléchargeable sur l'API GitHub Actions, soit en cache.

J'ai testé les deux et finalement j'ai choisi le cache vu que l'artifact n'est pas transférable entre les différents workflows pour le moment (https://github.community/t/builtin-support-for-artifact-sharing/16176).

@MattiSG
Copy link
Member

MattiSG commented Sep 14, 2021

La clé SSH qui permet le déploiement de l'API France est disponible dans le secret FRANCE_API_DEPLOY_KEY. L'usage est apparemment de l'ajouter manuellement, il n'y a pas de configuration dédiée sur GitHub Actions 😕

@HAEKADI
Copy link
Contributor Author

HAEKADI commented Sep 14, 2021

@MattiSG Done! J'ai rajouté la clé SSH. En regardant les logs, le script fr.openfisca.org/api/deploy-latest.sh semble bien être exécuté. Par contre, la version sur l'API ne change pas, mais j'imagine que ceci est dû à la version beta 🤔

Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

Awesome, I think we're finally ready to ship! 😃
Please just make sure to add back the condition on deployment to deploy only on master 😉

.github/workflows/workflow.yml Outdated Show resolved Hide resolved
.github/workflows/workflow.yml Outdated Show resolved Hide resolved
setup.py Outdated
@@ -5,7 +5,7 @@

setup(
name = "OpenFisca-France",
version = "74.1.1-beta.1",
version = "74.1.1-beta.4",
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to publish a new tagged version if we only change CI stuff? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure. We did when we restructured the CircleCI config file.

@MattiSG
Copy link
Member

MattiSG commented Sep 15, 2021

Par contre, la version sur l'API ne change pas, mais j'imagine que ceci est dû à la version beta

Oui, comme c'est une prerelease elle n'est pas installée comme latest automatiquement. Je pense qu'on peut se reposer sur le log pour dire que c'est bon 🙂

@HAEKADI HAEKADI merged commit 9094867 into master Sep 15, 2021
@HAEKADI HAEKADI deleted the github-actions branch September 15, 2021 15:02
@MattiSG
Copy link
Member

MattiSG commented Sep 15, 2021

Super !

On a un juste un souci : la mise à jour de l'API a échoué 😕

J'ai bien suivi le déroulé du déploiement, qui a fonctionné correctement, à ceci près que Requirement already up-to-date: OpenFisca-France au lieu de Installing collected packages: OpenFisca-France.
J'ai effectué manuellement la mise à jour vers la 74.1.1 en déployant depuis ma machine, afin de vérifier que le processus de déploiement lui-même n'était pas en cause.

Je m'inquiète donc que le problème provienne de la rapidité d'exécution sur GitHub Actions : peut-être que le délai entre la publication sur Pypi et l'installation n'est pas suffisant ?

Je propose que nous ajoutions un test automatisé pour vérifier que la version de l'API déployée est bien celle que nous venons de publier. Cela nous permettra a minima d'être informés rapidement si le problème arrive à nouveau !

@MattiSG
Copy link
Member

MattiSG commented Sep 15, 2021

Dans la mesure où la configuration CircleCI a été supprimée, je vais :

  1. Cesser de build le projet avec CircleCI, afin que nous n'ayons plus de coche rouge.
  2. Rendre obligatoire les tests GitHub Actions.

@HAEKADI
Copy link
Contributor Author

HAEKADI commented Sep 15, 2021

@MattiSG On peut tester de suspendre l'exécution quelque temps entre la publication sur Pypi et le déploiement de l'api ?

@MattiSG
Copy link
Member

MattiSG commented Sep 15, 2021

Ça sera en dernier recours. Déjà, vérifions à quelle fréquence le problème apparaît 😉

@HAEKADI
Copy link
Contributor Author

HAEKADI commented Sep 22, 2021

@MattiSG L'API a été bien mise à jour hier avec ce merge. Mais j'ai rajouté un job pour signaler le fail s'il se reproduit.

@MattiSG
Copy link
Member

MattiSG commented Sep 23, 2021

A priori le problème continue à apparaître régulièrement.

À première vue, je serais d'avis d'attendre que openfisca/openfisca-ops#95 soit finalisée, et que le déploiement se fasse automatiquement à la publication sur Pypi. Ainsi, ça ne serait plus la CI qui déploierait l'instance de France sur openfisca.org, elle se contenterait de publier sur Pypi, et un autre dispositif effectuerait la mise à jour, comme n'importe quel autre client. Cela permettra d'augmenter le dogfooding et de tester de bout en bout la procédure d'installation proposée aux tiers 🙂

@HAEKADI
Copy link
Contributor Author

HAEKADI commented Sep 23, 2021

Ok. Donc en attendant, quelqu'un doit se charger de la mise à jour manuelle en cas de problème 🤔

@MattiSG
Copy link
Member

MattiSG commented Sep 23, 2021

Je ne pense pas que ça soit vraiment nécessaire : en l'état, on aura au pire une version de retard, de manière imprévisible. Or, nous n'avons aucune promesse sur le déploiement de cette API. Je ne pense donc pas que la situation actuelle soit problématique 🙂

@HAEKADI HAEKADI restored the github-actions branch September 29, 2021 10:47
@guillett guillett deleted the github-actions branch April 25, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants