search

YunoHost-Apps / jellyfin_ynh

Jellyfin package for YunoHost
https://jellyfin.org/
GNU General Public License v3.0
25 stars 24 forks source link

Don't Touch Master or Testing without Ask. #41

Closed liberodark closed 3 years ago

liberodark commented 3 years ago

@ericgaspar @tituspijean merci de passer par des PR et en ayant tout testé.

tituspijean commented 3 years ago

@liberodark, regardless of being the app's maintainer:

1) The app was failing Continuous Integration and could not be installed since at least June. 2) Putting the app in YunoHost-Apps means that the community can step in to help and fix things, there is no need to be territorial. 3) Due process was followed. Dev branch > PR > Testing branch > PR > Master, with quite enough time to step in and review it. 4) Since the app was not installable, the "upgrade from older commit" test was overlooked by the CI (Step 5/9). If you have a bug regarding the upgrade of the app, you are most welcome to open an issue regarding this.

kay0u commented 3 years ago

The application had been level 0 for a little while and did not follow the recommendations. You are the maintainer of the application, but you don't own it, and others have the right to contribute to it. Moreover they used the ci-dev before merge in testing. This PR was opened almost a month ago and it greatly improves the quality of the package since it takes it from level 0 to level 7.

There is no reason to revert the work they did. If there is a problem when upgrading from the old version (which seems normal to me since it was level 0 and nobody could install it to try the upgrade), instead of reverting the work, we must open an issue with logs, in order to move forward and remain constructive. I'm sure they will be happy to help on this app that YunoHost really lacks in a decent quality.

So thank you to open a new issue, to bring logs, and please, don't do that again.

liberodark commented 3 years ago

@kay0u Im go to revert again this work broke the APP and you CI dont make all tests... Your work is not good @tituspijean this master make app broken... Yes is work in your CI but your CI is not good. Im reworking that but don't touch that. please again... When have finish app go to LVL 7 too.

@alexAubin J'ai mes raisons d'avoir fait ce que j'ai fait. Cela fait seulement 6 jours que la demande de PR à était faite car elle était fini. Et il y a pas mal de soucis dedans un ce n'est pas un bon point. Cela casse l'application alors je ne comprends pourquoi vous voulez forcer une PR. Qui rend instable l'application de plus votre Ci n'est pas un gage de fonctionnement ou stabilité.

Comme je l'ai dit je suis pas contre cette PR. Car je bosse dessus mais je suis contre le fait de faire n'importe quoi.

Si on doit faire de la m**** sur les projets des autres alors très bien vous pourrez me le préciser et je ferait l'app de mon coté. Sachant que j'ai poussé une bonne partie de vos apps.

Thanks

liberodark commented 3 years ago

Now app is LVL 7 and non broken Have fixed issues and now master is for YNH 4.x and have specific branch for YNH 3.x

FR : C'est une bonne chose que vous participiez mais je suis le mainteneur de cette application et ceux depuis les débuts. Il y a des soucis qui sont propre à cette applications voilà pourquoi je ne voulais pas tout casser. Je comprend que votre CI vous importe autant et je ne suis pas contre des PR bien au contraitre sauf quand cela peut planter l'application comme vous l'avez fait. Et vous arrivez ainsi en cassant une app que je maintient depuis longtemps. Je remercie les contributeurs pas votre comportement il aurait était utile de bien vérifier le comporte de l'application et votre CI ne le controle pas. Donc s'il vous plait ne forcer pas de PR car il y a de bonne raisons au fonctionnement actuel. Normalement l'app est maintenant Niveau 7 sur votre CI sans cassé la compatibilité ARM. Ce qui est un probleme spéficique à cette application. Et si vous auriez patienter j'aurais bien sur merge la PR avec les changements et correctifs de pourquoi cela ne va pas.

EN : It is good that you are participating but I am the maintainer of this application and those from the beginning. There are concerns that are specific to this application that's why I didn't want to break everything. I understand that your CI matters to you as much and I am not against PR, much to the contrary except when it can crash the application like you did. And you do this by breaking an app that I have maintained for a long time. I thank the contributors not for your behavior it would have been useful to check the behavior of the application and your CI does not control it. So please don't force a PR as there are good reasons for how it works now. Normally the app is now Level 7 on your CI without breaking ARM compatibility. Which is a problem specific to this application. And if you would have to wait I would of course merge the PR with the changes and fixes for why this is wrong.

alexAubin commented 3 years ago

  1. You're still not explaining why/what it supposedly broke, despite the team being genuinely interested in what happened exactly.

  2. Yes, the app passing the tests on the CI is not a 100% guarantee that the app works, but here's we're talking about an app not passing any tests on the CI, which usually is a pretty good indicator that the app really is broken. So nobody "broke" anything by merging the PR, considering the app was already broken since quite a while. If you have some ideas on making the CI tests more accurate and reliable, we would be glad to hear about those.

  3. The initial PR proposing a rework of the app is from ~28 days ago. Contributors pinged you multiple times for a review or any thoughts on it, and you did not respond.

  4. This app is in the Yunohost-Apps organization. The very purpose of this organization is, among other things, that the packaging community can come and contribute / maintain the application ecosystem. Especially because one may stop maintaining an app he/she was previously maintaining without a warning.

  5. And then yes, when it comes to apps that are broken according to our automatic testing, we tend to take the liberty to merge without an explicit approval, because, again, can't hurt that much considering it's already broken. (And again in that case, there was plenty of buffer time to react).

Summarizing : volunteers used their free time and energy to fix your app for free, politely asked for feedback that you did not provide, yet you suddenly show up and throw a drama because you're claiming they broke "your" app without explaining why, and aggressively revert their work. What do you think that sounds like ?

If you disagree with the fact that the community may intervene on broken apps (or other critical issues), you are free to create app repositories anywhere else - just understand that Yunohost will tend to discourage admins to install apps that are not maintain-able by the community.

Si on doit faire de la m**** sur les projets des autres alors très bien vous pourrez me le préciser et je ferait l'app de mon coté. Sachant que j'ai poussé une bonne partie de vos apps.

If you really want to play with this : let's have a look at the numbers. There are 22ish apps where you are flagged as maintainer. 5 of them are actually flagged as "working" on the app list, and only 3 are levels 7 ... and pretty sure that's thanks to the same people that worked on fixing Jellyfin ! (Basically the same people that maintain the currently 160+ level 7 apps of the Yunohost catalog) So maybe you can give them a bit of credit too, 'cause apps don't magically pass level 7...

tituspijean commented 3 years ago

@kay0u and @alexAubin have covered the team-work-related issues. Let me get into specifics. I am trying very hard to not talk shit like you are doing.

this work broke the APP

Et il y a pas mal de soucis dedans un ce n'est pas un bon point.

Have fixed issues

I have the CI, my installations in a virtual machine and on a Raspberry Pi that I used last night to prove you otherwise. Please, please, explain how this app after our fix was broken for you.

What issues?

Cela casse l'application alors je ne comprends pourquoi vous voulez forcer une PR.

How was it broken? We "forced" the PR because no work was being done on it.

Qui rend instable l'application de plus votre Ci n'est pas un gage de fonctionnement ou stabilité.

Describe "unstable". And yes, correct, the CI is not an omnipotent indicator, but it is still a good one. And if upgrade fails, it's great we have the backup system, right?

Now app is LVL 7 and non broken

Normally the app is now Level 7 on your CI without breaking ARM compatibility.

How have you tested it, to say it is level 7? You have no test under your name in the dev CI. If you rely on the badge in the README, this badge was generated from our fix, not your. (check the date)

It is good that you are participating but I am the maintainer of this application and those from the beginning.

Then do not abandon it for months. Your patch made in September did not fix it for me, and the CI agrees.

There are concerns that are specific to this application that's why I didn't want to break everything.

What. are. the. concerns. Do not behave all mysterious, if there are specific steps to take to package this app, disclose them.

I cannot say anything but thank you for making me discover Jellyfin after the Emby drama. But here you are not behaving like a team player. We want to understand what, technically, your issue is with our fix.

liberodark commented 3 years ago

@alexAubin Pardon je répond en FR je suis trop fatigué :D

  1. You're still not explaining why/what it supposedly broke, despite the team being genuinely interested in what happened exactly. Le problème viens de pas mal de chose autre le style que j'aime pas mais c'est pas trop grave. Le soucis est sur ARM principalement le fait qu'il sont un soucis bien spécifique avec ffmpeg qui si il est utilisé depuis leur dépot vous etes sur que votre applications ne peut lire aucun film. N'avré il était 6h du matin j'ai zappé...

  2. Yes, the app passing the tests on the CI is not a 100% guarantee that the app works, but here's we're talking about an app not passing any tests on the CI, which usually is a pretty good indicator that the app really is broken. So nobody "broke" anything by merging the PR, considering the app was already broken since quite a while. If you have some ideas on making the CI tests more accurate and reliable, we would be glad to hear about those.

Considéré comment cassé ne veux pas dire quel ne fonctione pas encore une fois votre CI n'est en aucun cas un gage de fonctionnement. Après cela ce discute. Disons aussi que j'aime pas trop l'approche de passé par des Call pythons. Mais c'est une autre histoire.

  1. The initial PR proposing a rework of the app is from ~28 days ago. Contributors pinged you multiple times for a review or any thoughts on it, and you did not respond.

Oui effectivement j'ai pas vu son travail depuis 28 jours c'est exacte. Je ne connaissez pas cette PR avec le confinement j'ai eu pas mal de soucis. De plus je participe sur pas mal d'autres projets. Et je n'ai pas vu ce point précise. J'en suis désolé.

  1. This app is in the Yunohost-Apps organization. The very purpose of this organization is, among other things, that the packaging community can come and contribute / maintain the application ecosystem. Especially because one may stop maintaining an app he/she was previously maintaining without a warning.

Bien sur cela je le comprend parfaitement.

  1. And then yes, when it comes to apps that are broken according to our automatic testing, we tend to take the liberty to merge without an explicit approval, because, again, can't hurt that much considering it's already broken. (And again in that case, there was plenty of buffer time to react).

Je peux le comprendre mais avant d'etre un maintenaneur je suis aussi un utilisateur de cette apps. Ayant pas réussi à comminiqué sur element j'ai eu personne. Cela ma déranger sur le coup mais j'ai quand meme commencer à regarder ça PR car je respecte tout travail.

Summarizing : volunteers used their free time and energy to fix your app for free, politely asked for feedback that you did not provide, yet you suddenly show up and throw a drama because you're claiming they broke "your" app without explaining why, and aggressively revert their work. What do you think that sounds like ?

Ce n'est pas la PR le problème en aucun cas. Encore cela enerve mais ça va. Comme vous l'avez vu au début j'ai rien dit de spécialement seulement :

@ericgaspar @tituspijean merci de passer par des PR et en ayant tout testé.

Car bon c'est aussi moi qui est pas vu cette PR. Non la ou j'ai était énervé c'est avec @kay0u car j'ai pas meme pas eu le temps de finir le taff pour le remettre dans le master sans casser l'app.

If you disagree with the fact that the community may intervene on broken apps (or other critical issues), you are free to create app repositories anywhere else - just understand that Yunohost will tend to discourage admins to install apps that are not maintain-able by the community.

Cela il y a pas de problème et vous le savez je suis très cool et ne dérange pas pour rien.

Si on doit faire de la m**** sur les projets des autres alors très bien vous pourrez me le préciser et je ferait l'app de mon coté. Sachant que j'ai poussé une bonne partie de vos apps.

If you really want to play with this : let's have a look at the numbers. There are 22ish apps where you are flagged as maintainer. 5 of them are actually flagged as "working" on the app list, and only 3 are levels 7 ... and pretty sure that's thanks to the same people that worked on fixing Jellyfin ! (Basically the same people that maintain the currently 160+ level 7 apps of the Yunohost catalog) So maybe you can give them a bit of credit too, 'cause apps don't magically pass level 7...

Non j'en suis consience ce n'est pas le soucis certaines apps sont aussi assez complexe. Et encore une fois cette PR est une bonne chose. Je suis content que l'app passe au niveau 7. Mais avec quelques correction et c'est fait.

La méprise ce n'est pas la PR en elle meme qui me pose soucis. Mais cela fait uniquement suite au comportement de @kay0u mais pareil j'aurais bien voulu pouvoir communiquer avec quelqu'un pour en parler calmement. Et il était pas au courrant. :( J'ai réellement passé toute la journée à tenter de joindre quelqu'un sur element pour en discuter. J'ai aussi sauvegardé le travail fait et iméditement comment à réintégré ligne par ligne le travail de cette PR car il est majoriairement bon. Donc cela est aussi bon pour les utilisateurs.

liberodark commented 3 years ago

Pardon aussi je répond en FR n'ayant pas pu vous joindre sur element j'en suis désolé. Que cela ce passe ainsi sur github :(

@tituspijean sache le j'ai jamais écrit que tu avait fait de la merde mais j'ai pas un bon anglais. Donc j'ai dit : Your work is not good @tituspijean this master make app broken... Sous le coup de l'enervement. Rassure toi ta PR est très bien majoritairement comme je l'ai dit je ne suis pas fan du style mais c'est très prore :D Le seul point sur le quel je parler en disans de faire de la merde c'est pour @kay0u après il est pas au courant que j'ai tenter de vous contacter pour en discuter sachez bien que c'est seulement seulement sous le coup de l'enervement. Mais je m'en excuse @kay0u @alexAubin @tituspijean @ericgaspar J'ai rien contre votre PR seulement le forcing qu'il y a eu. N'avré que l'ont est pas pu en discuter calmement comme je le voulais en début. J'espère pouvoir conversé avec vous de l'application pour faire un travail ensemble. Je ne suis pas du tout à vous dire ne me faite pas de PR. Et encore merci pour tout ce travail @tituspijean et @ericgaspar

liberodark commented 3 years ago

Pour pouvoir en discuter calmement serait il possible d'avoir le lien matrix yunohost packager ? Ce sera mieux que ici ou si vous voulez me mp mon pseudo c'est liberodark

kay0u commented 3 years ago

Issue: Don't Touch Master or Testing without Ask.

Commit: Revert "Testing (#39)"

I'm sorry, but you can't do that. Especially without providing any reason, any logs, and just saying "this work broke the APP and you CI dont make all tests". Before this PR the app was uninstallable.

The right way to deal with a problem is :

The CI may not be perfect, but when it say "ynh_add_nginx_config is now an official helper since version '2.7.2'", there is probably a reason. When the install fails with 

Warning: yunohost.hook <lambda> - [1482.1] E: Packages need to be removed but remove is disabled.
Warning: yunohost.hook <lambda> - [1482.1] E: Package 'libass5' has no installation candidate
Warning: yunohost.hook <lambda> - [1482.1] E: Package 'libbluray1' has no installation candidate
Warning: yunohost.hook <lambda> - [1482.1] E: Package 'libva-drm1' has no installation candidate
Warning: yunohost.hook <lambda> - [1482.1] E: Package 'libva-x11-1' has no installation candidate
Warning: yunohost.hook <lambda> - [1482.1] E: Package 'libva1' has no installation candidate
Warning: yunohost.hook <lambda> - [1482.1] E: Package 'libx264-148' has no installation candidate
Warning: yunohost.hook <lambda> - [1482.1] E: Package 'libx265-95' has no installation candidate
Warning: yunohost.hook <lambda> - [1482.1] E: Package 'libwebpmux2' has no installation candidate
Warning: yunohost.hook <lambda> - [1482.1] Unable to install dependencies

It's probably not a false positive error.

Pour pouvoir en discuter calmement serait il possible d'avoir le lien matrix yunohost packager ? Ce sera mieux que ici ou si vous voulez me mp mon pseudo c'est liberodark

https://yunohost.org/#/chat_rooms

You can use https://kiwiirc.com/nextclient/irc.freenode.net/yunohost-apps if you want

liberodark commented 3 years ago

Thank you for the feedback kayou it will be corrected as soon as possible. So as not to impact the quality of the application. @kay0u Have open issue for fixe that : https://github.com/YunoHost-Apps/jellyfin_ynh/issues/46

alexAubin commented 3 years ago

So now that things cooled down a bit.

We discussed this during today's meeting and collectively decided to communicate the following :

  1. We confirmed that the way the "rework" PR was handled was legitimate, considering that a) it followed the usual recommended development workflow, b) the app was broken and the proposed work fixed the app, as demonstrated by multiple tests on the CI, c) the contributors pinged you for feedback which you did not provide, and d) this app is hosted in the Yunohost-Apps organization, which purpose is precisely to allow the community to contribute/maintain broken or unmaintained apps.

  2. The fact that you may personally not trust the official CIs doesn't really matter (you did not provide any concrete facts to support that kind of claims, and even less proposals for improvements). You've been provided with clear logs / error messages that shows why the app is not working. Conversely, the fact that the app worked "on your machine" is by no mean an objective criteria for claiming that the app was "working" before the proposed work got merged.

  3. Your reaction to the whole situation was not appropriate because :

    • you aggressively reverted multiple times the legitimate work of volunteers without any proper discussion
    • even if the work broke a functional app (which it didn't, since we're talking about the opposite), reacting aggressively is not okay, and instead you should simply discuss calmly what happened with the other members of the community
    • you claimed multiple times that volunteers broke "your" app without providing any details to support that claim
    • you tried to justify your behavior by pointing at other people

  4. At the time we were discussing this in the meeting (and this is still the case) the app is still not working because of the revert war, whereas the internal CI shows that the proposed work was fixing the app. Namely these tests :

Also you claimed that the app was broken on ARM board, then Kayou took the time to test the install on a RPi and found no issue, and you did not provide any additional explanations or concrete details on why/what it is that's not working exactly.

  1. We therefore decided the following :

    • since you seem absolutely confident about fixing the app "your way", very well : you're free to fix the app in a timely manner.
    • if following a delay of our choosing, the app is still not demonstrated to be level 7, then yes, we will "Touch Master or Testing without Ask", a.k.a apply whichever changes which are deemed necessary on the repo to make the app level 7 (considering that the work for this has already been done, and this whole story just feels like an epic waste of time anyway)
    • if you persist in reverting legit work that is aimed at fixing this app, your write access to this repository and other repositories may be suspended
    • if you again react inappropriately to similar situations in the future (here or elsewhere), your write access to this repository and other repositories may be suspended

  2. We want you to take good note of the fact that people from the community may contribute to other apps for which you are currently flagged as maintainer. The corresponding pull requests authors will usually make sure that there's enough buffer time (typically a week or two at least) before deciding to merge if no feedback is provided. That amount of time may however be way shorter if the app is broken entirely, broken in specific situations, suffers from significant security issues, or suffers from any issue that the community collectively judges to be important enough to justify a quick-merge procedure. We can also only encourage you to take a better look at the variety of tools and processes related to quality and testing (in particular the CI) to make sure that "your" apps are functional and level 5+, which is a necessary condition for your app to be effectively usable by regular users. If you believe there are improvements to be made to the testing tools, then you are free to propose them. But for the love of god, when claimining that something doesn't work, please provide the damn logs.

alexAubin commented 3 years ago

c.f. https://github.com/YunoHost-Apps/jellyfin_ynh/issues/46

Tests still failing 5 days later, no feedback.

Master and testing branches were reverted to their state before the shitstorm.