Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-17 Thread Erik Moeller
On Mon, Jun 13, 2011 at 11:39 PM, Erik Moeller e...@wikimedia.org wrote: To keep the ball rolling, I've started a draft here, based in part on your comments: http://www.mediawiki.org/wiki/Development_process_improvement/20%25_policy As a quick update, yesterday we had a conversation to think

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-14 Thread Erik Moeller
On Thu, Jun 2, 2011 at 2:26 AM, Roan Kattouw roan.katt...@gmail.com wrote: I like the 20% model as well. It goes a little bit further than my suggestion, which was 20, 25 or 33% model for certain senior devs, but if you're volunteering to give me more than what I asked for that's even better

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-14 Thread MZMcBride
Erik Moeller wrote: On Thu, Jun 2, 2011 at 2:26 AM, Roan Kattouw roan.katt...@gmail.com wrote: I like the 20% model as well. It goes a little bit further than my suggestion, which was 20, 25 or 33% model for certain senior devs, but if you're volunteering to give me more than what I asked for

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-06 Thread Rob Lanphier
On Sun, Jun 5, 2011 at 9:13 PM, MZMcBride z...@mzmcbride.com wrote: If there's any support or help that non-developers can offer in this process, please update the mailing list with information as to how. A lot of people are keen on seeing some steady progress forward. :-) I think one thing

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-05 Thread MZMcBride
Rob Lanphier wrote: Here's an immediate challenge we face: in order to release 1.18, we need to get the red line in this graph to zero: http://toolserver.org/~robla/crstats/crstats.118all.html As of midnight UTC on June 3, we were at 1594 revisions. Doing the math, in order to get close to

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-03 Thread Rob Lanphier
On Wed, Jun 1, 2011 at 5:58 PM, Erik Moeller e...@wikimedia.org wrote: Right. And just to weigh in quickly on the resources issue -- the review/deploy/release train is clearly not moving at the pace we want. This does affect WMF staffers and contractors as well, but we know that it's

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-03 Thread Brion Vibber
On Fri, Jun 3, 2011 at 3:33 PM, Rob Lanphier ro...@wikimedia.org wrote: So, can we make it a goal to get to 1329 by this time next week? That first 265 should be a lot easier than the last 265, so if we can't get through the first 265, then we don't really stand a chance. I accept this

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-03 Thread Chad
On Fri, Jun 3, 2011 at 7:45 PM, Brion Vibber br...@pobox.com wrote: On Fri, Jun 3, 2011 at 3:33 PM, Rob Lanphier ro...@wikimedia.org wrote: So, can we make it a goal to get to 1329 by this time next week?  That first 265 should be a lot easier than the last 265, so if we can't get through the

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-02 Thread Ariel T. Glenn
Στις 01-06-2011, ημέρα Τετ, και ώρα 15:58 -0600, ο/η bawolff έγραψε: On Wed, Jun 1, 2011 at 3:02 PM, Brion Vibber br...@pobox.com wrote: On Wed, Jun 1, 2011 at 1:53 PM, bawolff bawolff...@gmail.com wrote: As a volunteer person, I'm fine if code I commit is reverted based on it sucking,

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-02 Thread Robert Leverington
Thank you for posting this Erik. I initially didn't reply on this thread because to me it just seems to be the same as the 3 or 4 times we've had the same discussion before. No technical or procedural change is going to solve a problem that is ultimately caused by a lack of time assigned to it.

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-02 Thread Roan Kattouw
On Thu, Jun 2, 2011 at 9:20 AM, Robert Leverington rob...@rhl.me.uk wrote: I reject the notion that we need a change in our review processes. Empirical evidence shows that when staff time is assigned to it, our code review system *does* work.  (Of course there are always minor changes that

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-02 Thread Platonides
Chad wrote: On Wed, Jun 1, 2011 at 9:09 PM, Russell N. Nelson - rnnelson rnnel...@clarkson.edu wrote: I've done release engineering before. It is my considered opinion that one person needs to be on point for each release. It's possible that that person could rotate in and out, but one

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Ashar Voultoiz
On 31/05/11 23:13, Brion Vibber wrote: I tend to agree that a more active 'pull good fixes features in from feeder branches' model can be helpful, for instance in allowing individual modules, extensions, etc to iterate faster in a shared-development environment without forcing the changes to

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Max Semenik
On Wed, Jun 1, 2011 at 1:13 AM, Brion Vibber br...@pobox.com wrote: OMG my commit will be wasted in 20 minutes! Heeelponeone is not the best scenario for clear thinking. Time pressure might result in people reviewing stuff they're less familiar with, and reviews being done in a haste, so CR

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Happy-melon
Chad innocentkil...@gmail.com wrote in message news:banlktinrbx45wsg6yyclwpzpo7mjb3r...@mail.gmail.com... On Tue, May 31, 2011 at 7:49 PM, Happy-melon happy-me...@live.com wrote: Every way of phrasing or describing the problem with MW CR can be boiled down to one simple equation: not enough

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Roan Kattouw
On Wed, Jun 1, 2011 at 1:49 AM, Happy-melon happy-me...@live.com wrote: By far the most likely outcome of this is that in the two months following its introduction, 95% of all commits are reverted, because the people who are supposed to be reviewing them don't spend any more time than usual

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Chad
On Wed, Jun 1, 2011 at 11:23 AM, Roan Kattouw roan.katt...@gmail.com wrote: On Wed, Jun 1, 2011 at 1:49 AM, Happy-melon happy-me...@live.com wrote: By far the most likely outcome of this is that in the two months following its introduction, 95% of all commits are reverted, because the people

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Roan Kattouw
On Wed, Jun 1, 2011 at 5:28 PM, Chad innocentkil...@gmail.com wrote: I don't think revert in 72 hours if its unreviewed is a good idea. It just discourages people from contributing to areas in which we only have one reviewer looking at code. I also don't really like it. But I do think we

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Max Semenik
On 01.06.2011, 7:59 Mark wrote: The Tiki community is a heavy user of SVN. While many open source communities have a send-a-patch-and-someone-else-will-commit approach, in Tiki, we encourage everyone to commit directly to the source code. Think of it as applying the Wiki Way to software

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Max Semenik
On 01.06.2011, 20:00 Roan wrote: On Wed, Jun 1, 2011 at 5:28 PM, Chad innocentkil...@gmail.com wrote: I don't think revert in 72 hours if its unreviewed is a good idea. It just discourages people from contributing to areas in which we only have one reviewer looking at code. I also don't

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Chad
That's because our phpunit setup is incredibly fragile. I'm trying to fix it. -Chad On Jun 1, 2011 12:09 PM, Max Semenik maxsem.w...@gmail.com wrote: On 01.06.2011, 20:00 Roan wrote: On Wed, Jun 1, 2011 at 5:28 PM, Chad innocentkil...@gmail.com wrote: I don't think revert in 72 hours if its

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Rob Lanphier
On Wed, Jun 1, 2011 at 8:23 AM, Roan Kattouw roan.katt...@gmail.com wrote: This, for me, is the remaining problem with the 72-hour rule. If I happen to commit a SecurePoll change during a hackathon in Europe just as Tim leaves for the airport to go home, it's pretty much guaranteed that he

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Brion Vibber
On Wed, Jun 1, 2011 at 8:23 AM, Roan Kattouw roan.katt...@gmail.com wrote: This, for me, is the remaining problem with the 72-hour rule. If I happen to commit a SecurePoll change during a hackathon in Europe just as Tim leaves for the airport to go home, it's pretty much guaranteed that he

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Brion Vibber
On Wed, Jun 1, 2011 at 8:28 AM, Chad innocentkil...@gmail.com wrote: I *do* think we should enforce a 48hr revert if broken rule. If you can't be bothered to clean up your breakages in within 48 hours of putting your original patch in, it must not have been very important. Officially

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Andrew Garrett
On Thu, Jun 2, 2011 at 1:28 AM, Chad innocentkil...@gmail.com wrote: I don't think revert in 72 hours if its unreviewed is a good idea. It just discourages people from contributing to areas in which we only have one reviewer looking at code. I *do* think we should enforce a 48hr revert if

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Trevor Parscal
+1 I believe the way forward involves using pre-commit review, requiring test coverage to pass review, and developers working in branches at all times. SVN may be a pita when it comes to branches, but that's a solvable problem. - Trevor On Wed, Jun 1, 2011 at 10:52 AM, Andrew Garrett

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Thomas Gries
Dear friends, please - can the discussion under this subject be continued in a forum ? Tom ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Chad
On Wed, Jun 1, 2011 at 3:17 PM, Thomas Gries m...@tgries.de wrote: Dear friends, please - can the discussion under this subject be continued in a forum ? What forum? I can't think of any place more suitable than this list. -Chad ___ Wikitech-l

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Max Semenik
On 01.06.2011, 23:17 Thomas wrote: Dear friends, please - can the discussion under this subject be continued in a forum ? What forum? wikitech-l is a list designed for discussions related to MediaWiki development and Wikimedia technical stuff. Exactly what we're discussing right now.

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Aryeh Gregor
On Wed, Jun 1, 2011 at 12:38 PM, Rob Lanphier ro...@wikimedia.org wrote: The genesis of the 72-hour idea was our discussion about what is stopping us from pre-commit review.  The set of us in the office discussing this felt it was pretty much just a tools issue; from a policy point of view, we

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Brion Vibber
On Wed, Jun 1, 2011 at 12:43 PM, Aryeh Gregor simetrical+wikil...@gmail.com wrote: [snip] The point is, people's code should only be rejected with some specific reason that either a) lets them fix it and resubmit, or b) tells them definitively that it's not wanted and they shouldn't waste

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Brion Vibber
On Wed, Jun 1, 2011 at 12:43 PM, Aryeh Gregor simetrical+wikil...@gmail.com wrote: So then what happens if volunteers' contributions aren't reviewed promptly? Indeed, this is why we need to demonstrate that we can actually push code through the system on a consistent basis... until we can,

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Aryeh Gregor
On Wed, Jun 1, 2011 at 4:06 PM, Brion Vibber br...@pobox.com wrote: This isn't ready for core yet -- code looks a bit dense and we don't understand some of what it's doing yet. Can you help explain why you coded it this way and add some test cases? IMO, this is totally fine as a reason to

[Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread bawolff
The problem isn't reverting commits that are bad, it's reverting commits solely because they haven't been reviewed.  In a pre-commit review system, the equivalent to the proposed 72-hour rule would be letting unreviewed code languish without comment.  Both of these are bad things. The point

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Brion Vibber
On Wed, Jun 1, 2011 at 1:53 PM, bawolff bawolff...@gmail.com wrote: As a volunteer person, I'm fine if code I commit is reverted based on it sucking, being too complicated, being too ugly, etc provided there is actually some objection to the code. However, I'd be rather offended if it was

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Sumana Harihareswara
On Wed, Jun 1, 2011 at 3:17 PM, Thomas Gries m...@tgries.de wrote: Dear friends, please - can the discussion under this subject be continued in a forum ? Tom Tom, if you might feel more comfortable following the discussion via a web interface, try

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread bawolff
On Wed, Jun 1, 2011 at 3:02 PM, Brion Vibber br...@pobox.com wrote: On Wed, Jun 1, 2011 at 1:53 PM, bawolff bawolff...@gmail.com wrote: As a volunteer person, I'm fine if code I commit is reverted based on it sucking, being too complicated, being too ugly, etc provided there is actually some

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Platonides
Max Semenik wrote: I would wholeheartedly supoport the idea of making tests a mandatory accompaniment to code. One latest example: we made latest security releases without tests. Actually, we have made security releases that *broke* tests. (ie. tests got outdated) This is bad not only

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Brion Vibber
On Wed, Jun 1, 2011 at 4:41 PM, Happy-melon happy-me...@live.com wrote: +1. Pre-commit-review, post-commit-lifetime, branching, testing, whatever; all of the suggestions I've seen so far are IMO doomed to fail because they do not fix the underlying problem that not enough experienced manhours

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread K. Peachey
On Thu, Jun 2, 2011 at 2:28 AM, Chad innocentkil...@gmail.com wrote: That's because our phpunit setup is incredibly fragile. I'm trying to fix it. -Chad Instead of the auto tests like we have now, could we some how (might need another testing suite?) perhaps have some sort of commit hook that

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Michael Dale
On 06/01/2011 08:28 AM, Chad wrote: I don't think revert in 72 hours if its unreviewed is a good idea. It just discourages people from contributing to areas in which we only have one reviewer looking at code. I *do* think we should enforce a 48hr revert if broken rule. If you can't be

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Aryeh Gregor
On Wed, Jun 1, 2011 at 5:02 PM, Brion Vibber br...@pobox.com wrote: When someone looks at your commit within ~72 hours and reverts it because nobody's yet managed to figure out whether it works or not and it needs more research and investigation... what was the reason for the revert? Because

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Erik Moeller
On Wed, Jun 1, 2011 at 4:50 PM, Brion Vibber br...@pobox.com wrote: This is one of the reasons I tend to advocate shorter development/review/deployment cycles. By keeping the cycle short, we can help build up regular habits: run through some reviews every couple days. Do a deployment update

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Russell N. Nelson - rnnelson
I've done release engineering before. It is my considered opinion that one person needs to be on point for each release. It's possible that that person could rotate in and out, but one person needs to be running through the checklist of things that need to be done.

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-06-01 Thread Chad
On Wed, Jun 1, 2011 at 8:31 PM, Michael Dale md...@wikimedia.org wrote: On 06/01/2011 08:28 AM, Chad wrote: I don't think revert in 72 hours if its unreviewed is a good idea. It just discourages people from contributing to areas in which we only have one reviewer looking at code. I *do*

[Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-05-31 Thread Rob Lanphier
On Tue, May 31, 2011 at 10:35 AM, Neil Kandalgaonkar ne...@wikimedia.org wrote: Are we all in deadlock or something? Are the users who can push waiting from some proposals/work from the rest of the community? We had a hallway conversation about this just now (Neil, Trevor, Brion and I, and then

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-05-31 Thread Russell N. Nelson - rnnelson
Robla writes: 1. We say that a commit has some fixed window (e.g. 72 hours) to get reviewed, or else it is subject to automatic reversion. This will motivate committers to make sure they have a reviewer lined up, and make it clear that, if their code gets reverted, it's nothing personal...it's

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-05-31 Thread Trevor Parscal
The obvious problem with the 72 hour time limit is that it imposes unnecessary time limits, adding lots of to the complexity, chaos and stress - possibly a net-negative, but it's hard to say for sure. That said, at least it's an actionable idea. It's pretty clear to me that the inconvenience

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-05-31 Thread Krinkle
Rob Lanphier wrote: On Tue, May 31, 2011 at 10:35 AM, Neil Kandalgaonkar ne...@wikimedia.org wrote: Are we all in deadlock or something? snip independent suggestions, but any of these might help: 1. We say that a commit has some fixed window (e.g. 72 hours) to get reviewed, or else it is

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-05-31 Thread Brion Vibber
On Tue, May 31, 2011 at 12:09 PM, Russell N. Nelson - rnnelson rnnel...@clarkson.edu wrote: Robla writes: 1. We say that a commit has some fixed window (e.g. 72 hours) to get reviewed, or else it is subject to automatic reversion. This will motivate committers to make sure they have a

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-05-31 Thread Brion Vibber
On Tue, May 31, 2011 at 12:37 PM, Max Semenik maxsem.w...@gmail.com wrote: On 31.05.2011, 22:41 Rob wrote: So, there's a number of possible solutions to this problem. These are independent suggestions, but any of these might help: 1. We say that a commit has some fixed window (e.g. 72

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-05-31 Thread MZMcBride
Brion Vibber wrote: Ultimately, we need to remember what reverting a commit means: ** Reverting means we're playing it safe: we are taking a machine in unknown condition out of the line of fire and replacing it with a machine in known-good condition, so the unknown one can be reexamined

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-05-31 Thread Happy-melon
Russell N. Nelson - rnnelson rnnel...@clarkson.edu wrote in message news:777bc8a5a78cc048821dbfbf13a25d39208f1...@exch01.ad.clarkson.edu... Robla writes: 1. We say that a commit has some fixed window (e.g. 72 hours) to get reviewed, or else it is subject to automatic reversion. This will

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-05-31 Thread Chad
On Tue, May 31, 2011 at 7:49 PM, Happy-melon happy-me...@live.com wrote: Every way of phrasing or describing the problem with MW CR can be boiled down to one simple equation: not enough qualified people are not spending enough time doing Code Review (until a mad rush before release) to match

Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)

2011-05-31 Thread Mark Dilley
The Tiki community is a heavy user of SVN. While many open source communities have a send-a-patch-and-someone-else-will-commit approach, in Tiki, we encourage everyone to commit directly to the source code. Think of it as applying the Wiki Way to software development. Over 220 people have done