Validation des PR sur les apps officielles

Contribute Room Apps packaging
1

Semblerais donc que nous ayons un conflit de décision sur la gestion et l’ouverture de la validation des PR sur les apps officielles.

Voir ici pour le débat en question: Add upgrade from a previous commit by maniackcrudelis · Pull Request #30 · YunoHost-Apps/opensondage_ynh · GitHub

Alors soit, discutons-en.

Pour rappel, lors d’une pull request sur une app officielle, nous utilisons un modèle, disponible au complet ici: https://github.com/YunoHost/project-organization/blob/master/apps_group_PR_model.md

Nous avons 5 points différents à tester, en fonction du niveau de validation requis par la PR (sachant que nous utilisons essentiellement les décisions mineures pour les PR habituelles sur les apps officielles)

  • Complete test
  • Upgrade previous version
  • Code review
  • Approval (LGTM)
  • CI succeeded

Complete test correspond à un test complet de l’application et de ses fonctionnalités. Donc de s’assurer que l’app fonctionne correctement.
C’est assez rarement demandé, car nous utilisons essentiellement des décisions mineures.
Upgrade previous version est une mise depuis la version actuellement dans la liste (ou plus ancienne) vers la version de la pull request. L’idée ici est de s’assurer que l’upgrade ne casse pas l’app.
Ce test est en cours d’automatisation par le CI. Il ne devrait donc plus être nécessaire de le faire manuellement, sauf pour la décision majeure ou en cas d’inquiétude particulière pour l’upgrade.
Code review est une relecture approfondie du code modifié, pour s’assurer qu’il n’y a pas d’erreurs.
Approval (LGTM) est un accord de principe, cela signifie que l’ont donne son accord sur la finalité de la PR et ses éventuelles conséquences sur l’application.
CI succeeded est une validation par Package check que la PR n’implique aucune régression.
Ce test est automatisé par le CI dev dans le cas d’une branche créée sur le dépôt, en cas de PR depuis un fork, il faut cloner la branche manuellement sur ce serveur.

Les règles de validation actuelles sont disponible ici: https://github.com/YunoHost/project-organization/blob/master/yunohost_project_organization_fr.md#2-revue-et-validation-collective
Elles impliquent ceci:

Si l’auteur ne fait pas parti du groupe concerné par la PR, tous ces quotas sont augmentés de 1. Dans tous les cas, ces quotas doivent être remplis au moins à 50% par des relecteurs membres du groupe concerné par la PR. […]

Ce n’est pas véritablement appliqué en l’état, pour 2 raisons, nous utilisons un modèle de validation légèrement différent, ainsi qu’un modèle de PR que nous dupliquons sans se prendre la tête.

Toutefois, une proposition a été faite dans le même sens:
Elle est disponible ici: https://github.com/YunoHost/project-organization/pull/54#discussion_r125982193
Et en substance propose cela (en convertissant approximativement au modèle utilisé pour les apps):

  • Le test complet pourrait ou pas être fait par n’importe quel contrib…
  • Un upgrade depuis la précédente version peut être fait par tout contrib. Un log me semblerais tout de même utile, pour voir si un truc n’est pas passé inaperçu. sauf si c’est une version en prod.
  • Une review de code peut être faite par tout contrib, (À partir du moment où ce contrib est reconnu comme étant capable de lire et comprendre le code)
  • Un accord de principe devrait provenir uniquement d’un membre du groupe Apps.
  • Le test de CI peut éventuellement être fait par n’importe qui (avec un log consultable). Mais le CI s’en charge généralement pour nous.

En somme, toutes les validations peuvent être faites par des contrib non membres du groupe Apps, à l’exception des accords de principe qui seraient réservés au groupe Apps.
Ce dernier point s’explique par le fait que c’est la responsabilité du groupe Apps de vérifier chaque PR sur les apps officielles et également d’avoir un oeil sur les modifications apportées.

2

(00:27:05) irc: <frju365[m]> euh moi, je dirais que notre système de vérification des PR officielles est déjà bien comme ça. Je ne pense pas que faire contribuer un contributeur extérieur à l’app (et extérieur au gr apps) soit si bien selon moi.
(00:27:33) irc: <frju365[m]> (si c’est vraiment ça dont il est question)
(00:30:08) Maniack C: La question est plutôt de clarifier ce point, qui avait déjà été soulevée
(00:31:07) Maniack C: Après faut bien voir qu’on se dirige vers des PR avec 3 validations seulement
(00:31:20) Maniack C: 1 review de code et 2 LGTM uniquement
(00:31:20) frju365: ben, après faire participer la communauté, c’est super… mais je pense que cela est quand même risqué quand il s’agit d’une review du code de l’app.
(00:32:32) Maniack C: J’aurais tendance à dire que si le contrib n’a pas ta confiance, rien n’empêche de faire une review de code toi aussi
(00:33:12) Maniack C: Mais on peut également avoir des contrib bien connu dont on sait qu’il auront lu le code correctement
(00:33:43) Maniack C: Mais, ce topic est là pour ouvrir le débat, si il doit y avoir
(00:34:01) Maniack C: Alors n’hésite pas à donner ton avis
(00:34:06) frju365: pour des reviewers avertis, je suis ok.
(00:34:25) Maniack C: C’est un point peu clair, qui mériterais sans doute d’être éclairci
(00:34:32) frju365: Jean-Baptiste et rafi ont par exemple toute ma confiance.
(00:34:53) Maniack C: Un reviewer non connu pourrait aussi faire ces armes par ce biais
(00:35:09) Maniack C: Tout en étant doublé par un autre si c’est quelqu’un qu’on ne connait pas
(00:35:59) Maniack C: Et même sans être stigmatisant, tu peux donner un accord et revérifier le code sans rien dire
(00:36:08) Maniack C: Juste pour être sûr
(00:36:42) frju365: euh, si c’est qqn qu’on ne connait pas, je dirais que de toute façon la review n’est pas recevable. Dans les faits, on ne peut pas reviewer une app comme ça, sans connaître bien les helper, les discussions ici et les apps elles-mêmes.
(00:37:07) frju365: (Il faut clarifier ce point… même si je dirais que ce serait plutôt en réunion mumble qu’il faudrait en discuter.)
(00:37:17) Maniack C: Je suis d’accord avec toi sur ce point
(00:37:32) Maniack C: Mais j’y vois aussi un point d’entrée pour des nouveaux contrib
(00:37:51) Maniack C: Pour ce cas là, je préferais doubler la review
(00:38:11) Maniack C: Le problème c’est qu’on aura pas tout le monde sur mumble pour cette question je pense
(00:39:42) frju365: oui. Tout à fait d’accord. Cela peut pousser des gens à contribuer… mais peut-on vraiment reviewer une app en ne connaissant pas les autres helper… est-ce que ce ne serait pas mieux de le pousser à regarder les nouveaux helper ? en plus, “nouveaux contrib”… oui et non, j’espère qu’ils ont déjà essayé de packager qqch avant des reviewer une app officielle quand même.
(00:40:02) Maniack C: Oui c’est pas faux :wink:
(00:40:38) Maniack C: Toutefois, comment tu définis le reviewer de confiance sans lui coller une étiquette ?
(00:42:29) frju365: Ben, qqn déjà qu’on a vu plusieurs fois, qui a fait bcp d’app ou alors une ou deux app, qu’il maintient, avec un plus ou moins bonne qualité.
(00:42:52) Maniack C: Oui je suis d’accord, par contre c’est très relatif
(00:43:09) frju365: oui, mais bon… c’est relatif, mais c’est humain.
(00:43:17) Maniack C: Du coup j’aurais tendance à dire qu’on laisse des contribs faire une review de code
(00:43:31) Maniack C: Et qu’on juge nous même si on veut revoir le code ou pas
(00:44:00) Maniack C: De toute manière, ce serait un peu étrange qu’on ai 2 LGTM sans review de code
(00:44:38) Maniack C: Donc on devrait toujours avoir quelqu’un de confiance qui repasse derrière et pourrait juger qu’il devrait mieux relire le code lui aussi
(00:44:43) frju365: qqn qui n’a jamais parlé, jamais packagé, qui ne connait pas l’app et qui fait un review du code de l’app… mille excuses mais ma réponse est non.
(00:44:45) frju365: oui
(00:45:00) Maniack C: Je suis d’accord avec toi
(00:45:22) Maniack C: Disons que sinon, les validations sont inutiles
(00:46:22) frju365: Les contributeurs de confiance pourraient être des préadmis dans le groupe app… et donc qui seraient élus pas le groupe app.
(00:46:50) frju365: (c’est une idée…)
(00:47:11) Maniack C: Tu es vraiment contre l’idée que tout le monde puisse tenter sa chance ?
(00:47:32) Maniack C: Et qu’on juge de la validité nous même
(00:48:04) Maniack C: Parce qu’au final je pense qu’on ai vraiment des cas d’arrivistes qui viennent faire des review comme ça
(00:48:10) Maniack C: *pas
(00:48:59) frju365: ben, moi, je suis contre. Pour reviewer des apps officielles, je pars de l’idée qu’il faut avoir travaillé sur d’autres apps avant… donc, ce n’est déjà plus tout le monde.
(00:49:39) Maniack C: Oui… la notion d’Officielle me revient soudainement
(00:49:40) frju365: et puis, ces arrivistes seraient plus utiles d’abord pour les apps communautaires.
(00:49:53) Maniack C: C’est sûr
(00:50:04) Maniack C: Mais on a pas trop de PR sur ces apps
(00:50:11) Maniack C: Mais oui en effet
(00:50:38) Maniack C: Donc en gros ce serait seulement de contribs connus
(00:50:49) frju365: je pense qu’un arriviste devrait d’abord s’attaquer à des apps communautaires (au moins 4 PR) avant de prétendre réviewer un app officielle mais après c’est mon point de vue.
(00:51:07) frju365: ben, oui… ce sont des apps officielles quand même…
(00:51:16) Maniack C: Non mais c’est vrai qu’on est toujours sur cette notion de qualité quand même
(00:51:23) Maniack C: C’est le but de ces vérifs
(00:51:37) frju365: c’est un peu comme si on sponsorisait ces paquets.
(00:52:04) Maniack C: J’ai pas saisi là
(00:52:25) frju365: ben, il y a une légitimité et un investissement.
(00:53:10) Maniack C: Ah ok
(00:53:11) frju365: bon, après… ok. Ma métaphore n’était pas celle-ci dans ma tête :wink: (je suis un peu fatigué, il faut dire)
(00:53:42) Maniack C: De même
(00:54:08) Maniack C: Euh, ça te va si j’ajoute cette discussion au forum, plutôt que de tenter un résumer bancal ?
(00:54:37) frju365: euh oui, c’est ok
(00:56:33) Maniack C: Done
(00:56:35) frju365: Moi je suis qqn de très porté sur la communauté et l’esprit communautaire… mais là, c’est quand même Yunohost, c’est dans un cadre officiel, et le groupe app a quand même était fait pour ça.
(00:56:44) frju365: ok
(00:57:08) Maniack C: Oui je suis plutôt d’accord avec toi
(00:57:27) Maniack C: Les apps officielles sont censées être de qualité
(00:57:42) Maniack C: Et le rôle du groupe Apps est principalement de s’en assurer
(00:58:28) Maniack C: Juste on devrait intégrer aussi le mainteneur de l’app à toutes les décisions concernant son app
(00:58:40) Maniack C: Je pense
(00:58:51) frju365: oui, ça, j’ai oublié d’en parler, mais c’est aussi ce que je pensais.
(00:59:00) frju365: c’est même très important selon moi
(00:59:15) Maniack C: Ben oui quand même !
(00:59:33) frju365: c’est quand même celui qui connaît le mieux le travail qu’il a fait.
(01:00:29) frju365: et puis l’app elle-même.
(01:01:24) frju365: même LGTM, c’est délicat pour qqn qui ne connaît pas l’app…
(01:01:29) frju365: *bien l’app
(01:02:33) frju365: comment décider si qqch est bon ou pas quand on ne connaît pas bien les enjeux de la PR ?.. c’est de ça dont il est question aussi.
(01:03:11) Maniack C: Ben c’est pour ça que moi je suis pour limiter LGTM au groupe apps ou au mainteneur
(01:03:35) frju365: +1 aussi.
(01:03:53) Maniack C: Et aussi que le groupe apps est pas impliqué, ça veut dire que des PR peuvent passer sans que le groupe les voit
(01:04:37) frju365: oui, en effet.

3

Bonjour @Maniack_Crudelis et @frju365 ,

Pour poser le contexte, je ne suis qu’un simple utilisateur qui malheureusement n’a ni les compétences, ni le temps de prendre plus part au projet yunoHost que ce que je ne peux le faire actuellement sur le forum de temps à autre.

Ceci dit, je suis plutôt d’avis de distinguer les applications officielles des applications communautaires.
Les applications officielles étant la vitrine du projet, elles doivent donc être fonctionnelles et maintenues.

De mon point de vue, si les contributeurs aux applications communautaires souhaitent aller plus loin, en l’occurence passer les applications au statut officielle, alors il faudrait qu’ils fassent partie intégrante du groupe applicatif.
Ce passage sous entend une véritable implication sur a minima du moyen terme -sauf situation particulière- afin de ne pas sortir une application un jour puis qu’elle se meurt ensuite par défaut de maintien (mise à jour, correctifs, compatibilité, …).

Au delà de ça, aujourd’hui et toujours de mon point de vue, une priorité doit être donner au passage de Jessie à Stretch a minima dans ces 2 cas de figure : nouvelle installation et mise à niveau d’une installation existante avec une non-casse des applications officielles.

Ensuite, il pourra être discuté d’une liste des applications qui se doivent prioritairement d’être officielles dans le cadre de l’auto-hébergement :

  • serveur mail ;
  • nuage (NextCloud) ;
  • administration de base de données (phpMyAdmin) ;
  • webmail (Roundcube et/ou Rainloop) ;
  • synchronisation des contacts et agendas (Baikal, ) ;
  • application web personalisé avec accès (S)FTP (Custom Webapp) ;
  • lecture-pour-plus-tard (Wallabag) ;
  • blog (WordPress) ;
  • faire des sondages (OpenSondage/Framadate et Framaform) ;
  • client/serveur OpenVPN ;
  • Collabora/Collabora online ;
  • Mastodon ;
  • Diaspora ;
  • Tor Relay ;
  • etc.

Bref, YunoHost est un chouette projet et il doit être poursuivi.
Ces questions de choix et/ou de gouvernance à la veille de ce point important et inédit qu’est le passage à une nouvelle version basée sur Stretch est un point d’étape qui doit permettre de rencentrer et redonner de la cohérence au projet ainsi qu’à sont but initial : aider et faciliter l’auto-hébergement.
C’est seulement une fois le passage sous Stretch fait que vous pourrez y voir plus clair. C’est à ce moment précis que vous allez voir quels sont à lors les contributeurs d’applications non-officielles qui vous suivent dans l’aventure et qui s’efforceront d’adapter leurs contributions sur un plus long terme.

<3

ppr

4

Coucou Maniack,

franchement non, on n’a pas de conflit. Ça prend du temps de faire de son mieux pour bien faire les choses. Et parfois il y a des délais monstrueux entre la réalisation du travail puis la validation. Pour au final avoir des bugs qui montrent que le travail de test externe n’a pas été mené.

Donc oui, je donne parfois un coup de pied dans la fourmilière
pour exprimer la lenteur de nos processus.
Je constate, et ce n’est peut-être pas corrélé que ça semble aller plus vite maintenant et ça me réjouis.

Peut-être qu’une simple proposition de rejoindre le groupe apps aurait arrondi les angles, ou même simplement une formulation de phrase qui permette d’exprimer que tu es navré de cette situation imparfaite.

J’ai passé du temps à normaliser plusieurs applications, c’est long, c’est rébarbatif et pourtant j’apprécie de faire de ce job, peut-être qu’un poil de reconnaissance ponctuel ferait du bien.

Par exemple, la réaction sur le cache de package_check est révélateur. Le rejet de l’idée plus encore que ça faisabilité est dommage. In fine, ça marche, c’est efficace. Faudrait peut-être ajouté une tâche cron pour le CI, mais pour un contributeur d’une application ça fait parfaitement le boulot.

Nous faisons collectivement un travail important. J’aime ce que nous faisons et je respecte beaucoup ton travail. Je t’ai demandé de nombreuse fois des conseils, des relectures et d’apprendre de toi. Nous sommes simplement très différents ce qui crée des frictions inutiles.

À bientôt,

5

Je m’efforce de soulever la problématique de manière neutre.
Mais si tu veux être personnel, je peux l’être également.
Toutefois, et tu le sais sans doute depuis le temps, je ne suis guère politiquement correct…

Donc pour faire simple, je ne cherche pas à “arrondir les angles”, ça se saurais si c’était mon genre…
Et je ne suis pas “navré de la situation imparfaite”, qui bien qu’imparfaite est bien mieux que ce que nous avions jusque là. Et qui, si elle peux parfois être longue et fastidieuse, à le mérite de fonctionner et de donner des résultats pour le moins intéressants.
Quand à la reconnaissance… J’entends et je comprends. Mais je ne la recherche ni ne la dispense… Je ne suis celui de qui il faut en attendre.

Si tu as avancé sur la gestion des caches pour package check, une PR avec une version finalisée n’est toujours pas rejetée. Tout comme elle ne l’a jamais été…

6

Ben, je ne suis pas trop d’accord avec toi. Comme je l’ai déjà dit à Maniack, le groupe Apps a une raison d’exister et une des raisons (parmi tant d’autres) est celle-ci, c’est à dire l’inspection des apps officielles. Commencer à faire de l’auto-gestion ou de la gestion extérieur n’est pas super, et on perd l’idée que ce sont des apps OFFICIELLES. C’est Yunohost qui est garant de ces apps et c’est ici toute leur légitimité.

Pourrais-tu donner des exemples concrets ? il est vrai que parfois des erreurs se glissent, mais ce sont rarement des bugs majeurs. De plus notre processus semble bien fonctionner pour l’instant.

Au sujet de la rapidité de review, je me souviens vaguement de la philosophie de Debian à propos de la stabilité : une application doit être testée longuement avant de pouvoir sortir au grand jour. Pour tout dire je trouve que + de 3 ans pour sortir un distro, c’est un peu long, mais au moins il n’y a rien à redire quant à sa stabilité.

Enfin, je ne pense pas que la reconnaissance doit être faite seulement à l’intérieur d’une d’une équipe et qu’il doit être individuel. Beaucoup d’utilisateurs nous remercient, et ce sont eux qui reconnaissent le plus notre travail, ce sont les utilisateurs finaux. De plus ce n’est pas une reconnaissance individuelle, mais un reconnaissance collective. Enfin, je ne pense pas que Yunohost ou une quelconque communauté sur internet prônant le Libre soit faite pour avoir de la reconnaissance.

Autrement, je pense que tout le monde dans Yunohost est assez productif et avance vite. Peut-être qu’en ce moment, il y a qqe lenteur mais cela tend à s’améliorer ce qui est d’autant plus positif (et ce n’est pas pour cela qu’il faut changer notre processus de review).

Ce n’est que mon point de vue et je sais qu’il est discutable (je suis ouvert à tout argument, même visant à me faire changer d’avis :wink: ). Moi je trouve que tu fais du très bon travail en tout cas.

frju365