On Fri, Jun 15, 2012 at 8:48 AM, Sumana Harihareswara sumanah@wikimedia.org wrote:
If you merge into mediawiki/core.git, your change is considered safe for inclusion in a wmf branch. The wmf branch is just branched out of master and then deployed. We don't review it again. Because we're deploying more frequently to WMF sites, the code review for merging into MediaWiki's core.git needs to be more like deployment/shell-level review, and so we gave merge access to people who already had deployment access. We have since added some more people. The current list: https://gerrit.wikimedia.org/r/#/admin/groups/11,members
Let me elaborate on this. As unclear as our process is for giving access, it's even less clear what our policy is for taking it away. If we can settle on a policy for taking access away/suspending access, it'll make it much easier to loosen up about giving access.
Here's the situation we want to avoid: we give access to someone who probably shouldn't have it. They continually introduce deployment blockers into the code, making us need to slow down our frequent deployment process. Two hour deploy windows become six hour deploy windows as we need time to fix up breakage introduced during the window. Even with the group we have, there are times where things that really shouldn't slip through do. It's manageable now, but adding more people is going to multiply this problem as we get back into a situation where poorly conceived changes become core dependencies.
We haven't had a culture of making a big deal about the case when someone introduces a breaking change or does something that brings the db to its knees or introduces a massive security hole or whatever. That means that if the situation were to arise that we needed to revoke someones access, we have to wait until it gets egregious and awful, and even then the person is likely to be shocked that their rights are being revoked (if we even do it then). To be less conservative about giving access, we also need to figure out how to be less conservative about taking it away. We also want to be as reasonably objective about it. It's always going to be somewhat subjective, and we don't want to completely eliminate the role of common sense.
It would also be nice if we didn't have to resort to the nuclear option to get the point across. One low-stakes way we can use to make sure people are more careful is to have some sort of rotating "oops" award. At one former job I had, we had a Ghostbusters Stay Puft doll named "Buster" that was handed out when someone broke the build that they had to prominently display in their office. At another job, it was a pair of Shrek ears that people had to wear when they messed something up in production. In both cases, it was something you had to wear until someone else came along. Perhaps we should institute something similar (maybe as simple as asking people to append "OOPS" to their IRC nicks when they botch something).
Rob
On Fri, Jun 15, 2012 at 1:53 PM, Rob Lanphier robla@wikimedia.org wrote:
It would also be nice if we didn't have to resort to the nuclear option to get the point across. One low-stakes way we can use to make sure people are more careful is to have some sort of rotating "oops" award. At one former job I had, we had a Ghostbusters Stay Puft doll named "Buster" that was handed out when someone broke the build that they had to prominently display in their office. At another job, it was a pair of Shrek ears that people had to wear when they messed something up in production. In both cases, it was something you had to wear until someone else came along. Perhaps we should institute something similar (maybe as simple as asking people to append "OOPS" to their IRC nicks when they botch something).
whobrokeitthistime.wikimedia.org? :)
Roan
On Sat, Jun 16, 2012 at 9:41 AM, Roan Kattouw roan.kattouw@gmail.com wrote:
whobrokeitthistime.wikimedia.org? :)
Roan
Point it to http://en.wikipedia.org/wiki/Wikipedia:BLAMEWHEEL ?
On Sat, Jun 16, 2012 at 6:53 AM, Rob Lanphier robla@wikimedia.org wrote:
On Fri, Jun 15, 2012 at 8:48 AM, Sumana Harihareswara sumanah@wikimedia.org wrote:
If you merge into mediawiki/core.git, your change is considered safe for inclusion in a wmf branch. The wmf branch is just branched out of master and then deployed. We don't review it again. Because we're deploying more frequently to WMF sites, the code review for merging into MediaWiki's core.git needs to be more like deployment/shell-level review, and so we gave merge access to people who already had deployment access. We have since added some more people. The current list: https://gerrit.wikimedia.org/r/#/admin/groups/11,members
Let me elaborate on this. As unclear as our process is for giving access, it's even less clear what our policy is for taking it away. If we can settle on a policy for taking access away/suspending access, it'll make it much easier to loosen up about giving access.
Here's the situation we want to avoid: we give access to someone who probably shouldn't have it. They continually introduce deployment blockers into the code, making us need to slow down our frequent deployment process. Two hour deploy windows become six hour deploy windows as we need time to fix up breakage introduced during the window. Even with the group we have, there are times where things that really shouldn't slip through do. It's manageable now, but adding more people is going to multiply this problem as we get back into a situation where poorly conceived changes become core dependencies.
We haven't had a culture of making a big deal about the case when someone introduces a breaking change or does something that brings the db to its knees or introduces a massive security hole or whatever. That means that if the situation were to arise that we needed to revoke someones access, we have to wait until it gets egregious and awful, and even then the person is likely to be shocked that their rights are being revoked (if we even do it then). To be less conservative about giving access, we also need to figure out how to be less conservative about taking it away. We also want to be as reasonably objective about it. It's always going to be somewhat subjective, and we don't want to completely eliminate the role of common sense.
It would also be nice if we didn't have to resort to the nuclear option to get the point across. One low-stakes way we can use to make sure people are more careful is to have some sort of rotating "oops" award. At one former job I had, we had a Ghostbusters Stay Puft doll named "Buster" that was handed out when someone broke the build that they had to prominently display in their office. At another job, it was a pair of Shrek ears that people had to wear when they messed something up in production. In both cases, it was something you had to wear until someone else came along. Perhaps we should institute something similar (maybe as simple as asking people to append "OOPS" to their IRC nicks when they botch something).
In general I think we'd want to start by making sure that the person who broke something actually found out that they broke it. I don't think we need to get into "punishment" unless we actually start having serious problems. Otherwise this is a solution looking for a problem.
In terms of low stakes "punishment" for breaking the build, I've heard of an organisation where the last person who broke it is responsible for some unpleasant task (resolving merge conflicts?). In more homogenous co-located organisations I can see something like "has to buy a drink for the people who cleaned up after it" working, but that doesn't really work in our organisation.
On 16.06.2012 8:20, Andrew Garrett wrote:
On Sat, Jun 16, 2012 at 6:53 AM, Rob Lanphierrobla@wikimedia.org wrote:
On Fri, Jun 15, 2012 at 8:48 AM, Sumana Harihareswara sumanah@wikimedia.org wrote:
If you merge into mediawiki/core.git, your change is considered safe for inclusion in a wmf branch. The wmf branch is just branched out of master and then deployed. We don't review it again. Because we're deploying more frequently to WMF sites, the code review for merging into MediaWiki's core.git needs to be more like deployment/shell-level review, and so we gave merge access to people who already had deployment access. We have since added some more people. The current list: https://gerrit.wikimedia.org/r/#/admin/groups/11,members
Let me elaborate on this. As unclear as our process is for giving access, it's even less clear what our policy is for taking it away. If we can settle on a policy for taking access away/suspending access, it'll make it much easier to loosen up about giving access.
Here's the situation we want to avoid: we give access to someone who probably shouldn't have it. They continually introduce deployment blockers into the code, making us need to slow down our frequent deployment process. Two hour deploy windows become six hour deploy windows as we need time to fix up breakage introduced during the window. Even with the group we have, there are times where things that really shouldn't slip through do. It's manageable now, but adding more people is going to multiply this problem as we get back into a situation where poorly conceived changes become core dependencies.
We haven't had a culture of making a big deal about the case when someone introduces a breaking change or does something that brings the db to its knees or introduces a massive security hole or whatever. That means that if the situation were to arise that we needed to revoke someones access, we have to wait until it gets egregious and awful, and even then the person is likely to be shocked that their rights are being revoked (if we even do it then). To be less conservative about giving access, we also need to figure out how to be less conservative about taking it away. We also want to be as reasonably objective about it. It's always going to be somewhat subjective, and we don't want to completely eliminate the role of common sense.
It would also be nice if we didn't have to resort to the nuclear option to get the point across. One low-stakes way we can use to make sure people are more careful is to have some sort of rotating "oops" award. At one former job I had, we had a Ghostbusters Stay Puft doll named "Buster" that was handed out when someone broke the build that they had to prominently display in their office. At another job, it was a pair of Shrek ears that people had to wear when they messed something up in production. In both cases, it was something you had to wear until someone else came along. Perhaps we should institute something similar (maybe as simple as asking people to append "OOPS" to their IRC nicks when they botch something).
In general I think we'd want to start by making sure that the person who broke something actually found out that they broke it. I don't think we need to get into "punishment" unless we actually start having serious problems. Otherwise this is a solution looking for a problem.
In terms of low stakes "punishment" for breaking the build, I've heard of an organisation where the last person who broke it is responsible for some unpleasant task (resolving merge conflicts?). In more homogenous co-located organisations I can see something like "has to buy a drink for the people who cleaned up after it" working, but that doesn't really work in our organisation.
Shrek ears?? Why people have to be humiliated? Why don't just: 1. temporary reduce the salary 2. permanently reduce the salary 3. fire ? Dmitriy
On Jun 16, 2012, at 12:23 AM, Dmitriy Sintsov wrote:
Shrek ears?? Why people have to be humiliated? Why don't just:
- temporary reduce the salary
- permanently reduce the salary
- fire
?
I'm not sure that your solution is less of a humiliation, my friend.
--- Brandon Harris, Senior Designer, Wikimedia Foundation
Support Free Knowledge: http://wikimediafoundation.org/wiki/Donate
Dmitriy Sintsov wrote:
Shrek ears?? Why people have to be humiliated? Why don't just:
- temporary reduce the salary
- permanently reduce the salary
- fire
? Dmitriy
That approach but it has serious drawbacks:
- people will be afraid about doing anything => short term consequences is that will have people covering their ass to avoid any responsability. => long term, talented people go away and the organization as a whole is no more progressing.
- lowering salary would make people feel less committed to their work, they will in turn produce less and eventually leave for greener pastures.
- fire is always a last resort. Nobody wants to fire people.
Shreak ears could be a lot of fun if everyone adheres to it and acknowledge it is not about humiliating but about having fun. That could be replaced with a "PHP Exception barnstar" :-]
Anyway, with lot of people being remote, that is probably not going to work for us!
On Sat, Jun 16, 2012 at 8:03 AM, Antoine Musso hashar+wmf@free.fr wrote:
Dmitriy Sintsov wrote:
Shrek ears?? Why people have to be humiliated? Why don't just:
- temporary reduce the salary
- permanently reduce the salary
- fire
? Dmitriy
That approach but it has serious drawbacks:
Such as the fact that a good number of our developers are volunteers and we can't fire them ;-)
Overall though, I kind of like the idea of a "village stocks" style of recognition when you break the site. Back on CodeReview, it was impossible to convince people to resolve their fixmes--endless mailing list and personal nagging did not work. However, once I put the "list of fixmes by author" report in (aka: The wall of shame), people got much much better at taking care of them. I think ways of highlighting the mistake but showing where you can fix it is what's most useful here. Nobody wants to be at the top of the wall of shame, so they fix their code and learn as a result. Something similar here would be useful.
-Chad
On 06/15/2012 04:53 PM, Rob Lanphier wrote:
On Fri, Jun 15, 2012 at 8:48 AM, Sumana Harihareswara sumanah@wikimedia.org wrote:
If you merge into mediawiki/core.git, your change is considered safe for inclusion in a wmf branch. The wmf branch is just branched out of master and then deployed. We don't review it again. Because we're deploying more frequently to WMF sites, the code review for merging into MediaWiki's core.git needs to be more like deployment/shell-level review, and so we gave merge access to people who already had deployment access. We have since added some more people. The current list: https://gerrit.wikimedia.org/r/#/admin/groups/11,members
Let me elaborate on this. As unclear as our process is for giving access, it's even less clear what our policy is for taking it away. If we can settle on a policy for taking access away/suspending access, it'll make it much easier to loosen up about giving access.
Here's the situation we want to avoid: we give access to someone who probably shouldn't have it. They continually introduce deployment blockers into the code, making us need to slow down our frequent deployment process. Two hour deploy windows become six hour deploy windows as we need time to fix up breakage introduced during the window. Even with the group we have, there are times where things that really shouldn't slip through do. It's manageable now, but adding more people is going to multiply this problem as we get back into a situation where poorly conceived changes become core dependencies.
We haven't had a culture of making a big deal about the case when someone introduces a breaking change or does something that brings the db to its knees or introduces a massive security hole or whatever. That means that if the situation were to arise that we needed to revoke someones access, we have to wait until it gets egregious and awful, and even then the person is likely to be shocked that their rights are being revoked (if we even do it then). To be less conservative about giving access, we also need to figure out how to be less conservative about taking it away. We also want to be as reasonably objective about it. It's always going to be somewhat subjective, and we don't want to completely eliminate the role of common sense.
It would also be nice if we didn't have to resort to the nuclear option to get the point across. One low-stakes way we can use to make sure people are more careful is to have some sort of rotating "oops" award. At one former job I had, we had a Ghostbusters Stay Puft doll named "Buster" that was handed out when someone broke the build that they had to prominently display in their office. At another job, it was a pair of Shrek ears that people had to wear when they messed something up in production. In both cases, it was something you had to wear until someone else came along. Perhaps we should institute something similar (maybe as simple as asking people to append "OOPS" to their IRC nicks when they botch something).
Rob
[0]
Now that we are being looser in granting access,[1] it's probably a good idea to figure out how and when to remove access -- the D in CRUD, right? :-) There should be social and technical procedures for removing members from Gerrit project owner groups. As Rob said, if someone is a chronic offender in breaking the deployment, then it's important to have some consequence. Possible reasons to consider removing members should include *persistently* breaking things through negligence or incompetence, lack of respect for other community members, anticollaborative behavior, and possibly "I have left the community and have no plans to come back" (as opposed to "I am now going to take a year off from MediaWiki to concentrate on my new baby" or a similar hiatus). There were more thoughts on this in http://thread.gmane.org/gmane.science.linguistics.wikipedia.technical/59681 .
I personally want clearer guidelines for this *so that we can be more open to giving more volunteers +2*.
[0] (original thread: http://www.gossamer-threads.com/lists/wiki/wikitech/281340 . Summary: let's not create solutions in search of problems; ideas for embarrassing IRC nicknames and other shame-based norm enforcement; this is about fun and responsibility, not humiliation; the need to balance caution and decisiveness.)
[1] Per https://www.mediawiki.org/wiki/Talk:Git/Gerrit_project_ownership#Removing_un... the policy for giving +2 for Git repositories is now "If there is consensus from the existing project owners, then we'll approve the candidate." This is a change to the previous policy, which allowed a single existing owner to veto a candidate. Chad made this change to policy on the 4th: https://www.mediawiki.org/w/index.php?title=Git%2FGerrit_project_ownership&a...
I know this is pretty obvious, but self-merging pretty much any change should be grounds for removal (or at the very least no second chance).
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Thu, Feb 14, 2013 at 6:19 PM, Sumana Harihareswara <sumanah@wikimedia.org
wrote:
On 06/15/2012 04:53 PM, Rob Lanphier wrote:
On Fri, Jun 15, 2012 at 8:48 AM, Sumana Harihareswara sumanah@wikimedia.org wrote:
If you merge into mediawiki/core.git, your change is considered safe for inclusion in a wmf branch. The wmf branch is just branched out of master and then deployed. We don't review it again. Because we're deploying more frequently to WMF sites, the code review for merging into MediaWiki's core.git needs to be more like deployment/shell-level review, and so we gave merge access to people who already had deployment access. We have since added some more people. The current list: https://gerrit.wikimedia.org/r/#/admin/groups/11,members
Let me elaborate on this. As unclear as our process is for giving access, it's even less clear what our policy is for taking it away. If we can settle on a policy for taking access away/suspending access, it'll make it much easier to loosen up about giving access.
Here's the situation we want to avoid: we give access to someone who probably shouldn't have it. They continually introduce deployment blockers into the code, making us need to slow down our frequent deployment process. Two hour deploy windows become six hour deploy windows as we need time to fix up breakage introduced during the window. Even with the group we have, there are times where things that really shouldn't slip through do. It's manageable now, but adding more people is going to multiply this problem as we get back into a situation where poorly conceived changes become core dependencies.
We haven't had a culture of making a big deal about the case when someone introduces a breaking change or does something that brings the db to its knees or introduces a massive security hole or whatever. That means that if the situation were to arise that we needed to revoke someones access, we have to wait until it gets egregious and awful, and even then the person is likely to be shocked that their rights are being revoked (if we even do it then). To be less conservative about giving access, we also need to figure out how to be less conservative about taking it away. We also want to be as reasonably objective about it. It's always going to be somewhat subjective, and we don't want to completely eliminate the role of common sense.
It would also be nice if we didn't have to resort to the nuclear option to get the point across. One low-stakes way we can use to make sure people are more careful is to have some sort of rotating "oops" award. At one former job I had, we had a Ghostbusters Stay Puft doll named "Buster" that was handed out when someone broke the build that they had to prominently display in their office. At another job, it was a pair of Shrek ears that people had to wear when they messed something up in production. In both cases, it was something you had to wear until someone else came along. Perhaps we should institute something similar (maybe as simple as asking people to append "OOPS" to their IRC nicks when they botch something).
Rob
[0]
Now that we are being looser in granting access,[1] it's probably a good idea to figure out how and when to remove access -- the D in CRUD, right? :-) There should be social and technical procedures for removing members from Gerrit project owner groups. As Rob said, if someone is a chronic offender in breaking the deployment, then it's important to have some consequence. Possible reasons to consider removing members should include *persistently* breaking things through negligence or incompetence, lack of respect for other community members, anticollaborative behavior, and possibly "I have left the community and have no plans to come back" (as opposed to "I am now going to take a year off from MediaWiki to concentrate on my new baby" or a similar hiatus). There were more thoughts on this in http://thread.gmane.org/gmane.science.linguistics.wikipedia.technical/59681 .
I personally want clearer guidelines for this *so that we can be more open to giving more volunteers +2*.
[0] (original thread: http://www.gossamer-threads.com/lists/wiki/wikitech/281340 . Summary: let's not create solutions in search of problems; ideas for embarrassing IRC nicknames and other shame-based norm enforcement; this is about fun and responsibility, not humiliation; the need to balance caution and decisiveness.)
[1] Per
https://www.mediawiki.org/wiki/Talk:Git/Gerrit_project_ownership#Removing_un... the policy for giving +2 for Git repositories is now "If there is consensus from the existing project owners, then we'll approve the candidate." This is a change to the previous policy, which allowed a single existing owner to veto a candidate. Chad made this change to policy on the 4th:
https://www.mediawiki.org/w/index.php?title=Git%2FGerrit_project_ownership&a...
-- Sumana Harihareswara Engineering Community Manager Wikimedia Foundation
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Feb 15, 2013, at 1:11 AM, Tyler Romeo tylerromeo@gmail.com wrote:
I know this is pretty obvious, but self-merging pretty much any change should be grounds for removal (or at the very least no second chance).
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
Though certain repositories in Gerrit have their own policies (such as operations, who appears to use Git mostly as a log, they self-merge continuously, maybe even after deployment - don't know if that's the case)
But I agree that for MediaWiki core self-merging needs to be frowned upon more. I don't know if revoking access should be a directly correlated consequence though, that may be too extreme.
Note though that there are at least 2 commonly accepted exceptions:
* Backporting a change approved by someone else in master to another branch, and self-merging that backport
e.g. User A pushes for review to master, User B approves it and merges it, User A or User C then pushes the same change to another branch and self-merges that) - such as to REL branches or WMF branches.
* Reverting
Due to the way Git and Gerrit interact, a revert (unlike a merge) is a two-step process. It considers it to be a separate commit (which it is, of course). So clicking Revert only drafts a commit, it still needs to be merged. This is generally done straight away if the revert reason comes from someone with merge rights.
These 2 cases (and maybe more, such as i18n-bot) were the reasons that a few weeks/months back we didn't make the change in Gerrit to make self-merging no longer an option (e.g. reject merge attempts by the author of the change).
-- Krinkle
Mhm, yep. And yeah, we'd definitely want to be a little lenient.
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On Thu, Feb 14, 2013 at 7:29 PM, Krinkle krinklemail@gmail.com wrote:
On Feb 15, 2013, at 1:11 AM, Tyler Romeo tylerromeo@gmail.com wrote:
I know this is pretty obvious, but self-merging pretty much any change should be grounds for removal (or at the very least no second chance).
*--* *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
Though certain repositories in Gerrit have their own policies (such as operations, who appears to use Git mostly as a log, they self-merge continuously, maybe even after deployment - don't know if that's the case)
But I agree that for MediaWiki core self-merging needs to be frowned upon more. I don't know if revoking access should be a directly correlated consequence though, that may be too extreme.
Note though that there are at least 2 commonly accepted exceptions:
- Backporting a change approved by someone else in master to another
branch, and self-merging that backport
e.g. User A pushes for review to master, User B approves it and merges it, User A or User C then pushes the same change to another branch and self-merges that) - such as to REL branches or WMF branches.
- Reverting
Due to the way Git and Gerrit interact, a revert (unlike a merge) is a two-step process. It considers it to be a separate commit (which it is, of course). So clicking Revert only drafts a commit, it still needs to be merged. This is generally done straight away if the revert reason comes from someone with merge rights.
These 2 cases (and maybe more, such as i18n-bot) were the reasons that a few weeks/months back we didn't make the change in Gerrit to make self-merging no longer an option (e.g. reject merge attempts by the author of the change).
-- Krinkle
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 02/14/2013 07:29 PM, Krinkle wrote:
Due to the way Git and Gerrit interact, a revert (unlike a merge) is a two-step process. It considers it to be a separate commit (which it is, of course). So clicking Revert only drafts a commit, it still needs to be merged. This is generally done straight away if the revert reason comes from someone with merge rights.
I'm not sure this should be an exception. In general (potentially excluding urgent hotfixes if one is confident the fix is correct), the decision to revert itself should be reviewed.
Matt Flaschen
On Thu, Feb 14, 2013 at 7:29 PM, Krinkle krinklemail@gmail.com wrote:
Note though that there are at least 2 commonly accepted exceptions:
Backporting a change approved by someone else in master to another branch, and self-merging that backport
Reverting
I'd propose one more:
* Someone else gives +2, but Jenkins rejects it because it needs a rebase that is not quite trivial enough for it to do it automatically. For example, something in RELEASE-NOTES-1.21.
On 15/02/13 01:38, Brad Jorsch wrote:
I'd propose one more:
- Someone else gives +2, but Jenkins rejects it because it needs a
rebase that is not quite trivial enough for it to do it automatically. For example, something in RELEASE-NOTES-1.21.
Seems a better example. I'm not convinced that backporting should be automatically merged, though. Even if the code at REL-old is the same as master (ie. the backport doesn't needs any code change), approving something from master is different than agreeing that it should be merged to REL-old (unless explicitly stated in the previous change). I'm not too firm on that for changes that it's obvious should be backported, such as a XSS fix*, but I would completely oppose to automerge a minor feature because it was merged into master. Note that we are not alone opinating about what it's worth backporting, since downstream distros will also call into question if our new release is “just bugfixes” before they agree into accepting it as-is.
* Still, we could be making a complete new class in master but just stripping the vulnerable piece in the old release.
On Feb 15, 2013, at 3:32 PM, Platonides platonides@gmail.com wrote:
I'm not convinced that backporting should be automatically merged, though. Even if the code at REL-old is the same as master (ie. the backport doesn't needs any code change), approving something from master is different than agreeing that it should be merged to REL-old (unless explicitly stated in the previous change). I'm not too firm on that for changes that it's obvious should be backported, such as a XSS fix*, but I would completely oppose to automerge a minor feature because it was merged into master. Note that we are not alone opinating about what it's worth backporting, since downstream distros will also call into question if our new release is “just bugfixes” before they agree into accepting it as-is.
I don't know where you pull "auto-merging" from but it certainly isn't from my e-mail, which was about revoking merge access and about when self-merging may or may not be tolerated.
Auto-merging would imply some random dude can take a change from master merged by someone else *for master*, and submit it to any branch and have it be auto-merged.
What I was talking about is that a code reviewer with merge access can submit an approved change from master to another branch and self-merge it.
Just because one can however doesn't mean one should.
When our random dude pushes a change for review to an old branch that backports a feature from master, the assigned reviewer should (as you explain) not approve it.
And for the same reason, when that reviewer backports himself, he wouldn't self-merge. Or rather, he wouldn't draft such a change in the first place.
-- Krinkle
On Fri, Feb 15, 2013 at 6:32 AM, Platonides Platonides@gmail.com wrote:
On 15/02/13 01:38, Brad Jorsch wrote:
I'd propose one more:
- Someone else gives +2, but Jenkins rejects it because it needs a
rebase that is not quite trivial enough for it to do it automatically. For example, something in RELEASE-NOTES-1.21.
Seems a better example. I'm not convinced that backporting should be automatically merged, though. Even if the code at REL-old is the same as master (ie. the backport doesn't needs any code change), approving something from master is different than agreeing that it should be merged to REL-old (unless explicitly stated in the previous change). I'm not too firm on that for changes that it's obvious should be backported, such as a XSS fix*, but I would completely oppose to automerge a minor feature because it was merged into master.
We should probably reset the subject again since this is something of a tangent from revoking +2, but since it was brought up, let me clarify the process behind when gerrit reports that someone is doing a self-merge for security fixes (and I welcome input for ways to improve the process!).
When we get a report of an xss (assuming isn't not yet "public" or being actively exploited), we file a bugzilla ticket in the security category. Usually me or someone else in that group adds a patch to the bug. Someone else gives a "yes, this looks ok to deploy" comment on the bug. The patch is put into production, backported to the supported release versions, then we release tarballs / patches. When the tarballs are released, I typically submit and merge the patches into master and supported branches, since we have a number of users who pull from git to update their systems.
This process is painful (no one like reviewing patches in bugzilla), and the wrangling to get the right people to review patches in bugzilla is slowing down our security releases. It would be much better if we had a way to submit the patches in gerrit, go through the normal review process by a trusted group of developers ending in a +2's, and then the final merge is just a single click when we release the tarballs. But we haven't been able to get gerrit to do that yet (although if any java developers want to work on that, I would be very excited).
Note that we are not alone opinating about what it's worth backporting, since downstream distros will also call into question if our new release is “just bugfixes” before they agree into accepting it as-is.
- Still, we could be making a complete new class in master but just
stripping the vulnerable piece in the old release.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 15/02/13 18:51, Chris Steipp wrote:
This process is painful (no one like reviewing patches in bugzilla), and the wrangling to get the right people to review patches in bugzilla is slowing down our security releases. It would be much better if we had a way to submit the patches in gerrit, go through the normal review process by a trusted group of developers ending in a +2's, and then the final merge is just a single click when we release the tarballs. But we haven't been able to get gerrit to do that yet (although if any java developers want to work on that, I would be very excited).
gerrit drafts? Although those are not as private as we would like. Another option would be to email those amongst +2 reviewers, although willingness to review through email perhaps won't be bigger than in bugzilla.
On 02/14/2013 03:19 PM, Sumana Harihareswara wrote:
On 06/15/2012 04:53 PM, Rob Lanphier wrote:
On Fri, Jun 15, 2012 at 8:48 AM, Sumana Harihareswara sumanah@wikimedia.org wrote:
If you merge into mediawiki/core.git, your change is considered safe for inclusion in a wmf branch. The wmf branch is just branched out of master and then deployed. We don't review it again. Because we're deploying more frequently to WMF sites, the code review for merging into MediaWiki's core.git needs to be more like deployment/shell-level review, and so we gave merge access to people who already had deployment access. We have since added some more people. The current list: https://gerrit.wikimedia.org/r/#/admin/groups/11,members
Let me elaborate on this. As unclear as our process is for giving access, it's even less clear what our policy is for taking it away. If we can settle on a policy for taking access away/suspending access, it'll make it much easier to loosen up about giving access.
Here's the situation we want to avoid: we give access to someone who probably shouldn't have it. They continually introduce deployment blockers into the code, making us need to slow down our frequent deployment process. Two hour deploy windows become six hour deploy windows as we need time to fix up breakage introduced during the window. Even with the group we have, there are times where things that really shouldn't slip through do. It's manageable now, but adding more people is going to multiply this problem as we get back into a situation where poorly conceived changes become core dependencies.
We haven't had a culture of making a big deal about the case when someone introduces a breaking change or does something that brings the db to its knees or introduces a massive security hole or whatever. That means that if the situation were to arise that we needed to revoke someones access, we have to wait until it gets egregious and awful, and even then the person is likely to be shocked that their rights are being revoked (if we even do it then). To be less conservative about giving access, we also need to figure out how to be less conservative about taking it away. We also want to be as reasonably objective about it. It's always going to be somewhat subjective, and we don't want to completely eliminate the role of common sense.
It would also be nice if we didn't have to resort to the nuclear option to get the point across. One low-stakes way we can use to make sure people are more careful is to have some sort of rotating "oops" award. At one former job I had, we had a Ghostbusters Stay Puft doll named "Buster" that was handed out when someone broke the build that they had to prominently display in their office. At another job, it was a pair of Shrek ears that people had to wear when they messed something up in production. In both cases, it was something you had to wear until someone else came along. Perhaps we should institute something similar (maybe as simple as asking people to append "OOPS" to their IRC nicks when they botch something).
Rob
[0]
Now that we are being looser in granting access,[1] it's probably a good idea to figure out how and when to remove access -- the D in CRUD, right? :-) There should be social and technical procedures for removing members from Gerrit project owner groups. As Rob said, if someone is a chronic offender in breaking the deployment, then it's important to have some consequence. Possible reasons to consider removing members should include *persistently* breaking things through negligence or incompetence, lack of respect for other community members, anticollaborative behavior, and possibly "I have left the community and have no plans to come back" (as opposed to "I am now going to take a year off from MediaWiki to concentrate on my new baby" or a similar hiatus). There were more thoughts on this in http://thread.gmane.org/gmane.science.linguistics.wikipedia.technical/59681 .
I personally want clearer guidelines for this *so that we can be more open to giving more volunteers +2*.
[0] (original thread: http://www.gossamer-threads.com/lists/wiki/wikitech/281340 . Summary: let's not create solutions in search of problems; ideas for embarrassing IRC nicknames and other shame-based norm enforcement; this is about fun and responsibility, not humiliation; the need to balance caution and decisiveness.)
[1] Per https://www.mediawiki.org/wiki/Talk:Git/Gerrit_project_ownership#Removing_un... the policy for giving +2 for Git repositories is now "If there is consensus from the existing project owners, then we'll approve the candidate." This is a change to the previous policy, which allowed a single existing owner to veto a candidate. Chad made this change to policy on the 4th: https://www.mediawiki.org/w/index.php?title=Git%2FGerrit_project_ownership&a...
OK, the policy is at https://www.mediawiki.org/wiki/%2B2#Revocation - RobLa reminded me that we already had had this discussion. :-) Thanks.
wikitech-l@lists.wikimedia.org