feat: add workflow to check version consistency

[Pierlou]

No description provided.

[pierrecamilleri]

Je me suis permis de faire quelques remarques purement sur le code.

Sur les aspects fonctionnels, je pense que tu as répondu à l'oral sur les deux points ci-dessous, et je vais créer un ticket chantier-qualité pour la maj du pattern frictionless.

(remarques obsolètes :)

• Je pense qu'il y a un risque d'erreur si la version n'est pas précédée de la lettre v. Le pattern n'est pas explicite à ce propos, et l'exemple donné dans le pattern utilise même "version": "1.0.1",. *

• Est-ce que les champs homepage et version sont obligatoires dans les schémas ? Idem, comme c'est un pattern, je ne sais pas quelle est la politique schema.data.gouv

[Pierlou]

Merci pour cette review Pierre !
Pour les numéros de versions, nous cherchons à standardiser l'utilisation de vX.Y.Z pour les schémas de schema.data.gouv, dans un souci de cohérence, d'où ce choix. Nous pouvons aussi changer le pattern pour que le v soit facultatif, qu'en penses-tu ?
De même dans les schémas que nous supervisons, les champs homepage et version sont attendus, nous y serons vigilants.

[thbar]

Hello !

Merci pour la PR, le besoin de “vérifier que les liens qui contiennent une version cohérente” avant de générer le tag/la release est quelque chose qui mérite d’être creusé, comme en témoigne l’historique sur le schéma IRVE 🤣 (voir etalab/schema-irve#56).

Voici mes commentaires en première passe.

Lecture du code

En premier lieu j’ai ressenti le besoin de comprendre ce que faisait précisément la méthode def check. Je comprends l’idée générale (car j’ai implémenté quelque chose d’un peu similaire), le côté arbre récursif etc.

➡️ une petite explication juste au dessus de la méthode aidera les utilisateurs

Test sur schema-irve

Pour voir si ça fonctionnait “pour nous”, j’ai été intégrer le test dans https://github.com/etalab/schema-irve en local.

Le premier retour concret est que l’implémentation actuelle ne supporte pas la fonction data package que nous utilisons (https://github.com/etalab/schema-irve/blob/master/datapackage.json).

Ça pourrait être adapté à peu de frais, toutefois l’usage de data packages est récent, je ne suis pas sûr que ça intéressa beaucoup de monde avant un moment.

J’ai modifié le code en local pour aller lire statique/schema-statique.json par exemple, en changeant un peu les versions dans le JSON pour “créer des erreurs”, toutefois le code n’a pas remonté d’erreurs.

Donc je peux suggérer des tests en s’appuyant sur nos JSON ; je ne sais pas si le souci est dans nos JSON ou dans le test !

Packaging

Préoccupation plus long terme, si on devait à un moment utiliser cela de façon standardisée sur beaucoup de schémas, ça pourrait être une bonne idée de shipper plutôt une “GitHub action”, qu’on pourrait intégrer dans les workflows, pour faciliter l'intégration et la mise à jour.

On pourrait avoir des validations lancées “lors des releases”, pour bloquer une release si elle ne respecte pas les bonnes conditions, ce qui serait top.

Voilà c’est tout pour aujourd’hui, j’espère que ça te sera utile !

[Pierlou]

Merci pour ce retour Thibaut ! 🙏 Point par point :

• en effet le code n'est pas forcément intuitif, je vais ajouter des commentaires. En quelques mots ici : on parcourt le schéma, et pour chaque champ on va vérifier (récursivement) que tous les sous-éléments qui contiennent une mention du schéma (via regex) sont bien tous conformes à la version mentionnée dans le champ version
• pour le test sur le repo IRVE, c'est attendu en réalité, je me doutais qu'il faudrait adapter le code pour créer un workflow dans le template datapackage. Je peux modifier le code pour qu'il s'adapte aux deux cas sinon
• je ne suis pas sûr de voir la différence avec le fait de mettre ça en Github action, tu peux préciser stp ?

[Pierlou]

Update : j'ai modifié le code pour prendre en compte les datapackages et j'ai initié une PR sur le repo template : datagouv/datapackage-template#1 (qui montre d'ailleurs une incohérence due à l'absence d'un v comme tu le faisais remarquer Pierre). Tu pourras tester de ton côté Thibaut ?

[thbar]

Tu pourras tester de ton côté Thibaut ?

@Pierlou merci ! J'ai bien noté, je testerai et je te ferai un retour global aussi.

[pierrecamilleri]

... Nous pouvons aussi changer le pattern pour que le v soit facultatif, qu'en penses-tu ? ...

Comme nous sommes à l'origine du pattern et que nous avons l'opportunité de participer au groupe de travail pour la v2 de la spec, je dirais qu'on a tout intérêt à essayer de pousser un format unique avec le "v" ! (et donc garder le comportement actuel de ton script)

[Pierlou]

je dirais qu'on a tout intérêt à essayer de pousser un format unique avec le "v"

En réalité ici il s'agit uniquement de la détection des versions dans le schéma, pas de l'intention du choix du format. Dans la dernière version du script, le pattern cherche avec et sans le "v", pour détecter les cas où un schéma aurait des dissonances du type v1.0.0 VS 1.0.0. En parallèle, je suis effectivement d'accord pour qu'on pousse pour un formalisme unique 🤝

[pierrecamilleri]

En réalité ici il s'agit uniquement de la détection des versions dans le schéma, pas de l'intention du choix du format. Dans la dernière version du script, le pattern cherche avec et sans le "v", pour détecter les cas où un schéma aurait des dissonances du type v1.0.0 VS 1.0.0. En parallèle, je suis effectivement d'accord pour qu'on pousse pour un formalisme unique 🤝

Entendu, c'est parfait !

[Pierlou]

Poke @thbar est-ce que tu as pu te pencher sur le sujet ? 🙏 J'aimerais le passer sur d'autres schémas (on a beaucoup de cas où ce serait bénéfique)
La version pour datapackage est ici : datagouv/datapackage-template#1

[thbar]

@Pierlou je viens de faire un test, voilà l'output !

Versions are mismatched within the schema 'schema-irve-statique', expected version '2.3.0' but:
- path has version 'v2.3.0'
- resources[0].path has version 'v2.3.0'

Versions are mismatched within the schema 'schema-irve-dynamique', expected version '2.3.0' but:
- path has version 'v2.3.0'
- resources[0].path has version 'v2.3.0'

Voir:

• https://github.com/etalab/schema-irve/actions/runs/8551361252/job/23430211578?pr=58
• Ajout du préfixe "v" devant le numéro de version + intégration de "checks" automatisés etalab/schema-irve#58

C'est déjà chouette, je vais amender dans ce sens !

Il serait utile, à terme, d'avoir un check "pre-release" qui vérifie quand on pose un tag/crée une release que le tag ex: 'v2.3.1` est raccord avec ce qui est dans le schéma, et d'empêcher la création du tag/release si cela est possible ! (là ce n'est pas raccord chez nous, par exemple).

Chouette boulot, ça me paraît bien, je vais conserver de notre côté.

[Pierlou]

Merci pour ce retour ! En effet dans l'idéal ce serait bien d'avoir un process qui gère un peu tout (checks de cohérence, montée de version...). On pourrait imaginer un script qui passe lors d'une nouvelle release, mais je ne suis pas sûr de voir comment ça s'articulerait : l'action ici est sur un push, donc on a encore le temps d'amender avant de merge, mais lors de la création d'une release on n'a pas cette étape intermédiaire, donc on serait sur un test a posteriori ? Preneur d'idées sur le sujet

[Pierlou]

On a l'air d'avoir atterri sur un bon premier jet, merci à tous pour vos retours ! Je vous propose de continuer nos échanges sur le sujet dans cette issue