Sync API Entreprise/API Particulier fiches to data.gouv.fr - #264
Sync API Entreprise/API Particulier fiches to data.gouv.fr#264Samuelfaure wants to merge 20 commits into
Conversation
93ba634 to
beea280
Compare
|
Problème: il faut sync back le datagouv_uid depuis l'action github 🤔 mais on a des règles de protection de commits et de PR reviews, faut que je trouve comment bypass pour les actions github |
|
On ne peut pas passer par un moissonneur ? |
|
Ce |
|
Faire faire des commits à travers une API c'est une très mauvaise idée btw. Faut trouver une autre solution. Vu qu'il y a l'air d'avoir un search ( https://guides.data.gouv.fr/api-de-data.gouv.fr/reference/dataservices ) je pense qu'il faut explorer cette piste. Ça simplifiera. Par ailleurs, je vois qu'il y a des exclus du style DGFIP/URSSAF.. c'est voulu ? |
|
Imo destroy de deprecated bonne idée datagouv_uid ça simplifie la sync mais effectivement je suis pas fan de la solution actuelle, le search sera peut-être bien meilleur si ça marche (imo ça devrait passer, faut juste faire un search à chaque sync, un peu lourd mais plus simple) Je crois pas qu'on puisse être source de vérité sur datagouv_uid vu que c'est set par datagouv (à explorer si on peut pas le set nous-même, juste au cas où ça soit possible) - mais avec le search on peut peut-être juste jarter tout ça Le moissoneur j'ai reçu l'avis de @nicolaskempf57 qui m'a tldr dit "ne fait pas ça" (la team a l'air assez terrifiée du code relatif au moissonnage, la complexité semble +++) |
Pourquoi ? Je ne vois pas de use case en fait, vu qu'un bump de version on garde le même uid et on udpate la donnée.
C'est évident oui, mais ça ne change pas le fait qu'on devrait être le seul "pusher" de la donnée et qu'on ne devrait pas avoir besoin de faire des itérations techniques avec les ids dans notre codebase (c'est clairement une béquille actuellement), d'où ma proposition de faire des search (limite sur le champ de l'url qui est imo une clé valide). |
|
dans les cas comme DGFIP-TVA vaudrait mieux suppr la fiche non? J'ai du mal à voir pourquoi tu veux garder des fiches d'endpoints depreciés sur datagouv |
Ces cas se comptent sur les doigts d'une main (voir c'est le seul ..?) |
|
OKAY U GOT A POINT lol |
|
Cela étant dit c'est une balance code/logique métier à maintenir VS confort de supprimer les dépréciés sur datagouv. Vu qu'actuellement ce n'est de toute manière par réellement maintenu un j'aurais tendance à ne pas le gérer mais j'ai aucun strong belief la dessus |
|
je serais partisan de le garder si t'as pas d'opinion forte du coup, flemme de me rappeller de delete manuellement si le cas repop |
a9503c7 to
c6fc014
Compare
|
C'est techniquement prêt mais je suis pas sûr d'être satisfait du résultat, + faut tester en sandbox sur demo. Je demande pas encore les reviews du coup, je me laisse un peu de temps pour voir si je trouve mieux |
Le problème du moissonnage c'est que ça s'appuie sur un standard (DCAT principalement) et le format RDF. Donc ça veut dire faire un endpoint dédié de zero, avoir la même correspondance pour les champs qui n'ont pas toujours un mapping claire entre le standard et les données sur data.gouv, ce qui semble plus complexe à faire à maintenir par rapport à faire des appels d'API. Concernant la récupération de la "bonne" fiche à mettre à jour, si vous ne voulez pas de |
|
@nicolaskempf57 Big thanks pour ces conseils (c'est déjà ce qu'on fait en vrai avec l'ID DINUM) |
c6fc014 to
2f39345
Compare
|
Okay j'ai enfin trouvé le focus pour vérifier le taff ici, c'est prêt pour review du coup |
There was a problem hiding this comment.
Moi ca me convient dans l'ensemble niveau qualité du code.
J'ai une questions cependants, pourquoi avoir fait des services et clients and co et pas avoir suivi le "pattern coc" qu'on utilise partout et qui ferait le taff ici aussi ?
En vrai, techniquement je trouve ca tout aussi bien, c'est plus par convention dans la team
Sinon, le corps de la PR ne décrit plus trop les changements que tu as fais, j'imagine que ca correspond à une précédente opération.
Faudrait voir ce qu'il se passe si un fichier _swagger_shared change. J'ai l'impression que ca peut faire planter la tache et du coup tout le deploy et pour le coup je serait assez contre avoir des step bloquantes.
datagouv:sync takes uids via the SYNC_UIDS env var (comma-separated) -- the mechanism the GitHub Actions workflow uses, since Rake's bracket-arg CLI syntax (`datagouv:sync[a,b,c]`) splits on commas into separate positional arguments before the task runs and silently drops all but the first uid. The bracket argument remains a fallback for a single uid passed manually from a shell. datagouv:sync_all syncs every endpoint.
Runs on push to main (the auto-generated force-push from develop after CI goes green) when commons/endpoints/** changed, and computes the touched fiche uids from the push's before/after SHAs to sync only what changed; workflow_dispatch runs a full sync instead. DATAGOUV_HOST is a plain vars entry (swappable to point at production later) and DATAGOUV_API_TOKEN a secret. Sync is search-based (Datagouv::DataserviceIndex lists data.gouv.fr's own records and matches them by an embedded marker) rather than relying on a locally-stored id, so the workflow never writes back to the repo -- no repo write permissions needed, no branch-protection bypass required.
2f39345 to
dab9481
Compare
…ument sync_with_datagouv Three shared swagger yml files (cnous, france_travail, mesri) contain unquoted Date literals that Psych::safe_load rejects, so touching any of them would crash the "compute changed uids" workflow step and fail the whole sync job. _swagger_shared files never carry fiche entries anyway, so exclude them from the changed-file glob. Also documents datagouv_uid/sync_with_datagouv in the entreprise fiche template per review feedback on PR #264.
…nstead of a marker Per review on PR #264, the HTML-comment marker embedded in the description (with a title-match fallback) was fragile and more complex than needed: business_documentation_url is already unique per endpoint uid and comes back verbatim on every dataservice, so it works as the match key on its own. Drops DataserviceIndex's title fallback and by_marker/by_title split down to a single lookup, which also lets SyncFiche use #find for both the update and the deprecated-delete paths instead of a separate #marker_match.
|
@skelz0r C'est supposé être run avec un workflow github, donc pas possible de faire ça en job, github a pas accès à notre queue (à moins qu'on mette en place un systeme de call complexe maybe?) - mais ça fait sens d'être un job synchrone du coup |
Per review on PR #264, punchline embeds the endpoint's raw technical swagger description as bold Markdown text verbatim. Surveyed all 74 current endpoints -- none are actually unsafe today (max 264 chars, single-line, no markdown-special characters) -- but nothing enforced that going forward. Strips HTML tags and markdown emphasis/code/link characters, collapses newlines to spaces, and caps length so a future endpoint description can't break the fiche's rendering.
Pourquoi ? Tu n'es pas passé sur mes autres points btw. |
|
@skelz0r C'est juste plus pratique non? Surtout qu'on ne touche qu'aux fiches qui ont changées donc ça ne ralentis pas les autres déploiements Tu voudrais remplacer ça par un cron job toutes les nuits? Ça me semble un poil plus relou (oui les autres points je vais passer dessus) |
|
Plus pratique cela m'étonne, lancer un job au démarrage c'est 1 ligne dans un initializer, et on s'assure que ça run à chaque boot d'app VS un workflow github qui est moins déterministe et plus compliqué à maintenir/comprendre. Garder le workflow github why not, mais dans ce cas là je ferai tout en ruby pour simplifier ces quelques lignes de bash (qui sont peu lisibles) je lançerais à chaque déploiement sans faire de disjonction (comme ça c'est sûr que ça soit toujours à jour) et j'utiliserai un PORO et non une tâche rake. |
…iption Per further review on PR #264: the ad hoc <[^>]*> regex used to strip HTML out of the swagger description lived in FichePayloadBuilder and was weaker than what Rails already ships. Moves HTML-tag stripping into AbstractEndpoint#description_plain_text (via ActionView::Base.full_sanitizer, already a transitive dependency through rails-html-sanitizer/Loofah, no new gem), and renames the leftover punchline method/constant to summary_line since it now only handles the datagouv-specific markdown escaping and truncation.
6e62cde to
dd1e94b
Compare
|
Je trouve le workflow github plus safe (on a une meilleure vue dessus si ça foire VS lancer le job au boot) donc je vais faire les modifs dans ce sens |
Le job qui fail lève une erreur sentry et est résilient (si erreur réseau temporaire ça retry) contrairement à github qui va juste planter. Le fait d'utiliser du job async via notre pipe donne toute la stack by design pour gérer exactement ce genre de use cases (qui est de keep sync le code en prod avec des ressources externes). Après, le cas du fail n'existe pas encore, mais clairement si ça arrive je vais mécaniquement migrer vers du job car évident pour moi 😅 (vs des dizaines de lignes de yaml en dehors de la codebase rails). |
|
bon tu as de bons arguments je vais migrer en job 😅 |
Per review on PR #264: GitHub Actions had no reason left to own this sync once matching moved off datagouv_uid (no more write-back commit needed) -- replaces the site-sync-datagouv.yml workflow and the datagouv:sync/:sync_all rake tasks with Datagouv::SyncFichesRemoteService, wired up in an initializer the same way siade's SyncPingsWithMonitorsRemoteService already is: a Rails.cache-locked service, spawned in a Thread from after_initialize, production only. Every boot now does a full sync instead of only syncing uids changed since the last deploy; Datagouv::SyncFiche#unchanged? already skips no-op API calls, so this stays cheap. No manual rake-task escape hatch is kept, matching the mirrored pattern. Needs DATAGOUV_HOST/DATAGOUV_API_TOKEN added to site's own production environment before deploy -- today they only exist as GitHub Actions secrets.
site's Redis cache namespace bakes in Time.now.to_i at process boot (config/application.rb), so two dynos booting seconds apart in a rolling deploy get different namespaces and Rails.cache's unless_exist lock never actually crosses processes -- both would run the full sync concurrently and race create_dataservice calls for the same endpoint. Passing an explicit namespace: on the lock's write/delete/exist? calls overrides the store default for just this key, without touching the global cache config other features may depend on.
DatagouvAPIClient read DATAGOUV_HOST/DATAGOUV_API_TOKEN straight from ENV, unlike every sibling client (AbstractHubeeAPIClient, AbstractINSEEAPIClient) which goes through AdminApientreprise.credentials -- the only place site's own runtime actually provisions these secrets, since nothing in the app's environment config defined them (they only ever existed as GitHub Actions secrets for the now-removed workflow).
perform() ran unguarded inside its boot-time Thread: SyncRunner already rescues Faraday::Error internally, but anything else (a missing credential, a bug) died silently -- Thread.new swallows unhandled exceptions by default, and Sentry has no hook into raw threads the way it does for requests/jobs. Wraps the sync in rescue StandardError => e; Sentry.capture_exception(e), matching the DatapassWebhook::CreateFormulaireQFResources convention. Moves release_lock! into ensure as part of the same change -- adding the rescue without it would leave the lock held for the full hour on every reported failure, silently blocking retries with no error to explain why.
|
Pour info on peut tester en sandbox sans risque (ça envoie sur demo.data.gouv) |
skelz0r
left a comment
skelz0r
left a comment
There was a problem hiding this comment.
2 gros points:
- Transformer le service en job pour avoir toute la stack de retry/tracking etc..
- Le
summary_line
…the first Per review on PR #264: an endpoint can have several provider_uids, so picking providers.first silently dropped the rest from the fiche title and description. Claude-Session: https://claude.ai/code/session_01FraV2QQbbbGpUQnUiL8etP
Per review on PR #264: the swagger-description-derived summary line was hacky and the sanitizer it depended on (description_plain_text) had no reason to live on AbstractEndpoint since only the datagouv sync used it. Adds an optional, authored datagouv_summary attribute -- documented in both commons/endpoints/template.*.yml.example -- that FichePayloadBuilder prefers verbatim over the derived summary line when present. None of the existing fiches have it yet, so the derivation stays as a fallback; backfilling is editorial work for whoever owns each fiche. Also moves description_plain_text out of AbstractEndpoint into FichePayloadBuilder, where it's now only used by that fallback path. Claude-Session: https://claude.ai/code/session_01FraV2QQbbbGpUQnUiL8etP
Per review on PR #264: SyncFichesRemoteService mirrored siade's SyncPingsWithMonitorsRemoteService (Thread-spawned from an initializer, manual rescue + Sentry.capture_exception), but that pattern exists in siade because siade has no ActiveJob/GoodJob stack -- this app does. Datagouv::SyncFichesJob replaces it, enqueued via perform_later with retry_on so genuinely unexpected errors actually retry instead of being swallowed into a single Sentry report; sentry-rails already reports ActiveJob failures automatically once retries are exhausted, matching every other job in app/jobs/. Claude-Session: https://claude.ai/code/session_01FraV2QQbbbGpUQnUiL8etP
Backfills the datagouv_summary field added in a prior commit for all 55 non-deprecated, datagouv-synced fiches across both catalogs, so the data.gouv.fr description no longer depends on the swagger-description derivation for any of them. Also strips a trailing period from the authored summary the same way the derived one already was -- the markdown template appends its own period, so an authored sentence (naturally written with one) produced a double period otherwise. Folded the two summary_line code paths into one shared formatting step while at it. Claude-Session: https://claude.ai/code/session_01FraV2QQbbbGpUQnUiL8etP
4 participants
Summary
commons/endpoints/*.ymlto data.gouv.fr's dataservices API, triggered by a GitHub Actions workflow when fiche files change (push tomain, i.e. the develop→main CI force-push, filtered tocommons/endpoints/**).datagouv_uidvalues are written back into the yml automatically, committed todevelopvia the GitHub Contents API so the commit is GitHub-signed (required bydevelop's branch-protection ruleset).sync_with_datagouv: falseadded to all endpoint entries that share adatagouv_uidwith a sibling (deprecated/current pairs, grouped INSEE Sirene/DJEPVA entries), so the shared dataservice is never deleted out from under a still-live sibling.demo.data.gouv.frvia theDATAGOUV_HOSTGitHub variable — swapping it todata.gouv.frlater works identically, since the client has no hardcoded demo reference (also need to change DATAGOUV_API_TOKEN)Test plan
bundle exec rspec spec/services/datagouv/ spec/clients/datagouv_api_client_spec.rb spec/models/abstract_endpoint_spec.rb spec/lib/tasks/datagouv_spec.rb— 62 examples, 0 failuresbundle exec rubocopon all touched files — clean (one pre-existing, unrelated offense ininflections.rb)datagouv_uidwithoutsync_with_datagouv: falsedemo.data.gouv.fronce the bot identity is added to the "Secure contain protek" ruleset's bypass list ondevelop(required for the write-back commit'spull_requestrule, separate from the signing fix already in place)