Hello,
Yesterday we (the Release Engineering team) enabled a Gerrit plugin that will automatically add reviewers to your changes based on who previously has committed changes to the file.
For more, please read the blog post at: https://phabricator.wikimedia.org/phame/post/view/139/gerrit_now_automatical...
NOTE: There are a couple requests from us open upstream to improve the plugin[0], we'll incorporate those improvements when they are released.
On behalf of the rest of the Release Engineering Team[1],
Greg
[0] https://phabricator.wikimedia.org/T101131#4890023 [1] As well as Paladox, a Wikimedia volunteer with strong ties to upstream Gerrit.
On Thu, Jan 17, 2019 at 10:52 PM Greg Grossmeier greg@wikimedia.org wrote:
Hello,
Yesterday we (the Release Engineering team) enabled a Gerrit plugin that will automatically add reviewers to your changes based on who previously has committed changes to the file.
While I commend the intention, this means I will get pinged for virtually any change in a couple of very busy repositories.
The amount of noise will prevent me from being able to notice anyone's review request. I think it's going to be the same for other developers - I don't want to imagine what the inbox of a long-time mediawiki-core contributor must look like!
What I fear is that the flood of reviews will make everyone just dull to notifications, obtaining the exact opposite effect that was intended. I say this because I auto added myself to all reviews in operations/puppet[1] in the past, which resulted in me ignoring all code review requests.
I think a good compromise would be to modify the plugin so that it adds reviewers automatically, only if you're a new contributor (so you have - say - less than N patches submitted).
While this gets improved, is there a way to opt-out from the feature individually or as a project?
Thanks, Giuseppe
[1] we already have a way to "monitor" all changes to a repository, to a directory within a repository, or even to individual files, which I was using extensively. Should we remove that?
Although this change doesn't personally affect me because I'm not active on Gerrit, my reading of the situation is similar to Giuseppe's. Sending people potentially large numbers of automatic and unsolicited notifications is not something that I would generally support, and I would likely believe to be a misuse of communications tools and an inconsiderate use of others' time, no matter how well-intentioned.
My impression of this change to communications is that it should have been broadly discussed, and consensus requested through an RfC, before implementation. Was there an RfC? Were there previous wide notifications regarding this proposed change to communications?
On Thu, Jan 17, 2019 at 10:58 PM Giuseppe Lavagetto < glavagetto@wikimedia.org> wrote:
While this gets improved, is there a way to opt-out from the feature individually or as a project?
Individually, no. On the project level, just set the max number of reviewers added by the plugin to zero. (It seems that you need to use the old Gerrit interface to see settings that come from plugins...)
I’ve been working on a opt out (blacklist) in the plugin, see https://gerrit-review.googlesource.com/c/plugins/reviewers-by-blame/+/210812 On Friday, 18 January 2019, 09:39:55 GMT, Gergo Tisza gtisza@wikimedia.org wrote:
On Thu, Jan 17, 2019 at 10:58 PM Giuseppe Lavagetto < glavagetto@wikimedia.org> wrote:
While this gets improved, is there a way to opt-out from the feature individually or as a project?
Individually, no. On the project level, just set the max number of reviewers added by the plugin to zero. (It seems that you need to use the old Gerrit interface to see settings that come from plugins...) _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Hello,
I agree with what Giuseppe Lavagetto and Jaime Crespo said.
In my case, I am now getting review requests from several repos I contributed some time in the past, but for which I'm not a qualified reviewer. The plugin is also adding bots to review changes such as in https://gerrit.wikimedia.org/r/485174.
While I think it is certainly a good idea to help people find reviewers for their patches, I feel this plugin as it is now is going to achieve the contrary (mail blindness due to too many emails).
I suggest we disable the plugin until at least Paladox's blacklist could be implemented and so we are given the choice to opt-out from it.
Thank you, M.
Hi Thiemo, hi all,
given my strong support for T78768 and the connection to this change, I would like to express my regrets to the engineers blocked by the change in Gerrit. As typical for strike actions, it disrupts the usual business. However, I see the positive effect that people are made aware of the fact that the code review process needs to be improved.
While I am happy with the code review duration of the patches I submit today, I had problems to get the (obviously bad) code I wrote in the beginning reviewed. I guess one of the main reasons for the improvement is that I know the reviewers in person, today.
However, visiting Hackathons and annoying WMF employees in their offices is not something that scales. Therefore, the review process needs to be changed. After years of discussion and only little changes to the process, this change brings this important but not urgent topic to the agenda, which I appreciate.
To my experience Thiemo ist one of the most predictable (=good) code reviewers. I am more than happy that he shared his process in his email. I think his code reviewing procedure is exemplary and should act as a general template. Thank you Thiemo.
One last hopefully constructive point. To my experience, the most annoying experience in waiting for CR is when multiple reviewers are requested and no feedback is provided. My wish would be that the state of the patch is visualized in the (Gerrit) UI. So that the submitter of the patch knows the state of the change and can estimate how long it might take until the patch proceeds to the next stage.
Happy coding physikerwelt
On Fri, Jan 18, 2019 at 1:25 PM MA strigiwm@gmail.com wrote:
Hello,
I agree with what Giuseppe Lavagetto and Jaime Crespo said.
In my case, I am now getting review requests from several repos I contributed some time in the past, but for which I'm not a qualified reviewer. The plugin is also adding bots to review changes such as in https://gerrit.wikimedia.org/r/485174.
While I think it is certainly a good idea to help people find reviewers for their patches, I feel this plugin as it is now is going to achieve the contrary (mail blindness due to too many emails).
I suggest we disable the plugin until at least Paladox's blacklist could be implemented and so we are given the choice to opt-out from it.
Thank you, M.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Fri, 18 Jan 2019 at 07:58, Giuseppe Lavagetto glavagetto@wikimedia.org wrote:
The amount of noise will prevent me from being able to notice anyone's review request. I think it's going to be the same for other developers - I don't want to imagine what the inbox of a long-time mediawiki-core contributor must look like!
What I fear is that the flood of reviews will make everyone just dull to notifications, obtaining the exact opposite effect that was intended. I say this because I auto added myself to all reviews in operations/puppet[1] in the past, which resulted in me ignoring all code review requests.
Reading this thread -- and Guiseppes comment above in particular, made me think a bit on how we do code review, and how automatically adding reviewers may be counterproductive. This includes the Gerrit Reviewer Bot: even though it is opt-in, it may still overwhelm the inbox of whoever opted in, and if that person does not unsubscribe (but rather creates a filter to move all the requests into a 'I don't have time to look at this now, but I will review these later' mailbox - I have done this myself at some point, which meant that in practice, I didn't review anything for that project anymore), we end up in a situation where code is not actually reviewed in a more timely manner.
One thing I wondered about is how big the problem of patches without relevant reviewers is. This is not entirely trivial to query, but the best I could come up with was looking for patches without any reviewer at all: https://gerrit.wikimedia.org/r/q/status:open+-reviewerin:%2522Registered+Use... . This is only a very small number of patches -- while the number of patches that have V+1 CR=0 Age > 1 month is much larger: https://gerrit.wikimedia.org/r/q/status:open+label:Verified%253E%253D0+label...
This suggests to me that the problem is not a lack of reviewers added, but a lack of reviewing :-). This might be due to the wrong reviewers being added (which an improved version of the auto-reviewer plugin could solve), but it might also just be that the reviewers don't have enough time to actually perform the reviews. That includes myself -- I find it very difficult to get started doing reviews on code I haven't looked at a while. After all, it's much more fun to write code than to review it ;-)
How to solve that? I don't know -- I think initiatives such as Andre's 'Patchsets by new Gerrit contributors' emails help (as they focus on a small number of changes). The same is true for a form of gamification (such as the statistics in the previous Thank You day threads).
In general, I believe that trying something new (which includes trying the Gerrit plugin!) is going to be beneficial, as otherwise we never discover which directions improve things (and which ones don't). After all, the Reviewer Bot is already 8 years old, and it's unlikely that the thing I hacked together back then is still the best solution now :-)
Merlijn
[…] automatically add reviewers to your changes based on who previously has committed changes to the file.
I'm already overwhelmed with review requests. I'm also one of the latest contributors in sooo many files that I'm worried the plugin will add me to dozens per day from now on. This surprising addition makes me worry very much about my sanity and the usability of my inbox and Gerrit dashboards. Please, please, tune it down heavily.
Until now, I had a process to find reviewers:
1. For planned changes, it's already obvious who needs to do the review: my team members. Often they don't even need to be added as reviewers, or don't want to, but use the "review" column on our Phabricator board. Automatically adding random *other* people is not only useless in this situation, it's counterproductive and frustrating for everybody involved. Other people should not waste their time with patches like this. When they do, it's frustrating for the one who was supposed to do the review, as well as for the "auto-reviewer". His review is not helpful and ultimately not appreciated.
2. For code cleanups in core and codebases my team does not own I open the list of merged patches on Gerrit to see if I can tell the names of one or two main contributors. Often, the list will contain nothing but the Translatewiki bot and a few people doing cross-codebase maintainance work. These highly active people should *not* be the first pick as potential reviewers for multiple reasons. Most importantly, just because someone is very productive it's not ok to expect him to accept even more workload. This is super-counterproductive and ultimately leads to people burning out. Secondly, just because someone updated, let's say, a call to a deprecated core function does not mean he is familiar with a codebase.
3. I look at the files my patch happens to touch in my PHPStorm IDE, enable the blame column that shows who touched a line last, and see if I can find the one who introduced the code I'm touching.
All steps in this process involve *reading* the commit messages and considering what people did, when and why. This can't be automated.
I do not entirely oppose the idea of adding reviewers automatically, as long as I (as a reviewer) have a chance to easily tell the difference between being added manually vs. automatically. For my sanity, I will most probably setup an email filter that auto-deletes all automated requests, and only look at these auto-reviews once a week via my Gerrit dashboard.
Based on all these arguments this is what I, personally, find acceptable: * Make sure no emails are sent for being automatically added, or make sure it's possible to turn them off (without also killing the wanted emails about manually being added). * Make sure tool accounts like the library-upgrader or Translatewiki bot are never added as reviewers. * Never automatically add reviewers if there are other reviewers already. Most importantly, if people have been added via the reviewer bot already, that's more than enough. * Only add 2 reviewers. 2 people will more likely feel like a team. 3 people are much more likely to all think "the other 2 will do it" and all ignore it. * Don't just pick the "most recent" contributor. That's most certainly not the person you want (probably one who fixed a typo, or updated a library). Implement an algorithm that can either understand who touched which places in the code, and assigns them to patches that happen to touch the same place again. Or go for an algorithm that (for example) analyzes the last year and picks the 2 people who did the most changes to a file in that time span.
If more than one of these criteria is not met or not possible, the only solution I see is to make the plugin opt-in per codebase, not opt-out as of now (because you can't expect me to opt-out from literally a thousand codebases).
Thank you for keeping my inbox sane.
Best Thiemo
One member of my team sadly left. Now he is pinged every time I upload a change, passively aggressive reminding him he used to work on this.
Don't get me wrong, I think this is great to get attention for new contributors, to make sure it is moving forward, but I would suggest to be able to opt-out of this.
On Fri, Jan 18, 2019 at 9:47 AM Thiemo Kreuz thiemo.kreuz@wikimedia.de wrote:
[…] automatically add reviewers to your changes based on who previously
has committed changes to the file.
I'm already overwhelmed with review requests. I'm also one of the latest contributors in sooo many files that I'm worried the plugin will add me to dozens per day from now on. This surprising addition makes me worry very much about my sanity and the usability of my inbox and Gerrit dashboards. Please, please, tune it down heavily.
Until now, I had a process to find reviewers:
- For planned changes, it's already obvious who needs to do the
review: my team members. Often they don't even need to be added as reviewers, or don't want to, but use the "review" column on our Phabricator board. Automatically adding random *other* people is not only useless in this situation, it's counterproductive and frustrating for everybody involved. Other people should not waste their time with patches like this. When they do, it's frustrating for the one who was supposed to do the review, as well as for the "auto-reviewer". His review is not helpful and ultimately not appreciated.
- For code cleanups in core and codebases my team does not own I open
the list of merged patches on Gerrit to see if I can tell the names of one or two main contributors. Often, the list will contain nothing but the Translatewiki bot and a few people doing cross-codebase maintainance work. These highly active people should *not* be the first pick as potential reviewers for multiple reasons. Most importantly, just because someone is very productive it's not ok to expect him to accept even more workload. This is super-counterproductive and ultimately leads to people burning out. Secondly, just because someone updated, let's say, a call to a deprecated core function does not mean he is familiar with a codebase.
- I look at the files my patch happens to touch in my PHPStorm IDE,
enable the blame column that shows who touched a line last, and see if I can find the one who introduced the code I'm touching.
All steps in this process involve *reading* the commit messages and considering what people did, when and why. This can't be automated.
I do not entirely oppose the idea of adding reviewers automatically, as long as I (as a reviewer) have a chance to easily tell the difference between being added manually vs. automatically. For my sanity, I will most probably setup an email filter that auto-deletes all automated requests, and only look at these auto-reviews once a week via my Gerrit dashboard.
Based on all these arguments this is what I, personally, find acceptable:
- Make sure no emails are sent for being automatically added, or make
sure it's possible to turn them off (without also killing the wanted emails about manually being added).
- Make sure tool accounts like the library-upgrader or Translatewiki
bot are never added as reviewers.
- Never automatically add reviewers if there are other reviewers
already. Most importantly, if people have been added via the reviewer bot already, that's more than enough.
- Only add 2 reviewers. 2 people will more likely feel like a team. 3
people are much more likely to all think "the other 2 will do it" and all ignore it.
- Don't just pick the "most recent" contributor. That's most certainly
not the person you want (probably one who fixed a typo, or updated a library). Implement an algorithm that can either understand who touched which places in the code, and assigns them to patches that happen to touch the same place again. Or go for an algorithm that (for example) analyzes the last year and picks the 2 people who did the most changes to a file in that time span.
If more than one of these criteria is not met or not possible, the only solution I see is to make the plugin opt-in per codebase, not opt-out as of now (because you can't expect me to opt-out from literally a thousand codebases).
Thank you for keeping my inbox sane.
Best Thiemo
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512
Hi,
On 1/18/19 1:11 AM, Jaime Crespo wrote:
One member of my team sadly left. Now he is pinged every time I upload a change, passively aggressive reminding him he used to work on this.
Don't get me wrong, I think this is great to get attention for new contributors, to make sure it is moving forward, but I would suggest to be able to opt-out of this.
While exacerbated by the plugin, I think this is a problem in general, when someone (typically staff) leaves the movement, and others are unaware and continue to request reviews from them (and likely a bouncing mailbox :/).
In these cases it would be nice if those people could set their Gerrit status[1] to something indicating that they won't be reviewing code anymore so others don't add them as reviewers.
[1] Simetrical has theirs set to "Inactive until April" for example: https://screenshots.firefox.com/JWmxdIdXtGaQ8EHW/gerrit.wikimedia.org
- -- Legoktm
So it turns out this addition is indeed the reason I get constantly spammed with the same fake review request over and over again, no matter how often I try to remove myself from a patch.
https://gerrit.wikimedia.org/r/484681
Not only that. The notification mails make it look like Matthias Mullie went rogue. He is a wonderful person and definitely doesn't deserve that such cyberbullying is misattributed to him.
With all the respect, but at this point this is just insane. Please make this stop!
Thiemo
Hi all,
Gerrit no longer automatically adds reviewers[0]. Unfortunately, this plugin appears (given the replies on this thread) to be missing key features needed to be useful for us at this time. Apologies to those folks whose inboxes were destroyed.
I would like to re-enable this plugin at some point, provided the features identified in this thread are added (perhaps also an "X-Gerrit-reviewers-by-blame: 1" email header, or subject line to make filtering these messages easier).
In the interim, project-owners are able to opt-in to using the reviewers-by-blame plugin on a per-project basis on their project admin page in Gerrit.
Also, the Git Reviewer Bot[1] provides folks an opt-in method of volunteering to review a subset of files in a particular repo.
Getting code review as a new contributor is hard. Thanks for bearing with us.
-- Tyler
[0]. https://gerrit.wikimedia.org/r/#/c/All-Projects/+/485184/ [1]. https://www.mediawiki.org/wiki/Git/Reviewers
On 19-01-17 13:51:58, Greg Grossmeier wrote:
Hello,
Yesterday we (the Release Engineering team) enabled a Gerrit plugin that will automatically add reviewers to your changes based on who previously has committed changes to the file.
For more, please read the blog post at: https://phabricator.wikimedia.org/phame/post/view/139/gerrit_now_automatical...
NOTE: There are a couple requests from us open upstream to improve the plugin[0], we'll incorporate those improvements when they are released.
On behalf of the rest of the Release Engineering Team[1],
Greg
[0] https://phabricator.wikimedia.org/T101131#4890023 [1] As well as Paladox, a Wikimedia volunteer with strong ties to upstream Gerrit.
-- | Greg Grossmeier GPG: B2FA 27B1 F7EB D327 6B8E | | Release Team Manager A18D 1138 8E47 FAC8 1C7D |
Engineering mailing list Engineering@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/engineering
On Fri, Jan 18, 2019 at 3:12 PM Tyler Cipriani tcipriani@wikimedia.org wrote:
I would like to re-enable this plugin at some point, provided the features identified in this thread are added (perhaps also an "X-Gerrit-reviewers-by-blame: 1" email header, or subject line to make filtering these messages easier).
Let me suggest one workflow that may work with this feature- Adding a button, for example, with "Suggest reviewers" which you can press to get this effect. Or doing it automatically if your history only has less than X CRs sent. What do you think?
Am 18.01.19 um 15:25 schrieb Jaime Crespo:
Let me suggest one workflow that may work with this feature- Adding a button, for example, with "Suggest reviewers" which you can press to get this effect.
Yes.
It could also be doneautomatically when a patch did not get any attention for a week or so.
In any case, there should be a way to opt out. Ideally, per project.
The patch i linked in my other email does what you are all suggesting :) (opt out per project). If done through All-Projects it will opt you out of all projects. On Friday, 18 January 2019, 14:44:50 GMT, Daniel Kinzler dkinzler@wikimedia.org wrote:
Am 18.01.19 um 15:25 schrieb Jaime Crespo:
Let me suggest one workflow that may work with this feature- Adding a button, for example, with "Suggest reviewers" which you can press to get this effect.
Yes.
It could also be doneautomatically when a patch did not get any attention for a week or so.
In any case, there should be a way to opt out. Ideally, per project.
I've forwarded jynus feedback upstream at https://bugs.chromium.org/p/gerrit/issues/detail?id=10337
On Friday, 18 January 2019, 15:01:09 GMT, Paladox via Wikitech-l wikitech-l@lists.wikimedia.org wrote:
The patch i linked in my other email does what you are all suggesting :) (opt out per project). If done through All-Projects it will opt you out of all projects. On Friday, 18 January 2019, 14:44:50 GMT, Daniel Kinzler dkinzler@wikimedia.org wrote:
Am 18.01.19 um 15:25 schrieb Jaime Crespo:
Let me suggest one workflow that may work with this feature- Adding a button, for example, with "Suggest reviewers" which you can press to get this effect.
Yes.
It could also be doneautomatically when a patch did not get any attention for a week or so.
In any case, there should be a way to opt out. Ideally, per project.
In the meantime, I would encourage those who have not looked at the Git Reviewer Bot page in a while, to do so and to add any updates.
Ariel
On Fri, Jan 18, 2019 at 4:12 PM Tyler Cipriani tcipriani@wikimedia.org wrote:
Hi all,
Gerrit no longer automatically adds reviewers[0]. Unfortunately, this plugin appears (given the replies on this thread) to be missing key features needed to be useful for us at this time. Apologies to those folks whose inboxes were destroyed.
I would like to re-enable this plugin at some point, provided the features identified in this thread are added (perhaps also an "X-Gerrit-reviewers-by-blame: 1" email header, or subject line to make filtering these messages easier).
In the interim, project-owners are able to opt-in to using the reviewers-by-blame plugin on a per-project basis on their project admin page in Gerrit.
Also, the Git Reviewer Bot[1] provides folks an opt-in method of volunteering to review a subset of files in a particular repo.
Getting code review as a new contributor is hard. Thanks for bearing with us.
-- Tyler
[0]. https://gerrit.wikimedia.org/r/#/c/All-Projects/+/485184/ [1]. https://www.mediawiki.org/wiki/Git/Reviewers
On 19-01-17 13:51:58, Greg Grossmeier wrote:
Hello,
Yesterday we (the Release Engineering team) enabled a Gerrit plugin that will automatically add reviewers to your changes based on who previously has committed changes to the file.
For more, please read the blog post at:
https://phabricator.wikimedia.org/phame/post/view/139/gerrit_now_automatical...
NOTE: There are a couple requests from us open upstream to improve the plugin[0], we'll incorporate those improvements when they are released.
On behalf of the rest of the Release Engineering Team[1],
Greg
[0] https://phabricator.wikimedia.org/T101131#4890023 [1] As well as Paladox, a Wikimedia volunteer with strong ties to upstream Gerrit.
-- | Greg Grossmeier GPG: B2FA 27B1 F7EB D327 6B8E | | Release Team Manager A18D 1138 8E47 FAC8 1C7D |
Engineering mailing list Engineering@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/engineering
Engineering mailing list Engineering@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/engineering
Gerrit no longer automatically adds reviewers […]
Thank you very much for reacting so fast.
I'm not sure if the list of issues in this commit message is meant to be complete. But I noticed it misses the bug I found the most annoying: The plugin doesn't get activated once per patch, but for every patch set, and ignores every human intervention that might have been done to this point, most notably when reviewers have manually been removed. There are many ways to fix this, but the current behavior is unacceptable.
It is due to this bug that I wish we would revoke this bad plugin entirely. I don't think anybody should be able to run it as this would open the exact same issues again, just on a smaller scale, but still hurting the same people.
Best Thiemo
I'm glad that this problematic change to communications was reverted.
I would like to suggest that this is the type of change that, when being planned, should get a design review from a third party before coding starts, should go through at least one RFC before coding starts, and be widely communicated before coding starts and again a week or two before deployment. Involving TechCom might also be appropriate. It appears that none of those happened here. In terms of process this situation looks to me like it's inexcusable.
In the English Wikipedia community, doing something like this would have a reasonable likelihood of costing an administrator their tools, and I hope that a similar degree of accountability is enforced in the engineering community. In particular, I expect engineering supervisors to follow established technical processes for changes that impact others' workflows, and if they decide to skip those processes without a compelling reason (such as a site stability problem) then I hope that they will be held accountable. Again, from my perspective, the failure to follow process here is inexcusable.
"should get a design review from a third party before coding starts" what do you mean by that? Also may i remind you that gerrit is not a encyclopedia, it's a code review system. So what ever process is on the encyclopedia is not the same here. Also please can we leave the threats out. It wasn't deployed to annoy users, it was deployed to help. Obviously we didn't know it was going to turn out like this, hence why it was made opt in for all projects today.
On Friday, 18 January 2019, 22:13:38 GMT, Pine W wiki.pine@gmail.com wrote:
I'm glad that this problematic change to communications was reverted.
I would like to suggest that this is the type of change that, when being planned, should get a design review from a third party before coding starts, should go through at least one RFC before coding starts, and be widely communicated before coding starts and again a week or two before deployment. Involving TechCom might also be appropriate. It appears that none of those happened here. In terms of process this situation looks to me like it's inexcusable.
In the English Wikipedia community, doing something like this would have a reasonable likelihood of costing an administrator their tools, and I hope that a similar degree of accountability is enforced in the engineering community. In particular, I expect engineering supervisors to follow established technical processes for changes that impact others' workflows, and if they decide to skip those processes without a compelling reason (such as a site stability problem) then I hope that they will be held accountable. Again, from my perspective, the failure to follow process here is inexcusable.
Pine ( https://meta.wikimedia.org/wiki/User:Pine ) _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Can I please ask again to *ban* this bad plugin entirely from our systems? Having it sit there for anybody to enable again is a ticking time bomb. It will start sending out the same misattributed, uncontrollable, aggressive fake request spam again. I don't want anybody to experience something like this again.
Look for a plugin that makes suggestions, please.
Best Thiemo
Hello,
El lun., 21 ene. 2019 a las 10:14, Thiemo Kreuz (thiemo.kreuz@wikimedia.de) escribió:
Can I please ask again to *ban* this bad plugin entirely from our systems? Having it sit there for anybody to enable again is a ticking time bomb. It will start sending out the same misattributed, uncontrollable, aggressive fake request spam again. I don't want anybody to experience something like this again.
Look for a plugin that makes suggestions, please.
Best Thiemo
I think Thiemo makes a good point again. As things stand now support disabling/removing the plugin entirely from our gerrit install.
Thank you, M.
I’m currently working on addressing all the feedback as fast as I can. I honestly think this extension is great especially for new users, who do not know they need reviewers or who would review there change. Granted this extension has some problems hence why feature requests were filed against the extension. On Monday, 21 January 2019, 15:41:53 GMT, MA strigiwm@gmail.com wrote:
Hello,
El lun., 21 ene. 2019 a las 10:14, Thiemo Kreuz (thiemo.kreuz@wikimedia.de) escribió:
Can I please ask again to *ban* this bad plugin entirely from our systems? Having it sit there for anybody to enable again is a ticking time bomb. It will start sending out the same misattributed, uncontrollable, aggressive fake request spam again. I don't want anybody to experience something like this again.
Look for a plugin that makes suggestions, please.
Best Thiemo
I think Thiemo makes a good point again. As things stand now support disabling/removing the plugin entirely from our gerrit install.
Thank you, M.
_______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
FYI i have a working prototype working ("Suggest Reviewer") button. On Monday, 21 January 2019, 16:32:35 GMT, Paladox via Wikitech-l wikitech-l@lists.wikimedia.org wrote:
I’m currently working on addressing all the feedback as fast as I can. I honestly think this extension is great especially for new users, who do not know they need reviewers or who would review there change. Granted this extension has some problems hence why feature requests were filed against the extension. On Monday, 21 January 2019, 15:41:53 GMT, MA strigiwm@gmail.com wrote:
Hello,
El lun., 21 ene. 2019 a las 10:14, Thiemo Kreuz (thiemo.kreuz@wikimedia.de) escribió:
Can I please ask again to *ban* this bad plugin entirely from our systems? Having it sit there for anybody to enable again is a ticking time bomb. It will start sending out the same misattributed, uncontrollable, aggressive fake request spam again. I don't want anybody to experience something like this again.
Look for a plugin that makes suggestions, please.
Best Thiemo
I think Thiemo makes a good point again. As things stand now support disabling/removing the plugin entirely from our gerrit install.
Thank you, M.
_______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
[…] i have a working prototype working ("Suggest Reviewer") button.
Ok, but that's another extension then. Can we kill the bad one, please?
Best Thiemo
Nope, im adding that functionality to reviewers-by-blame. Unless y'all want a plugin where you can define reviewers each repo and a user can press "Suggest Reviewers", but it would probably be best to use Reviewers-By-Blame once all the feedback has been addressed. On Monday, 21 January 2019, 19:37:24 GMT, Thiemo Kreuz thiemo.kreuz@wikimedia.de wrote:
[…] i have a working prototype working ("Suggest Reviewer") button.
Ok, but that's another extension then. Can we kill the bad one, please?
Best Thiemo
_______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Change is now available at: https://gerrit-review.googlesource.com/c/plugins/reviewers-by-blame/+/210812... On Monday, 21 January 2019, 19:46:41 GMT, Paladox via Wikitech-l wikitech-l@lists.wikimedia.org wrote:
Nope, im adding that functionality to reviewers-by-blame. Unless y'all want a plugin where you can define reviewers each repo and a user can press "Suggest Reviewers", but it would probably be best to use Reviewers-By-Blame once all the feedback has been addressed. On Monday, 21 January 2019, 19:37:24 GMT, Thiemo Kreuz thiemo.kreuz@wikimedia.de wrote:
[…] i have a working prototype working ("Suggest Reviewer") button.
Ok, but that's another extension then. Can we kill the bad one, please?
Best Thiemo
_______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
[…] im adding that functionality to reviewers-by-blame.
I'm afraid I don't understand. Does this mean all the issues that make the plugin send passive-aggressive, misattributed spam will still be in place, possibly hitting peoples inboxes again any time somebody decides it would be a good idea to turn this feature set on again? My impression was more that all the ideas that have been shared in this email thread describe an *entirely* different approach, following entirely different ideas, with an entirely different UI, and entirely different setup. What's the point of stuffing this into the same, fundamentally broken codebase?
Sysops, please, for our sanity, do not let anybody enable this toxic plugin again.
Best Thiemo
Fundamentally broken sounds like a bit of a stretch.
In fact, it was probably working quite well for our less-trafficked repositories. But the issues identified must be fixed and decent file exclusion rules written before it goes back on for the active ones.
-Chad
On Jan 22, 2019 12:11 AM, "Thiemo Kreuz" thiemo.kreuz@wikimedia.de wrote:
[…] im adding that functionality to reviewers-by-blame.
I'm afraid I don't understand. Does this mean all the issues that make the plugin send passive-aggressive, misattributed spam will still be in place, possibly hitting peoples inboxes again any time somebody decides it would be a good idea to turn this feature set on again? My impression was more that all the ideas that have been shared in this email thread describe an *entirely* different approach, following entirely different ideas, with an entirely different UI, and entirely different setup. What's the point of stuffing this into the same, fundamentally broken codebase?
Sysops, please, for our sanity, do not let anybody enable this toxic plugin again.
Best Thiemo
_______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Fundamentally broken sounds like a bit of a stretch.
A process that annoys people based on nothing but the fact that they happened to be the last one touching a file *is* fundamentally broken. This is not how anyone should look for reviewers, neither manually nor automatically.
Here is a thought experiment: We could send review requests to the *least* active users that are still around, but *never* touched a file. The positive effects of such an approach include: * More people get familiar with the code. * Knowledge gets spread more evenly. * Bottlenecks and bus factors get reduced. * These people probably have more time. * Review requests are spread more evenly. * Workload is spread more evenly.
Still sounds like a bad idea? Sure, because it is. Now tell me: How is it more clever to do the *opposite* and dump review requests on people that have to much workload already?
At this point I don't care any more if we are talking about a fully automated process or a suggest button. Both are targeting the wrong people.
it was probably working quite well for our less-trafficked repositories.
What is the difference between being the last one fixing a typo in a low-traffic vs. high-traffic repository? In both cases it's the wrong person.
Thiemo
On Tue, Jan 22, 2019 at 4:05 AM Thiemo Kreuz thiemo.kreuz@wikimedia.de wrote:
Fundamentally broken sounds like a bit of a stretch.
A process that annoys people based on nothing but the fact that they happened to be the last one touching a file *is* fundamentally broken. This is not how anyone should look for reviewers, neither manually nor automatically.
It isn't? I figured "people who have worked on this code before" is an excellent metric by which to find people to review your code. It's certainly who I'd look to help me if I didn't just _know_ who to ask.
Here is a thought experiment: We could send review requests to the
*least* active users that are still around, but *never* touched a file. The positive effects of such an approach include:
- More people get familiar with the code.
- Knowledge gets spread more evenly.
- Bottlenecks and bus factors get reduced.
- These people probably have more time.
- Review requests are spread more evenly.
- Workload is spread more evenly.
Still sounds like a bad idea? Sure, because it is.
Dumb straw man.
Now tell me: How is it more clever to do the *opposite* and dump review requests on people that have to much workload already?
Who said these people have too much workload? Just because they've made a commit before? The blame attribution has zero insight into how busy someone is.
At this point I don't care any more if we are talking about a fully automated process or a suggest button. Both are targeting the wrong people.
it was probably working quite well for our less-trafficked repositories.
What is the difference between being the last one fixing a typo in a low-traffic vs. high-traffic repository? In both cases it's the wrong person.
If it's a low-traffic repository there's likely to be fewer overall contributors. Fewer contributors increases the likelihood of people being qualified to review--whereas a high-traffic repo is more likely to have drive-by contributor less capable.
And if it's just one-line typofixing it'd be ideal to exclude those from the blame list--but we can't possibly know what was a one-line typofix and what was a one line that saved us 50% of execution time on all pages. At least not programmatically.
Honestly, if you think "people who've edited the code in the past" are a poor person to ask for review then you do not understand how code review works.
-Chad
Am Di., 22. Jan. 2019 um 13:25 Uhr schrieb Chad innocentkiller@gmail.com:
Dumb straw man.
can we avoid this tone? thanks
Who said these people have too much workload?
Um, Thiemo himself has said this? Are you going to tell him that he’s wrong about his own workload?
The blame attribution has zero insight into how
busy someone is.
Correct, which is why it’s a bad idea to let it run loose and add people who are already busy enough as reviewers.
If it's a low-traffic repository there's likely to be fewer overall contributors. Fewer contributors increases the likelihood of people being qualified to review--whereas a high-traffic repo is more likely to have drive-by contributor less capable.
Well, many drive-by contributions are tree-wide: they are applied to a large set of repositories collectively, e. g. all Wikimedia-deployed extensions or even all repositories. If a repository has generally low traffic, then these tree-wide drive-by contributions will make up a larger ratio of its commits than in repositories with more repository-specific commits.
I’m not sure if I phrased this well, but if repository A has 1000 specific commits and 10 drive-by commits, and repository B has 20 specific commits and the same 10 drive-by commits, then the drive-by commits will be ⅓ of all commits in repository B but less than .1% in repository A.
And if it's just one-line typofixing it'd be ideal to exclude those from the blame list--but we can't possibly know what was a one-line typofix and what was a one line that saved us 50% of execution time on all pages. At least not programmatically.
True to some extent, but then we should err on the side of not adding the reviewer, no? Otherwise we run the risk of overwhelming them with changes they’re not even qualified to review, even if they had the time.
Honestly, if you think "people who've edited the code in the past" are a poor person to ask for review then you do not understand how code review works.
Suggesting that Thiemo doesn’t understand how code review works is… bold, in my opinion, let’s put it that way. May I point out that he’s one of the top +2ers across all MediaWiki extensions https://lists.wikimedia.org/pipermail/wikitech-l/2019-January/091340.html?
Cheers, Lucas
I suggest discussing the implementation details on phabricator. Moreover, I second Lucas point on the tone. physikerwelt https://www.mediawiki.org/wiki/Code_of_Conduct
physikerwelt https://www.mediawiki.org/wiki/Code_of_Conduct
On Tue, Jan 22, 2019 at 1:43 PM Lucas Werkmeister lucas.werkmeister@wikimedia.de wrote:
Am Di., 22. Jan. 2019 um 13:25 Uhr schrieb Chad innocentkiller@gmail.com:
Dumb straw man.
can we avoid this tone? thanks
Who said these people have too much workload?
Um, Thiemo himself has said this? Are you going to tell him that he’s wrong about his own workload?
The blame attribution has zero insight into how
busy someone is.
Correct, which is why it’s a bad idea to let it run loose and add people who are already busy enough as reviewers.
If it's a low-traffic repository there's likely to be fewer overall contributors. Fewer contributors increases the likelihood of people being qualified to review--whereas a high-traffic repo is more likely to have drive-by contributor less capable.
Well, many drive-by contributions are tree-wide: they are applied to a large set of repositories collectively, e. g. all Wikimedia-deployed extensions or even all repositories. If a repository has generally low traffic, then these tree-wide drive-by contributions will make up a larger ratio of its commits than in repositories with more repository-specific commits.
I’m not sure if I phrased this well, but if repository A has 1000 specific commits and 10 drive-by commits, and repository B has 20 specific commits and the same 10 drive-by commits, then the drive-by commits will be ⅓ of all commits in repository B but less than .1% in repository A.
And if it's just one-line typofixing it'd be ideal to exclude those from the blame list--but we can't possibly know what was a one-line typofix and what was a one line that saved us 50% of execution time on all pages. At least not programmatically.
True to some extent, but then we should err on the side of not adding the reviewer, no? Otherwise we run the risk of overwhelming them with changes they’re not even qualified to review, even if they had the time.
Honestly, if you think "people who've edited the code in the past" are a poor person to ask for review then you do not understand how code review works.
Suggesting that Thiemo doesn’t understand how code review works is… bold, in my opinion, let’s put it that way. May I point out that he’s one of the top +2ers across all MediaWiki extensions https://lists.wikimedia.org/pipermail/wikitech-l/2019-January/091340.html?
Cheers, Lucas
-- Lucas Werkmeister Full Stack Developer
Wikimedia Deutschland e. V. | Tempelhofer Ufer 23-24 | 10963 Berlin Phone: +49 (0)30 219 158 26-0 https://wikimedia.de
Imagine a world in which every single human being can freely share in the sum of all knowledge. Help us to achieve our vision! https://spenden.wikimedia.de
Wikimedia Deutschland - Gesellschaft zur Förderung Freien Wissens e. V. Eingetragen im Vereinsregister des Amtsgerichts Berlin-Charlottenburg unter der Nummer 23855 B. Als gemeinnützig anerkannt durch das Finanzamt für Körperschaften I Berlin, Steuernummer 27/029/42207. _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
[…] "people who have worked on this code before" is an excellent metric by which to find people to review your code.
Sure. But this is neither what I wrote, nor what the plugin does, nor what can be done programmatically in the first place, as you conveniently pointed out yourself:
[…] we can't possibly know what was a one-line typofix and what was a one line that saved us 50% of execution time on all pages. At least not programmatically.
Kind regards Thiemo
What your saying is making me think I’m wasting my time on improving this extension. Also other users that have spoken to me have thought this extension is great but could do with improvements which I am doing. We need to think of new users and how to improve there experence. The task was opened for a long while yet no one commented on it. I agree with legoktm feedback. “A process that annoys people based on nothing but the fact that theyhappened to be the last one touching a file *is* fundamentally broken.” yes hence why I’ve been making improvements by adding a button which is better then nothing right? As chad mentions it has no idea what is a typo fix compared to other things as it’s not A.I.
On Tuesday, 22 January 2019, 12:05:24 GMT, Thiemo Kreuz thiemo.kreuz@wikimedia.de wrote:
Fundamentally broken sounds like a bit of a stretch.
A process that annoys people based on nothing but the fact that they happened to be the last one touching a file *is* fundamentally broken. This is not how anyone should look for reviewers, neither manually nor automatically.
Here is a thought experiment: We could send review requests to the *least* active users that are still around, but *never* touched a file. The positive effects of such an approach include: * More people get familiar with the code. * Knowledge gets spread more evenly. * Bottlenecks and bus factors get reduced. * These people probably have more time. * Review requests are spread more evenly. * Workload is spread more evenly.
Still sounds like a bad idea? Sure, because it is. Now tell me: How is it more clever to do the *opposite* and dump review requests on people that have to much workload already?
At this point I don't care any more if we are talking about a fully automated process or a suggest button. Both are targeting the wrong people.
it was probably working quite well for our less-trafficked repositories.
What is the difference between being the last one fixing a typo in a low-traffic vs. high-traffic repository? In both cases it's the wrong person.
Thiemo
_______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Tue, Jan 22, 2019 at 6:12 PM Paladox via Wikitech-l < wikitech-l@lists.wikimedia.org> wrote:
What your saying is making me think I’m wasting my time on improving this extension. Also other users that have spoken to me have thought this extension is great but could do with improvements which I am doing. We need to think of new users and how to improve there experence. The task was opened for a long while yet no one commented on it. I agree with legoktm feedback. “A process that annoys people based on nothing but the fact that theyhappened to be the last one touching a file *is* fundamentally broken.” yes hence why I’ve been making improvements by adding a button which is better then nothing right? As chad mentions it has no idea what is a typo fix compared to other things as it’s not A.I.
Thanks for working on this, Paladox. I think this can be a really useful feature for newcomers and experienced developers alike, if implemented well. I look forward to seeing it in action.
On Tuesday, 22 January 2019, 12:05:24 GMT, Thiemo Kreuz <thiemo.kreuz@wikimedia.de> wrote:
Fundamentally broken sounds like a bit of a stretch.
A process that annoys people based on nothing but the fact that they happened to be the last one touching a file *is* fundamentally broken. This is not how anyone should look for reviewers, neither manually nor automatically.
Here is a thought experiment: We could send review requests to the *least* active users that are still around, but *never* touched a file. The positive effects of such an approach include:
- More people get familiar with the code.
- Knowledge gets spread more evenly.
- Bottlenecks and bus factors get reduced.
- These people probably have more time.
- Review requests are spread more evenly.
- Workload is spread more evenly.
Still sounds like a bad idea? Sure, because it is. Now tell me: How is it more clever to do the *opposite* and dump review requests on people that have to much workload already?
At this point I don't care any more if we are talking about a fully automated process or a suggest button. Both are targeting the wrong people.
it was probably working quite well for our less-trafficked repositories.
What is the difference between being the last one fixing a typo in a low-traffic vs. high-traffic repository? In both cases it's the wrong person.
Thiemo
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
+1 to Niharika - the initial iteration caused some inconvenience, but I expect subsequent iterations to be useful. Thank you Paladox!
On 22 Jan 2019, at 13:09, Niharika Kohli nkohli@wikimedia.org wrote:
On Tue, Jan 22, 2019 at 6:12 PM Paladox via Wikitech-l < wikitech-l@lists.wikimedia.org> wrote:
What your saying is making me think I’m wasting my time on improving this extension. Also other users that have spoken to me have thought this extension is great but could do with improvements which I am doing. We need to think of new users and how to improve there experence. The task was opened for a long while yet no one commented on it. I agree with legoktm feedback. “A process that annoys people based on nothing but the fact that theyhappened to be the last one touching a file *is* fundamentally broken.” yes hence why I’ve been making improvements by adding a button which is better then nothing right? As chad mentions it has no idea what is a typo fix compared to other things as it’s not A.I.
Thanks for working on this, Paladox. I think this can be a really useful feature for newcomers and experienced developers alike, if implemented well. I look forward to seeing it in action.
On Tuesday, 22 January 2019, 12:05:24 GMT, Thiemo Kreuz < thiemo.kreuz@wikimedia.de> wrote:
Fundamentally broken sounds like a bit of a stretch.
A process that annoys people based on nothing but the fact that they happened to be the last one touching a file *is* fundamentally broken. This is not how anyone should look for reviewers, neither manually nor automatically.
Here is a thought experiment: We could send review requests to the *least* active users that are still around, but *never* touched a file. The positive effects of such an approach include:
- More people get familiar with the code.
- Knowledge gets spread more evenly.
- Bottlenecks and bus factors get reduced.
- These people probably have more time.
- Review requests are spread more evenly.
- Workload is spread more evenly.
Still sounds like a bad idea? Sure, because it is. Now tell me: How is it more clever to do the *opposite* and dump review requests on people that have to much workload already?
At this point I don't care any more if we are talking about a fully automated process or a suggest button. Both are targeting the wrong people.
it was probably working quite well for our less-trafficked repositories.
What is the difference between being the last one fixing a typo in a low-traffic vs. high-traffic repository? In both cases it's the wrong person.
Thiemo
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
-- Niharika Product Manager Community Tech Wikimedia Foundation _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
No, I added in a config so that it would disable automatically adding reviewers instead relying on users to press “Suggest Reviewers” button. On Tuesday, 22 January 2019, 08:11:23 GMT, Thiemo Kreuz thiemo.kreuz@wikimedia.de wrote:
[…] im adding that functionality to reviewers-by-blame.
I'm afraid I don't understand. Does this mean all the issues that make the plugin send passive-aggressive, misattributed spam will still be in place, possibly hitting peoples inboxes again any time somebody decides it would be a good idea to turn this feature set on again? My impression was more that all the ideas that have been shared in this email thread describe an *entirely* different approach, following entirely different ideas, with an entirely different UI, and entirely different setup. What's the point of stuffing this into the same, fundamentally broken codebase?
Sysops, please, for our sanity, do not let anybody enable this toxic plugin again.
Best Thiemo
_______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
This discussion could do with less theatrics IMO.
Hi all!
This plugin has been removed entirely from Wikimedia Gerrit[0]. I know of no one who intended to experiment with the plugin in its current form so it is now removed.
I have created a task to track suggestions for this plugin's improvement[1]. This task's scope is to track suggestions for improvements to the reviewers-by-blame plugin so they are not lost in this thread. Implementation details about individual suggestions should (likely) become seperate tasks.
Any discussion about what is needed to re-enable this plugin for Wikimedia's Gerrit is a different discussion for another task and time. We won't re-enable this plugin without notice or without further discussion.
On 19-01-21 18:20:13, Paladox via Wikitech-l wrote: FYI i have a working prototype working ("Suggest Reviewer") button.
On Monday, 21 January 2019, 16:32:35 GMT, Paladox via Wikitech-l wikitech-l@lists.wikimedia.org wrote:
I’m currently working on addressing all the feedback as fast as I can. I honestly think this extension is great especially for new users, who do not know they need reviewers or who would review there change. Granted this extension has some problems hence why feature requests were filed against the extension.
+1 to what Niharika and other have said: thank you for all your work on Gerrit, Paladox!
You have made maintaining and keeping Gerrit secure easier for me personally.
Thanks all for your thoughts on this thread, and thank you in advance for ensuring the task for suggestions for improvement is accurate.
-- Tyler
[0]. https://tools.wmflabs.org/sal/log/AWh2D-odA1BDhGjCUFA8 [1]. https://phabricator.wikimedia.org/T214397
Hello, This is Amir, Chair of the code of conduct committee, talking on behalf of it.
We received several reports about the discussion happening here which we will handle and act accordingly but in the mean time, we would like to remind everyone to follow CoC [1] and be more mindful of what they write. Think with yourself when writing an email "Is this email moving the discussion forward or it's just criticizing non-constructively?" If the answer is the latter, please refrain from sending it. Insisting on sending nonconstructive angry emails will get you banned temporarily or permanently from this mailing list.
[1]: https://www.mediawiki.org/wiki/Code_of_Conduct
Regards, Code of Conduct Committee, MediaWiki.
On Tue, Jan 22, 2019 at 4:36 PM Tyler Cipriani tcipriani@wikimedia.org wrote:
Hi all!
This plugin has been removed entirely from Wikimedia Gerrit[0]. I know of no one who intended to experiment with the plugin in its current form so it is now removed.
I have created a task to track suggestions for this plugin's improvement[1]. This task's scope is to track suggestions for improvements to the reviewers-by-blame plugin so they are not lost in this thread. Implementation details about individual suggestions should (likely) become seperate tasks.
Any discussion about what is needed to re-enable this plugin for Wikimedia's Gerrit is a different discussion for another task and time. We won't re-enable this plugin without notice or without further discussion.
On 19-01-21 18:20:13, Paladox via Wikitech-l wrote: FYI i have a working prototype working ("Suggest Reviewer") button.
On Monday, 21 January 2019, 16:32:35 GMT, Paladox via Wikitech-l <
wikitech-l@lists.wikimedia.org> wrote:
I’m currently working on addressing all the feedback as fast as I can. I honestly think this extension is great especially for new users, who do not know they need reviewers or who would review there change. Granted this extension has some problems hence why feature requests were filed against the extension.
+1 to what Niharika and other have said: thank you for all your work on Gerrit, Paladox!
You have made maintaining and keeping Gerrit secure easier for me personally.
Thanks all for your thoughts on this thread, and thank you in advance for ensuring the task for suggestions for improvement is accurate.
-- Tyler
[0]. https://tools.wmflabs.org/sal/log/AWh2D-odA1BDhGjCUFA8 [1]. https://phabricator.wikimedia.org/T214397 _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Umm, No.
-- Bawolff
On Fri, Jan 18, 2019 at 10:13 PM Pine W wiki.pine@gmail.com wrote:
I'm glad that this problematic change to communications was reverted.
I would like to suggest that this is the type of change that, when being planned, should get a design review from a third party before coding starts, should go through at least one RFC before coding starts, and be widely communicated before coding starts and again a week or two before deployment. Involving TechCom might also be appropriate. It appears that none of those happened here. In terms of process this situation looks to me like it's inexcusable.
In the English Wikipedia community, doing something like this would have a reasonable likelihood of costing an administrator their tools, and I hope that a similar degree of accountability is enforced in the engineering community. In particular, I expect engineering supervisors to follow established technical processes for changes that impact others' workflows, and if they decide to skip those processes without a compelling reason (such as a site stability problem) then I hope that they will be held accountable. Again, from my perspective, the failure to follow process here is inexcusable.
Pine ( https://meta.wikimedia.org/wiki/User:Pine ) _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Hi!
On Fri, Jan 18, 2019 at 10:13 PM Pine W wiki.pine@gmail.com wrote:
I'm glad that this problematic change to communications was reverted.
I would like to suggest that this is the type of change that, when being planned, should get a design review from a third party before coding starts, should go through at least one RFC before coding starts, and be
I think there's no reason to make it bigger deal than it is. There was a feature that people thought would be good, turned out it is not as good as expected, so it was disabled. Nothing broken, nobody hurt (well, maybe except some inboxes...). I don't think there's a reason to describe this situation as "inexcusable". Sometimes something that we expect to work one way and be useful turns out different way and things that seemed to be excellent idea turn out to be very bad one. And we have to adjust, and this is normal. We don't like such situations, but we know they happen, and as long as they are handled properly - and I think in this case it was - there's no reason to make it more than it is.
reasonable likelihood of costing an administrator their tools, and I hope that a similar degree of accountability is enforced in the engineering
I hope not. Expecting unreasonable perfection from people and processes and overreacting when inevitable problems happen will only lead to frustration, failure and stagnation. Every failure has some lesson to learn, but the lesson shouldn't be "let's find somebody to punish". I am not sure if that was the intent, but it kinda felt this way to me. And I don't think this is warranted neither in general nor in particular case.
Thanks,
On Fri, Jan 18, 2019 at 2:13 PM Pine W wiki.pine@gmail.com wrote:
I would like to suggest that this is the type of change that, when being planned, should get a design review from a third party before coding starts, should go through at least one RFC before coding starts, and be widely communicated before coding starts and again a week or two before deployment.
There was no coding to start--it's an upstream plugin.
One that I considered enabling ages ago, fwiw. I didn't because of the very issues outlined in this thread.
There _must_ be a way to disable this for certain files. Great examples:
site.pp in puppet en.json language file in MW core CommonSettings or InitialiseSettings in wmf-config
All examples of files that are edited by dozens of folks. Folks who could very well be unqualified to review your change and/or completely uninterested in it at all.
I did enable the reviewers (not by blame) plugin ages ago. This allows people to opt in with much more granularity (and would remove the need for the bot)
-Chad
On Fri, Jan 18, 2019 at 6:46 PM Chad innocentkiller@gmail.com wrote:
There _must_ be a way to disable this for certain files. Great examples:
site.pp in puppet en.json language file in MW core CommonSettings or InitialiseSettings in wmf-config
There *is* a way to disable it. See plugins docs: https://gerrit.wikimedia.org/r/plugins/reviewers-by-blame/Documentation/conf...
IMO the two blockers are having the plugin act under its own username and providing some form of user optout. The other issues are annoying but can be worked around (or opted out from in the worst case).
Am 19.01.19 um 03:46 schrieb Chad:
All examples of files that are edited by dozens of folks. Folks who could very well be unqualified to review your change and/or completely uninterested in it at all.
Coming to think of it, it would be much more useful to base the suggestion on who has *reviewed* and *merged* changes to the file before, as opposed to who has *authored* patches.
People who have merged before may merge again. People who have authored small changes to lots of files don't want to be spammed with review requests.
On 19-01-18 22:12:22, Pine W wrote:
I'm glad that this problematic change to communications was reverted.
Clarification: Enabling this plugin wasn't reverted, a configuration change was made to the default settings of the plugin.
Thanks to the helpful suggestions on this thread, it's my hope that the upstream plugin (in future) may contain additional configuration options to improve the usability of this plugin for everyone, including Wikimedia technical contributors.
I would like to suggest that this is the type of change that, when being planned, should get a design review from a third party before coding starts, should go through at least one RFC before coding starts, and be widely communicated before coding starts and again a week or two before deployment. Involving TechCom might also be appropriate. It appears that none of those happened here. In terms of process this situation looks to me like it's inexcusable.
As Chad mentioned this is a plugin developed by upstream Gerrit.
Enabling this plugin was tracked in Wikimedia's public Phabricator[0].
As is now well understood in hindsight, the default configuration of this plugin (as designed by Gerrit upstream) is far from optimal for Wikimedia technical contributors.
[0]. https://phabricator.wikimedia.org/T101131
In the English Wikipedia community, doing something like this would have a reasonable likelihood of costing an administrator their tools, and I hope that a similar degree of accountability is enforced in the engineering community. In particular, I expect engineering supervisors to follow established technical processes for changes that impact others' workflows, and if they decide to skip those processes without a compelling reason (such as a site stability problem) then I hope that they will be held accountable. Again, from my perspective, the failure to follow process here is inexcusable.
As was pointed out by others: it's difficult to make a comparison between the English Wikipedia community and the Wikimedia technical contributors community (although many folks belong to both groups). I don't believe holding individuals to a post hoc set of standards creates a healthy community in any case.
I do agree that technical contributors should be accountable. That is, technical contributors should strive to be responsive to issues when they arise (as issues will arise when attempting to accomplish goals in a technical space).
-- Tyler
Something new was tried in the hopes it'd be good, it turned out not to be good, it was reverted, and now there's some discussion about how to make it better. That's a successful process, not an unsuccessful one.
Given that this exact method of doing things is not only well-established on the English Wikipedia but is also a recommended pattern ( bold-revert-discuss https://en.wikipedia.org/wiki/Wikipedia:BRD), I'm not sure why you think this would be unacceptable there.
Dan
On Fri, 18 Jan 2019 at 22:13, Pine W wiki.pine@gmail.com wrote:
I'm glad that this problematic change to communications was reverted.
I would like to suggest that this is the type of change that, when being planned, should get a design review from a third party before coding starts, should go through at least one RFC before coding starts, and be widely communicated before coding starts and again a week or two before deployment. Involving TechCom might also be appropriate. It appears that none of those happened here. In terms of process this situation looks to me like it's inexcusable.
In the English Wikipedia community, doing something like this would have a reasonable likelihood of costing an administrator their tools, and I hope that a similar degree of accountability is enforced in the engineering community. In particular, I expect engineering supervisors to follow established technical processes for changes that impact others' workflows, and if they decide to skip those processes without a compelling reason (such as a site stability problem) then I hope that they will be held accountable. Again, from my perspective, the failure to follow process here is inexcusable.
Pine ( https://meta.wikimedia.org/wiki/User:Pine ) _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 18/01/2019 23:12, Pine W wrote:
I'm glad that this problematic change to communications was reverted.
I would like to suggest that this is the type of change that, when being planned, should get a design review from a third party before coding starts, should go through at least one RFC before coding starts, and be widely communicated before coding starts and again a week or two before deployment. Involving TechCom might also be appropriate. It appears that none of those happened here. In terms of process this situation looks to me like it's inexcusable.
In the English Wikipedia community, doing something like this would have a reasonable likelihood of costing an administrator their tools, and I hope that a similar degree of accountability is enforced in the engineering community. In particular, I expect engineering supervisors to follow established technical processes for changes that impact others' workflows, and if they decide to skip those processes without a compelling reason (such as a site stability problem) then I hope that they will be held accountable. Again, from my perspective, the failure to follow process here is inexcusable.
Pine
Hello Pine
We had the Gerrit reviewers-by-blame installed a while ago although it was not functional. Tyler, Gergo and I have been talking about that idea for quite a while and felt like it was a good idea to get patches reviewed.
On a quick though, if one authored some code, most probably that person knows the code and thus would qualify as a reviewers. For the few code I wrote from scratch, I am certainly interested in being added automatically.
Anyway, we went with upgrading the Gerrit plugin. I even wrote a blog post to explain a bit of the feature and other ways to find reviewers: https://phabricator.wikimedia.org/phame/post/view/139/
I don't write blogs that often. If I do it is because I am excited about some feature which I firmly believe to be a general improvement. I have been naive? Yes surely. Did we miss evaluating potential side effects? For sure.
Then one as to take in perspective the cost of trying and reverting versus spending ages and months on a project only to dish it out because that is not what the customer wanted. I, and several, choose the first path: quick cheap experiment with limited casualties. A benefit is that this thread gave a lot of exposure to the feature and gathered a lot of constructive feedback. One can then easily conclude that the plugin is not smart enough and fails to spot the proper reviewers.
The plugin does not add reviewers any more (it defaults to add 0 reviewers), and I guess we will just uninstall it entirely.
As for new features for Gerrit or Phabricator or CI. There is no due process established. We just f***ing do it, given we promptly rollback when we screw up which is thankfully rather rare. Else we iterate and refine the feature until it is deemed stable.
That is how we maintain our infrastructure, not by having four hours meetings week after weeks with no deployment in between, not by having cross teams agreement, nor five level of political hierarchy drama. We certainly had a few outages here and there, but given the very few people working on those parts and the number of modifications we do on a weekly basis, I think it is overall rather stable.
I am not willing to start a flame war, but I do not think the English Wikipedia community is a good example of an healthy one. The huge amount of process and policies makes it a challenge to have edits retained, it is partly what made me stop editing entirely.
In short, Pine, please assume good faith 8-]
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512
Hi,
On 1/18/19 6:11 AM, Tyler Cipriani wrote:
In the interim, project-owners are able to opt-in to using the reviewers-by-blame plugin on a per-project basis on their project admin page in Gerrit.
I'm happy to volunteer any of the projects I help maintain as a guinea pig once modifications to the plugin are ready for wider testing.
Also, the Git Reviewer Bot[1] provides folks an opt-in method of volunteering to review a subset of files in a particular repo.
IMO, the main place where the Reviewer-Bot falls short is that it relies on a hardcoded list of file paths that often change and then requires manual intervention. Using blame is nice in that it's automatic .
Getting code review as a new contributor is hard. Thanks for bearing with us.
I've spoken to plenty people that complain that their patch was never merged, and after looking, it had no reviewers assigned :-(
My suggestions for the plugin: * Require some manual interaction by the patch author, similar to GitHub (I believe Paladox is already working on this) * Some account-level opt-out for bots, people who have left the movement, etc. * Don't suggest people that have already been manually removed from the patch * Only suggest people that have been active on Gerrit (or maybe limit it to that repo) in the past X months/years. ** For repositories that have no active development but a maintainer who still checks email, using a time-based check might not work. In a past experiment I looked at the last 100 merged patches. * If feasible, introduce some kind of marking (hashtag? topic?) that can be used in mass cleanup style commits so that they are ignored by the blame so drive by contributors aren't suggested as reviewers. I currently have a mail filter with a few different Gerrit topics (e.g. bump-dev-deps) to separate those review requests from other ones.
Thanks to those that are working on this. - -- Legoktm
wikitech-l@lists.wikimedia.org