Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)
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 about how we can make this real. In this discussion, the participants were the Engineering Program Managers, Sumana and me. If you want the play-by-play, you can see Sumana's notes here: http://etherpad.wikimedia.org/Dev-Process-Improvement The high level summary is that within the management group, there's consensus that this is a good idea, and that we need to make it real ASAP, principally within the parameters of the aforementioned policy page. The practical barriers are that we 1) need to ensure that we're not adding to people's existing massive workload without taking something off, in order to do this, 2) we need to plan for skills development related to code review and deployment. With regard to 1), managers are beginning conversations with their reports with regard to project allocation, and we're working towards getting every full-time engineer freed up ASAP to make that 20% commitment. With regard to 2), RobLa and Sumana are starting to think about a code review/deployment boot camp (more WMF-focused than a hackathon, but ideally with some options for remote participation) -- again, see the notes for what's been discussed so far; Rob's going to talk more about this in a couple of weeks or so (he's off next week). All best, Erik -- Erik Möller Deputy Director, Wikimedia Foundation Support Free Knowledge: http://wikimediafoundation.org/wiki/Donate ___ 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)
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 :) . For numerous reasons, I think getting /everyone/ involved with service work is a great idea. 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 Please be bold and/or add your thoughts. As per the note on the page and other comments in this thread, a policy like this isn't a replacement for dedicated staff supporting the code review and release process; it's merely one component of ensuring appropriate capacity is available for relevant work. -- Erik Möller Deputy Director, Wikimedia Foundation Support Free Knowledge: http://wikimediafoundation.org/wiki/Donate ___ 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)
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 that's even better :) . For numerous reasons, I think getting /everyone/ involved with service work is a great idea. 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 Please be bold and/or add your thoughts. As per the note on the page and other comments in this thread, a policy like this isn't a replacement for dedicated staff supporting the code review and release process; it's merely one component of ensuring appropriate capacity is available for relevant work. This looks great! Both your post and Brion's yesterday have been spot-on. I think with a few people doing shell bugs every week (as an example), the backlog will be killed in no time. Shell bugs are generally some of the most user-facing issues, so reducing time between filing and fulfillment on those will go a long way toward keeping users happy. There are also structural improvements that can be made (such as getting a Configure extension working on Wikimedia wikis) that could eliminate the need for sysadmin intervention a lot of the time. I'm not sure if working on projects that like would fall within the 20% rule as written, but it's something to think about. The only other point from your list that I think maybe could possibly be made explicit is a focus on the sister projects. It's pretty clear that during the 80%, Wikipedia is the central focus. I've been doing some work on Wikisource lately and without looking very hard, it's very easy to see how some support structures on the sister sites (API support, rewrites of some of these extensions, moving code from JavaScript to PHP, etc.) could really enable third-party developers. Overall, this seems like a really positive step in the right direction. :-) MZMcBride ___ 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)
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 to do is to help triage the 1.17 blockers: https://bugzilla.wikimedia.org/show_bug.cgi?id=26676 It appears a few more were added to the list over the weekend, including bug 24415 (tracking bug for Resource Loader 1.0), for which there are many blockers. If these bugs are all truly blockers, then we'll have to pull people in from code review to fix those bugs. If we can wait until 1.18 or later, then we'll have the luxury of holding out for volunteer help. Rob ___ 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)
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 zero by July 15, we have to review 265 revisions/week. That means: 2011-Jun-03 1594 2011-Jun-10 1329 2011-Jun-17 1064 2011-Jun-24 799 2011-Jul-01 534 2011-Jul-08 269 2011-Jul-15 4 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. Thanks for posting this. This seems pretty reasonable. 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. :-) MZMcBride ___ 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)
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 especially frustrating for volunteers and third party committers. We kicked around the idea of a 20% rule for all funded engineers (IMO not just senior staff) in Berlin and in the office yesterday, and I think Roan mentioned it earlier in this thread: i.e. ensuring that every WMF-funded engineer spends one day a week on service work (code review, design/UX review, deployment, shell bugs, software bugs, bug triaging, etc.). Expect to hear more from the other EPMs on this subject, but this is a no-brainer for General Engineering to get on board with (we're already pretty much doing this). 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 zero by July 15, we have to review 265 revisions/week. That means: 2011-Jun-03 1594 2011-Jun-10 1329 2011-Jun-17 1064 2011-Jun-24 799 2011-Jul-01 534 2011-Jul-08 269 2011-Jul-15 4 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. Rob ___ 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)
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 challenge! My 20% is committed for next week... are y'all with us? :D -- brion ___ 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)
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 first 265, then we don't really stand a chance. I accept this challenge! My 20% is committed for next week... are y'all with us? :D Which day? I'll pencil it in as well :) -Chad ___ 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)
Στις 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, 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 reverted on the basis of no one got around to looking at in the last 3 days, since that is something essentially out of my control. 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 'no one reviewed it'? Or because someone looked at it and decided it needed more careful review than they could give yet? -- brion That's fine. I'm only concerned with the possibility of no one looks at the commit, and it gets summarily reverted (Which is what I thought was being proposed). --bawolff One question is how thorough is thorough: if the code is reviewed for security issues and doesn't apparently break things, does it need to get gone over with a fine toothed comb before we accept it for deployment? I would say no, and that we could live with the commit adheres to our basic coding guidelines, looks like it will generally do what it says (without necessarily having tested every detail personally), and follows general usage patterns (appropriate use of hooks, cacheing, i18n, etc. etc.). Can we ensure that folks have (now and ongoing) enough available time to review commits to trunk at that level within 72 hours? This means buy-in from reviewers *and from their managers* that they have X time for this and that time will not get reassigned to other priorities. If we can't ensure this, we need to decide on the minimum level of review we *can* get done, or extend the time period, or both. Asking all committers to trunk to guess whether their reviewer can or cannot make the 72-hour window for their commit seems like a recipe for pissing people off. Just my two cents. Ariel P.S. Oh and for the record I would like 4 releases a year ideally, I could live with 3, and I would be pretty unhappy with two. Deployment from trunk should happen a lot more often. I don't know if we can manage weekly, but it's a good target. ___ 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)
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. I was somewhat disappointed by the suggestion that the onus should be on volunteers to find a reviewer for their code, as this is just as unworkable without staff time assigned to doing it - and perhaps could be even more demotivating than the status quo. Having review be a portion of staff time was always how I assumed it would be arranged. The justification for not assigning time to review has often been that one person or team doing it all the time would result in burn-out, and I agree with this. As a (part-time) developer I certainly wouldn't want to spend all my time doing service work, even for just one or two weeks at a time. The idea of having a single person/team assigned for each deploy/release cycle is nice, but I think that it would lead to burn-outs and conflicts with existing responsibilities those people have. Whatever model is implemented, I hope it can finally be successful. 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 would improve efficiency, such as automating some of the reporting tasks that have traditionally been done manually.) Updates on how this is going would be nice, as again it took MZMcBride to bring it up before anyone senior posted about it - and this is still the big issue for volunteer developers. Cheers, Robert On 2011-06-01, Erik Moeller wrote: 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 *every* week. If you don't think your code will be working within that time, either work on it outside of trunk or break it up into pieces that won't interfere with other code. With a long cycle, review gets pushed aside until it's no longer habit, and gets lost. 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 especially frustrating for volunteers and third party committers. We kicked around the idea of a 20% rule for all funded engineers (IMO not just senior staff) in Berlin and in the office yesterday, and I think Roan mentioned it earlier in this thread: i.e. ensuring that every WMF-funded engineer spends one day a week on service work (code review, design/UX review, deployment, shell bugs, software bugs, bug triaging, etc.). An alternative model is to have rotating teams that do this work. I personally prefer the 20% model because it gives more consistency/predictability and less churn, but I'm curious what other folks think, and I'm comfortable with us continuing this discussion openly on this list. Whether that would get us to a healthier balance remains to be seen, but I think there's pretty broad agreement that adding more support to the review/deployment/release process is a necessary precondition for any other process changes like moving towards pre-commit review. Clearly what's been said in this thread is true -- there are lots of things that can be done to reduce our technical debt and make it easier to accommodate and manage new changes, but without added dedicated capacity, the train won't move at a speed that we're happy with. We can't change that overnight (because we need to figure out the right rhythm and the right processes together), but we will change it. Erik -- Erik Möller Deputy Director, Wikimedia Foundation Support Free Knowledge: http://wikimediafoundation.org/wiki/Donate ___ 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
Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)
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 would improve efficiency, such as automating some of the reporting tasks that have traditionally been done manually.) I agree that it's worth trying to add staff time without materially changing the process, see how that works, then change things as needed. Updates on how this is going would be nice, as again it took MZMcBride to bring it up before anyone senior posted about it - and this is still the big issue for volunteer developers. FWIW, I was planning to bring this up at the staff meeting following the Berlin hackathon. I said to a number of people that I wanted to have a discussion about code review, both before and during the event. This didn't end up happening, partly because some felt that people that weren't around should be involved, or that the entire discussion should be had in public, and partly because we were doing all sorts of other things. I think MZ knew about my plans and the lack of a post-conference update saying here's the outcome of our discussion, what do you think may have driven him to bring this up. I care about this issue, and I agree with MZ on most of the basics, but I'm also a busy guy who took some time off after the conference to focus on school. So having said the obligatory but I care too! thing, you are right that it seems, at least to people outside the office (both staff and non-staff: apparently there are lots of hallway discussions about CR these days, but they happen thousands of miles from my desk so working for WMF doesn't help me there), that there isn't a lot of attention given to this. Whether that's actually the case or just the impression that's being created due to lack of communication doesn't matter all that much; we can and should fix it. Roan Kattouw (Catrope) ___ 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)
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 person needs to be running through the checklist of things that need to be done. I've mentioned on several occasions in the past that we should try this. If each branch (REL1_17, 1.17wmf1, REL1_18) had someone to babysit it, it would lead to less turnaround for things to get merged to various branches because someone would be the go-to person for getting a trunk change into the branch in question. +1 ___ 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)
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 trunk/mainlines's production deployment release channels until they're ready. What prevents us to actually implements this? We could just restrict the /trunk/ to a few merging people, they will only take care of merging revisions set from other branches. Developers then build their fixes / features in their own branches and get it reviewed / signed-off by others. Once done, it is submitted to the merge team. This is much like the REL branches which only a few people are able to merge trunk changes in. Just with another layer :-) The VCS would be up to the developer (thus solving the migrate from svn to git issue in the meantime). Subversion will only be used to actually merge patch sets in trunk or REL branches. But these need to be combined with really active, consistent management; just like keeping the deployment release branches clean, working, and up to date consistently requires a lot of attention -- when attention gets distracted, the conveyor belt falls apart and fixes features stop reaching the public until the next giant whole-release push, which turns into a circus. Then we need merge windows. A date by which everyone would have to make sure their patch is ready: tested heavily, reviewed by people actually knowing the part of code impacted, test written, style fixed, i18n ... If the patch does not make it in the current merge windows, it will apply for the next one. -- Ashar Voultoiz ___ 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)
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 quality will suffer. I'd say better to do that continuously and confirm that things are being reviewed *and* run on automated tests *and* run against humans on actual-production-use beta servers *and* in actual production use within a reasonable time frame, than to let review slide for 10 months and rush it all right before a giant release. That IMO is what really hurts code review -- removing it from the context of code *creation*, and removing the iterative loop between writing, debugging, testing, debugging, deploying, debugging, fixing, and debugging code. I would wholeheartedly supoport the idea of making tests a mandatory accompaniment to code. One latest example: we made latest security releases without tests. This is bad not only because bugs not tested for *will* eventually reappear, but also because in my experience the process of writing tests helps to discover more problems with code. If compared with manual tests, writing low-level unit tests allows to look at possible problems more methodicallly, uncovering more problems. Ideally, I would like to see the same policy as Chome has: 1) All reasonably testable code must be accompanied with automatic tests, period. (It's easier to enforce for them because they practice pre-commit reviews.) 2) To commit, hang around on IRC. After committing, wait for buildbot to post test results of your commit. If something's broken, fix it ASAP, otherwise you'll be reverted. To make this reasonable, we would also need to run tests for branches on the central server, too - CruiseControl doesn't look flexible enough for this. ___ 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)
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 qualified people are not spending enough time doing Code Review (until a mad rush before release) to match the amount of code being committed. Maybe people shouldn't commit untested code so often. I'm not joking. -Chad That's a worthy goal, but one that's orthogonal to Code Review. Every single person on this list will have committed some unreviewed code to some repository at some time; the fewer times you've done it, the more likely you are to have crashed the cluster the times you did. People doing some unquantifiably greater amount of testing doesn't mean we can spend any less time on CR per revision. Automated testing, regression testing, other well-defined infrastructures (I think Ryan's Nova Stack is going to be of huge benefit in this respect) *do* save CR time *because reviewers know exactly what has been tested*. A policy like every bugfix must include regression tests would definitely improve things in that area. Of course, it's undeniable that more testing would lead to fewer broken commits, and that that's a Good Thing. If we implement processes which set a higher bar for commits 'sticking' in the repository, whether that's pre-commit review, a branch/integrate model, post-commit countdown, whatever; people will rise to that level. --HM ___ 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)
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 reviewing them. If I make a change to the preprocessor, HipHop, Sanitizer, SecurePoll, passwords, tokens, or any of a number of other things, I'm going to need Tim to review them. I don't begrudge Tim the thousand-and-one other things he has to do besides review my code within 72 hours. Does that mean that no one should make *any* changes to *any* of those areas? More dangerously still, does that mean that **only people who can persuade Tim to review** be allowed to make changes to those areas? I know what the answers to those questions are *supposed* to be, that's not the point. The point is **what actually happens**. 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 won't be able to look at my commit within 72 hours. Or maybe I'm committing an UploadWizard change just after 5pm PDT on Friday, and the next Monday is a federal holiday like the 4th of July. Or maybe the one reviewer for a piece of code has just gone on vacation for a week and we're all screwed. So yes, before we implement this we need to 1) ensure that reviewers spend enough time reviewing and 2) figure out what to do with one-reviewer situations. Roan Kattouw (Catrope) ___ 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)
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 who are supposed to be reviewing them don't spend any more time than usual reviewing them. If I make a change to the preprocessor, HipHop, Sanitizer, SecurePoll, passwords, tokens, or any of a number of other things, I'm going to need Tim to review them. I don't begrudge Tim the thousand-and-one other things he has to do besides review my code within 72 hours. Does that mean that no one should make *any* changes to *any* of those areas? More dangerously still, does that mean that **only people who can persuade Tim to review** be allowed to make changes to those areas? I know what the answers to those questions are *supposed* to be, that's not the point. The point is **what actually happens**. 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 won't be able to look at my commit within 72 hours. Or maybe I'm committing an UploadWizard change just after 5pm PDT on Friday, and the next Monday is a federal holiday like the 4th of July. Or maybe the one reviewer for a piece of code has just gone on vacation for a week and we're all screwed. So yes, before we implement this we need to 1) ensure that reviewers spend enough time reviewing and 2) figure out what to do with one-reviewer situations. 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 bothered to clean up your breakages in within 48 hours of putting your original patch in, it must not have been very important. -Chad ___ 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)
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 should be more liberal about reverting things that won't be reviewed soon because they're e.g. too large and should be branched or broken up, or whatever. 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. +1. Serious breakage should be reverted on sight, however. Roan Kattouw (Catrope) ___ 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)
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 development. Over 220 people have done it so far and it works very well. http://dev.tiki.org/How+to+get+commit+access I don't know if this is feasible in this community - but wanted to put it out is as a possible direction. Our commit access approval is quite liberal already, however it's not completely instantaneous - people still need to contribute a few patches before being allowed to work on core, due to the importance of security in MediaWiki as it's being used on a top 5 website. We already have 315 commiters (a few of accounts are dupes though). The problem is not about attracting more contributors, it's about reviewing and deploying their code. -- Best regards, Max Semenik ([[User:MaxSem]]) ___ 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)
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 really like it. But I do think we should be more liberal about reverting things that won't be reviewed soon because they're e.g. too large and should be branched or broken up, or whatever. 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. +1. Serious breakage should be reverted on sight, however. +1. Serious should include breaking the tests though - otherwise we'll remain with our current situation when tests are broken in multiple places and nobody knows who broke what: [19:39:08] codurr Something broke. See http://ci.tesla.usability.wikimedia.org/cruisecontrol/buildresults/mw. Possible culprits: aashrh/r89027 /r89028 /r89029 nbiabriket/r89035 krenkli/r89036 eryed/r89037 /r89038 eixal/r89039 /r89040 /r89041 /r89043 /r89044 /r89047 /r89049 /r89051 /r89061 /r89062 /r89063 /r89070 /r89071 /r89072 yaor_mdn/r89074 /r89075 /r89076 osnenl/r89079 /r89082 /r89083 /r89084 /r89085 /r89086 /r89087 eom-ylnphap/r89088 jedht/r89094 /r89099 /r89 [19:39:08] codurr /r89108 /r89110 /r89111 /r89112 /r89113 /r89114 /r89115 /r89116 /r89117 /r89118 /r89119 iwkiunta/r89120 /r89122 /r89123 onraa/r89128 /r89129 /r89134 /r89138 ^nmdoe/r89144 /r89145 /r89149 /r89150 algsinrtt/r89166 /r89176 /r89179 /r89180 sxemma/r89181 /r89182 /r89186 ptolsaiedn/r89191 /r89197 /r89204 /r89205 /r89206 /r89207 /r89208 /r89218 wadreoujdene/r89219 /r89223 /r89224 rtemo/r89225 /r89226 /r89227 /r89228 /r89230 /r89241 oribn/r89243 /r89244 /r89 [19:39:08] codurr kfoelokwrays/r89250 /r89251 /r89253 /r89254 awnurotkaot/r89258 /r89260 rnabsedi/r89261 /r89262 /r89263 -- Best regards, Max Semenik ([[User:MaxSem]]) ___ 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)
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 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 should be more liberal about reverting things that won't be reviewed soon because they're e.g. too large and should be branched or broken up, or whatever. 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. +1. Serious breakage should be reverted on sight, however. +1. Serious should include breaking the tests though - otherwise we'll remain with our current situation when tests are broken in multiple places and nobody knows who broke what: [19:39:08] codurr Something broke. See http://ci.tesla.usability.wikimedia.org/cruisecontrol/buildresults/mw. Possible culprits: aashrh/r89027 /r89028 /r89029 nbiabriket/r89035 krenkli/r89036 eryed/r89037 /r89038 eixal/r89039 /r89040 /r89041 /r89043 /r89044 /r89047 /r89049 /r89051 /r89061 /r89062 /r89063 /r89070 /r89071 /r89072 yaor_mdn/r89074 /r89075 /r89076 osnenl/r89079 /r89082 /r89083 /r89084 /r89085 /r89086 /r89087 eom-ylnphap/r89088 jedht/r89094 /r89099 /r89 [19:39:08] codurr /r89108 /r89110 /r89111 /r89112 /r89113 /r89114 /r89115 /r89116 /r89117 /r89118 /r89119 iwkiunta/r89120 /r89122 /r89123 onraa/r89128 /r89129 /r89134 /r89138 ^nmdoe/r89144 /r89145 /r89149 /r89150 algsinrtt/r89166 /r89176 /r89179 /r89180 sxemma/r89181 /r89182 /r89186 ptolsaiedn/r89191 /r89197 /r89204 /r89205 /r89206 /r89207 /r89208 /r89218 wadreoujdene/r89219 /r89223 /r89224 rtemo/r89225 /r89226 /r89227 /r89228 /r89230 /r89241 oribn/r89243 /r89244 /r89 [19:39:08] codurr kfoelokwrays/r89250 /r89251 /r89253 /r89254 awnurotkaot/r89258 /r89260 rnabsedi/r89261 /r89262 /r89263 -- Best regards, Max Semenik ([[User:MaxSem]]) ___ 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
Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)
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 won't be able to look at my commit within 72 hours. Or maybe I'm committing an UploadWizard change just after 5pm PDT on Friday, and the next Monday is a federal holiday like the 4th of July. Or maybe the one reviewer for a piece of code has just gone on vacation for a week and we're all screwed. We could pick a different window, but I don't see how it can be much longer if we're also pursuing much more frequent deployments. If we're deploying every week, and something hasn't been reviewed at deploy time, the reviewer can either rush a review of code they may or may not really understand, or revert. I think using the word screwed here belies the fundamental social problem we have with respect to reverts. As Brion has pointed out over and over, being reverted is not the end of the world. It doesn't mean this code must die, it means this code isn't ready for trunk yet. It still is in the repository and is available for comment and debate, and recommitting the code can happen a week or two later. 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 think it makes sense. Given that the 72-hour rule is less severe than pre-commit review, do you believe that our original premise is flawed (i.e. requiring pre-commit review is a bad idea)? Rob ___ 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)
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 won't be able to look at my commit within 72 hours. Or maybe I'm committing an UploadWizard change just after 5pm PDT on Friday, and the next Monday is a federal holiday like the 4th of July. Or maybe the one reviewer for a piece of code has just gone on vacation for a week and we're all screwed. I think rather than us being all screwed, this is a situation where we *don't actually have to* commit that change to trunk right at that exact time. Unless the change needs to be deployed immediately -- in which case you really should have someone else looking it over to review it IMMEDIATELY -- it can probably wait until reviewers are available. And if it does get reverted before the reviewer gets to it -- what's wrong with that? It'll get reviewed and go back in later. Part of the reason we went so hog-wild with handing out commit access for a long time is because we've never *been* as good as we want about following up with patches submitting through the bug trackers and mailing lists; sometimes things get reviewed and pushed through, but often they just get forgotten. Letting people commit directly meant that things didn't get forgotten -- they were right there in the code now -- with the caveat that if there turned out to be problems, we would have to go through and take them back out. It's a trade-off we chose to make at the time, and it's a trade-off we can revisit if we change the underlying conditions. -- brion ___ 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)
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 speaking, since forever we have had an it's acceptable to revert on sight if broken rule. If you encounter something broken, correct responses include: * revert it immediately to a known good state; send an explanation to the author about what's wrong (usually a CR comment and reopening a 'fixed' bugzilla entry) * figure out the problem and fix it; send follow-up explanation to the author (usually a CR comment and a Bugzilla comment if it's a followup on a bug fix) * take a stab at the problem and recommend a solution to someone else who will fix it straight away; including a note on CR or Bugzilla summarizing your IRC discussion is wise in case it ends up getting dropped * tag it fixme and ask someone more familiar with the stuff than you to handle it -- brion ___ 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)
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 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. I'm really not a fan of drop-dead deadlines in the one to two day range in general. I occasionally have periods of about two or three days when I don't have access to a computer (or time to use one). I think that if we actually want pre-commit review (which I don't have a problem with), we should have pre-commit review instead of excessive reversion. Reverts make the revision log hard to follow, feel like a slap in the face to many developers (especially new ones, who this policy is supposed to be attracting!), and of course give us lots of merge conflicts and what not. I think it would combine with commits of code that's broken in the first place to exacerbate the current situation where a single change can have up to ten associated revisions where people fix little things, revert, unrevert, and generally make things difficult to review and follow. -- Andrew Garrett Wikimedia Foundation agarr...@wikimedia.org ___ 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)
+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 agarr...@wikimedia.orgwrote: 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 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. I'm really not a fan of drop-dead deadlines in the one to two day range in general. I occasionally have periods of about two or three days when I don't have access to a computer (or time to use one). I think that if we actually want pre-commit review (which I don't have a problem with), we should have pre-commit review instead of excessive reversion. Reverts make the revision log hard to follow, feel like a slap in the face to many developers (especially new ones, who this policy is supposed to be attracting!), and of course give us lots of merge conflicts and what not. I think it would combine with commits of code that's broken in the first place to exacerbate the current situation where a single change can have up to ten associated revisions where people fix little things, revert, unrevert, and generally make things difficult to review and follow. -- Andrew Garrett Wikimedia Foundation agarr...@wikimedia.org ___ 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
Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)
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)
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 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)
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. -- Best regards, Max Semenik ([[User:MaxSem]]) ___ 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)
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 think it makes sense. Given that the 72-hour rule is less severe than pre-commit review, do you believe that our original premise is flawed (i.e. requiring pre-commit review is a bad idea)? 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 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 further effort or hope on the project. Whether you're using review-then-commit or commit-then-review, one of the most demoralizing things you can do to a volunteer's contributions is say nobody cares enough to provide any feedback on your code, so we're just going to reject it by default. Of course, all this requires time spent reviewing. It used to be that we had enough time in practice, even though volunteers greatly outnumbered paid staff. Now we seem to have a comparable number of paid staff and volunteers, so it seems clear that Wikimedia could find enough time if it wanted. But apparently Wikimedia's tech leadership feels it's more important to work on projects Wikimedia has a specific interest in, than to ensure that volunteer code is reviewed and deployed speedily and reliably. So the latter doesn't happen. On Wed, Jun 1, 2011 at 2:01 PM, Trevor Parscal tpars...@wikimedia.org wrote: 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. So then what happens if volunteers' contributions aren't reviewed promptly? In commit-then-review, they sit in trunk for months, by which point they're practically impossible to revert, so someone has to review them no matter what. In review-then-commit, they just never get committed, so they bitrot and are never used. The latter is incomparably more damaging to volunteers' willingness to participate. ___ 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)
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 further effort or hope on the project. [snip] 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? It's usually better if you can do this at a pre-commit stage: when reviewing a patch on bugzilla, when looking over a merge request from a branch, etc. But it's ok to pull something back out of trunk to be reworked, too. -- brion ___ 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)
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, nobody seems willing to trust pre-commit review. -- brion ___ 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)
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 reject a commit. It gives clear reasons why it's being rejected and suggests how to fix them. Another good reason, which I've given in the past to at least one thing I was asked to review on Bugzilla, is This is doing too many things at once -- break it up into a bunch of smaller patches so I can review them individually. But just letting the patches languish with no reason is really bad, and reverting them with no reason once they were already committed is psychologically even worse (even though practically it's similar). ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
[Wikitech-l] Code review process (was: Status of more regular code deployments)
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 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 further effort or hope on the project. Whether you're using review-then-commit or commit-then-review, one of the most demoralizing things you can do to a volunteer's contributions is say nobody cares enough to provide any feedback on your code, so we're just going to reject it by default. +1 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 reverted on the basis of no one got around to looking at in the last 3 days, since that is something essentially out of my control. As it stands, trivial one line changes aren't even reviewed in 72 hours. -bawolff ___ 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)
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 reverted on the basis of no one got around to looking at in the last 3 days, since that is something essentially out of my control. 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 'no one reviewed it'? Or because someone looked at it and decided it needed more careful review than they could give yet? -- brion ___ 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)
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 news://news.gmane.org/gmane.science.linguistics.wikipedia.technical or http://lists.wikimedia.org/pipermail/wikitech-l/2011-June/thread.html -- Sumana Harihareswara Volunteer Development Coordinator Wikimedia Foundation ___ 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)
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 objection to the code. However, I'd be rather offended if it was reverted on the basis of no one got around to looking at in the last 3 days, since that is something essentially out of my control. 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 'no one reviewed it'? Or because someone looked at it and decided it needed more careful review than they could give yet? -- brion That's fine. I'm only concerned with the possibility of no one looks at the commit, and it gets summarily reverted (Which is what I thought was being proposed). --bawolff ___ 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)
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 because bugs not tested for *will* eventually reappear, but also because in my experience the process of writing tests helps to discover more problems with code. If compared with manual tests, writing low-level unit tests allows to look at possible problems more methodicallly, uncovering more problems. (...) The problem is that we don't have a proper architecture for testing everything. For instance, I would have liked to add a test for 'stub color in contribution lists' when fixing it. But we have no tests able to check that. Perhaps we could track it with Selenium or storing the html output files, but we would need a more advanced testing fake db. ___ 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)
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 are being dedicated to Code Review for the amount of work (not the 'number of commits', the amount of *energy* to make changes to code) in the system. A pre-commit-review system doesn't reduce the amount of work needed to get a feature into deployment, it just changes the nature of the process. 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 *every* week. If you don't think your code will be working within that time, either work on it outside of trunk or break it up into pieces that won't interfere with other code. With a long cycle, review gets pushed aside until it's no longer habit, and gets lost. -- brion ___ 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)
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 tells the testing server a new commit has been entered to do a test against it up to that point compared to the guessing what commit between two points did it, although if there is multiple sets of tests going due to the closeness of commits, they may all fail but it would be easier to track back to the first one that failed. On Thu, Jun 2, 2011 at 4:01 AM, Trevor Parscal tpars...@wikimedia.org wrote: 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 Wouldn't working in branches just hide the mess and move it else where?, the same problems would still arise and could potentially increase workload (eg: if someone finds a bug and works on fixing it whilst someone else has noticed the same thing and worked on it as well but not checked to see if anyone else has). ___ 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)
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 bothered to clean up your breakages in within 48 hours of putting your original patch in, it must not have been very important. -Chad I think a revert on sight, if broken is fair ... you can always re-add it in after you fix it ... if its a 'works diffrently than expected' type issue / not perfectly following coding conventions a 48hr window to make progress ( during the work week ) sounds reasonable. --michael ___ 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)
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 'no one reviewed it'? Or because someone looked at it and decided it needed more careful review than they could give yet? Yet is the keyword here. Currently the only guarantee anyone has that their code will be reviewed at all is that it's in trunk, and historically releases and Wikimedia deployments have always been based on trunk, so someone's got to either review or revert it at some point, and it's eventually easier to review than revert once enough changes have accumulated on top of it. If a 72-hour revert rule were instituted with no additional policy changes, there's no reason to believe any of the reverted commits would ever be reviewed. And if it is reviewed weeks or months later, you've got to hope it hasn't bitrotted, which is a nonissue if it was on trunk the whole time. Plus it gets testing, if it's on trunk, so the longer it's been there, the more certain the reviewer can be that it doesn't cause serious issues. If the policy worked out to be 95% of volunteer commits get reviewed within three days, and the rest get temporarily reverted but put on a list where someone gets to them within a week or two -- that would be fine. Heck, if 95% of volunteer commits got reviewed in three days and the rest got reverted never to see the light of day again, that would probably be a big improvement over the status quo. But *currently*, what percentage of volunteer commits get reviewed within three days? ___ 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)
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 *every* week. If you don't think your code will be working within that time, either work on it outside of trunk or break it up into pieces that won't interfere with other code. With a long cycle, review gets pushed aside until it's no longer habit, and gets lost. 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 especially frustrating for volunteers and third party committers. We kicked around the idea of a 20% rule for all funded engineers (IMO not just senior staff) in Berlin and in the office yesterday, and I think Roan mentioned it earlier in this thread: i.e. ensuring that every WMF-funded engineer spends one day a week on service work (code review, design/UX review, deployment, shell bugs, software bugs, bug triaging, etc.). An alternative model is to have rotating teams that do this work. I personally prefer the 20% model because it gives more consistency/predictability and less churn, but I'm curious what other folks think, and I'm comfortable with us continuing this discussion openly on this list. Whether that would get us to a healthier balance remains to be seen, but I think there's pretty broad agreement that adding more support to the review/deployment/release process is a necessary precondition for any other process changes like moving towards pre-commit review. Clearly what's been said in this thread is true -- there are lots of things that can be done to reduce our technical debt and make it easier to accommodate and manage new changes, but without added dedicated capacity, the train won't move at a speed that we're happy with. We can't change that overnight (because we need to figure out the right rhythm and the right processes together), but we will change it. Erik -- Erik Möller Deputy Director, Wikimedia Foundation Support Free Knowledge: http://wikimediafoundation.org/wiki/Donate ___ 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)
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. From: wikitech-l-boun...@lists.wikimedia.org [wikitech-l-boun...@lists.wikimedia.org] on behalf of Erik Moeller [e...@wikimedia.org] Whether that would get us to a healthier balance remains to be seen, but I think there's pretty broad agreement that adding more support to the review/deployment/release process is a necessary precondition for any other process changes like moving towards pre-commit review. Clearly what's been said in this thread is true -- there are lots of things that can be done to reduce our technical debt and make it easier to accommodate and manage new changes, but without added dedicated capacity, the train won't move at a speed that we're happy with. We can't change that overnight (because we need to figure out the right rhythm and the right processes together), but we will change it. Erik -- Erik Möller Deputy Director, Wikimedia Foundation Support Free Knowledge: http://wikimediafoundation.org/wiki/Donate ___ 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
Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)
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* 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. -Chad I think a revert on sight, if broken is fair ... you can always re-add it in after you fix it ... if its a 'works diffrently than expected' type issue / not perfectly following coding conventions a 48hr window to make progress ( during the work week ) sounds reasonable. As long as we can agree that committing big things on a Friday is bad, because you might leave trunk broken all weekend :) -Chad ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
[Wikitech-l] Code review process (was: Status of more regular code deployments)
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 just Brion and I), which I think was pretty useful. Here's where we went with it: 1. We rehashed the pre-commit review proposal that Neil suggested a few months ago, and agreed that pre-commit would be helpful in keeping the backlog down 2. Given our current tools/process, we agreed that insisting on pre-commit review would be a pain in the butt. 3. Brion and I further discussed review process, trying to come up with a system that give us the benefits of pre-commit review, without actually switching to pre-commit review Here's where things I think got interesting. Brion pointed out that in ye olden days, he was much more aggressive about reverting things he didn't understand. I pointed out that, as we broaden the pool of committers, I don't understand-based reversions lead to a lot of ugliness, since very few people claim to have a broad understanding of the system and therefore an expectation of understanding every change. Most reviewers, faced with a commit they don't understand, will leave it for others to comment on. There's been a lot of unnecessary drama and churn over reversions because of misunderstandings about what a reversion means. 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 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 just our process. 2. We encourage committers to identify who will be reviewing their code as part of their commit comment. That way, we have an identified person who has license to revert if they don't understand the code. I coulda swore there are other ideas that came out of that conversation, but alas, I wasn't taking notes. Anyway, I'm sure they'll come up in this thread. Rob ___ 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)
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 just our process. Worse than pre-commit review. What if you make a change, I see it, and make changes to my code using your changes. 72 hours later, they get reverted, which screws my code. Okay, so then nobody's going to compile anything, or use anything, if it has 72-hour code in it. The alternative, if they want the code badly enough, is to review it so it will stick. Well, that devolves to the same thing as pre-commit review. And ... this only makes sense for core, or maybe for stable extensions. It doesn't make sense for experimental extensions where only one person is likely to understand or care what the code says. ___ 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)
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 that's been avoided in this area by shooting down ideas rather than just taking action and working out the details as we go is equivalent to a meer fraction of the effort that's been put forth in the form of demanding a perfect solution and generally resisting change. We should probably also not be setting the stakes so high, it's making the cost of failure unnecessarily expensive. One project should change their review and release strategy, and see if that can be worked out and fine tuned. Then we can apply those lessons to the code base in general. This is especially useful if a strategy is radically different than what we have now. - Trevor 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 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 just our process. 2. We encourage committers to identify who will be reviewing their code as part of their commit comment. That way, we have an identified person who has license to revert if they don't understand the code. Social problems are hard to fix using technical means. What will happen if someone approves a revision that later turns out to be defective, off with their head? 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 quality will suffer. Although I'm not a biggest fan of rushing towards DVCS, the distributed heavily-branched model where mainline is supported only through merges from committed-then-reviewed branches looks far more superior to me. This way, we will also be able to push stuff to WMF more often as there will be no unreviwewed or unfinished code in trunk. -- Best regards, Max Semenik ([[User:MaxSem]]) ___ 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
Re: [Wikitech-l] Code review process (was: Status of more regular code deployments)
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 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 just our process. 2. We encourage committers to identify who will be reviewing their code as part of their commit comment. That way, we have an identified person who has license to revert if they don't understand the code. Russell N. Nelson - rnnelson wrote: Worse than pre-commit review. What if you make a change, I see it, and make changes to my code using your changes. 72 hours later, they get reverted, which screws my code. Okay, so then nobody's going to compile anything, or use anything, if it has 72-hour code in it. The alternative, if they want the code badly enough, is to review it so it will stick. Well, that devolves to the same thing as pre-commit review. And ... this only makes sense for core, or maybe for stable extensions. It doesn't make sense for experimental extensions where only one person is likely to understand or care what the code says. Perhaps a combination is possible, a bit like Brion did in the old days: * A revision has to be reviewed within reasonble time (time upto it still allows reverting without pain, ie. hours or days. Not weeks! No more than, say, 3 days) * The people who do code review, which should always start at the back, will see soon enough if there's a revision stuck in the process (ie. not being reviewed / passed to someone else) at that point someone has to go bold and revert it. Nothing personal: If nobody understood your commit, either splits it up and document it better – or poke whoever is capable of reviewing your code and make sure he or she does so in time. We are responsible as a team (nothign like primary school hand-in-hand pairs of buddies, but you get the idea..). * A fixme should not maintain it's status for more than 24 hours. Not fixed within 24 hours ? Revert, no questions asked! * Once a week, on Monday, we make sure anything from after Friday noon (remember, 3 days!) is unreviewed or fixme'ed. They shall be reverted or sorted out the same day (some gnomes may want to do this over the weekend). * Tuesday anything last week should no longer be on our minds. We've moved on! Tomorrow (wednesday) is already the 3rd day of the week. Aside from the in-svn proccess, some points I learned from other committers and just now via IRC: * Unless the committer is subscribed to mediawiki-cvs or keeps a tight eye on Special:Code, one should have wiki-account linked in the CodeReview extension and e-mail notifications on (so you know about questions/comments., status changed and follow-ups as soon as possible) * the todo tag (or soon-to-be-introduced status needsmore or incomplete)[1] does not mean in another life time. * If you don't know where to start... http://etherpad.wikimedia.org/CodeReviewTracker -- Krinkle [1] https://bugzilla.wikimedia.org/28274 ___ 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)
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 reviewer lined up, and make it clear that, if their code gets reverted, it's nothing personal...it's just our process. Worse than pre-commit review. What if you make a change, I see it, and make changes to my code using your changes. 72 hours later, they get reverted, which screws my code. For the same reason my position is that a minimum time before revert is not desireable -- if anything is ever found to be problematic, the correct thing is to remove it immediately. Robla counters this with the observation that when there are multiple people handling code review, simply being *uncertain* about whether something needs a revert or not is better grounds for *passing it on* to someone else to look at than to immediately revert. This should usually mean a few minutes on IRC, however, not just a 72-hour silence without feedback! What really worries me though isn't the somebody else built a patch on this within 72 hours, now it's slightly awkward to revert case but the other people built 100 patches on this in the last 10 months, now it's effing impossible to revert. 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 safely in isolation while the whole assembly line keeps moving. ** It's not a moral judgment against you when someone marks code as fixme or reverts it; it's simple workplace safety. -- brion ___ 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)
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 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 just our process. 2. We encourage committers to identify who will be reviewing their code as part of their commit comment. That way, we have an identified person who has license to revert if they don't understand the code. Social problems are hard to fix using technical means. What will happen if someone approves a revision that later turns out to be defective, off with their head? This isn't a technical solution, but a social one to force us to make sure that things that haven't been looked over GET looked at. Consider it to already be our hypothetical model -- it's just not been enforced as consistently as it's meant to. Bad code - fixed or reverted ASAP. Unseen code - assumed bad code, so look at it. 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 quality will suffer. I'd say better to do that continuously and confirm that things are being reviewed *and* run on automated tests *and* run against humans on actual-production-use beta servers *and* in actual production use within a reasonable time frame, than to let review slide for 10 months and rush it all right before a giant release. That IMO is what really hurts code review -- removing it from the context of code *creation*, and removing the iterative loop between writing, debugging, testing, debugging, deploying, debugging, fixing, and debugging code. Although I'm not a biggest fan of rushing towards DVCS, the distributed heavily-branched model where mainline is supported only through merges from committed-then-reviewed branches looks far more superior to me. This way, we will also be able to push stuff to WMF more often as there will be no unreviwewed or unfinished code in trunk. 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 trunk/mainlines's production deployment release channels until they're ready. But these need to be combined with really active, consistent management; just like keeping the deployment release branches clean, working, and up to date consistently requires a lot of attention -- when attention gets distracted, the conveyor belt falls apart and fixes features stop reaching the public until the next giant whole-release push, which turns into a circus. -- brion ___ 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)
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 safely in isolation while the whole assembly line keeps moving. ** It's not a moral judgment against you when someone marks code as fixme or reverts it; it's simple workplace safety. Often wiki workflow follows code development workflow, but in this case, code development workflow can take a lesson from the wiki: http://en.wikipedia.org/wiki/Wikipedia:BOLD,_revert,_discuss_cycle. :-) MZMcBride ___ 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)
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 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 just our process. Worse than pre-commit review. What if you make a change, I see it, and make changes to my code using your changes. 72 hours later, they get reverted, which screws my code. Okay, so then nobody's going to compile anything, or use anything, if it has 72-hour code in it. The alternative, if they want the code badly enough, is to review it so it will stick. Well, that devolves to the same thing as pre-commit review. And ... this only makes sense for core, or maybe for stable extensions. It doesn't make sense for experimental extensions where only one person is likely to understand or care what the code says. 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 reviewing them. If I make a change to the preprocessor, HipHop, Sanitizer, SecurePoll, passwords, tokens, or any of a number of other things, I'm going to need Tim to review them. I don't begrudge Tim the thousand-and-one other things he has to do besides review my code within 72 hours. Does that mean that no one should make *any* changes to *any* of those areas? More dangerously still, does that mean that **only people who can persuade Tim to review** be allowed to make changes to those areas? I know what the answers to those questions are *supposed* to be, that's not the point. The point is **what actually happens**. 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 the amount of code being committed. Any attempt at a solution which does not change that fundamental equation is doomed to failure. There are at least six things that could be changed (enough people, qualified people, enough time, Code Review, match[ing], being committed); we almost certainly need to change more than one. The most likely outcome of this particular suggestion is simply to radically reduce the amount of code being committed. I'm not sure that that's the best way to deal with the problem. --HM ___ 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)
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 the amount of code being committed. Maybe people shouldn't commit untested code so often. I'm not joking. -Chad ___ 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)
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 it so far and it works very well. http://dev.tiki.org/How+to+get+commit+access I don't know if this is feasible in this community - but wanted to put it out is as a possible direction. Best, Mark On 31May2011, at 4:52 PM, Chad wrote: 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 the amount of code being committed. Maybe people shouldn't commit untested code so often. I'm not joking. -Chad ___ 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
