On 2017-03-09 6:51 PM, Justin Dolske wrote: > Similar to "r+ with fixes" are cases where a patch (or patch series) > requires reviews from a multitude of people. Which means variable delays in > getting reviews, during which things may slightly bitrot or be trivially > modified based on feedback from other reviewers. Code refactorings are one > example of this. Having to re-request review from everyone, perhaps > multiple times, would seem pretty painful. :)
Even with a single reviewer, I often times end up making some trivial changes to my patches to fix stupid mistakes and issues that I know the reviewer doesn't care enough to want to look at before landing. In general our code review process has a lot of flexibility built into it, and reviewers generally understand that the goal ultimately is to ensure the quality of the produced code, so depending on the circumstances as a reviewer I can treat a patch on different levels of scrutiny, from anywhere between checking the actual landed patch and complaining if something wasn't done in the way I asked to r+ing asking for a lot of changes and trusting the author will do the right thing without needing me look over their work more. > From previous discussions, it sounded like there was a middle ground in > still requiring all landings to go through automation (i.e., a strong link > between what actually landed and what was posted in the bug), Which, to some extent, goes against the flexibility of an author when they're trusted like in the above scenario. :( This is pretty disrupting to my personal productivity, for whatever that's worth (but I'll of course deal with it if I have to). I just don't understand why this is desirable as a goal. > but allow > some slop in what was formally reviewed (i.e., a weak link between last > review and what's was asked to land.) The automation might at least require > that you can't land a "r=sparky" unless "sparky" did at some point grant > r+. At the very least, this makes it somewhat easier to compare what landed > with what was last reviewed (with current tools, thanks to ReviewBoard's > revision diffing). FTR we have real actual experience with this: for years I have had to append a=me etc to work around various hooks. I don't understand how something in the commit message can help here, if for example the requirement is for the reviewer to have to r+ the thing that gets autolanded every time. > I suppose this policy also implies no more "Typo fix, no bug" landings. That is also unacceptable. > But > I never really liked those anyway, so I'm fine with that. Bugs are cheap! :) > > Justin > > On Thu, Mar 9, 2017 at 3:11 PM, <chris.ryan.pea...@gmail.com> wrote: > >> On Friday, March 10, 2017 at 10:53:50 AM UTC+13, Mike Connor wrote: >>> (please direct followups to dev-planning, cross-posting to governance, >>> firefox-dev, dev-platform) >>> >>> >>> Nearly 19 years after the creation of the Mozilla Project, commit access >>> remains essentially the same as it has always been. We've evolved the >>> vouching process a number of times, CVS has long since been replaced by >>> Mercurial & others, and we've taken some positive steps in terms of >>> securing the commit process. And yet we've never touched the core idea >> of >>> granting developers direct commit access to our most important >>> repositories. After a large number of discussions since taking ownership >>> over commit policy, I believe it is time for Mozilla to change that >>> practice. >>> >>> Before I get into the meat of the current proposal, I would like to >> outline >>> a set of key goals for any change we make. These goals have been >> informed >>> by a set of stakeholders from across the project including the >> engineering, >>> security, release and QA teams. It's inevitable that any significant >>> change will disrupt longstanding workflows. As a result, it is critical >>> that we are all aligned on the goals of the change. >>> >>> >>> I've identified the following goals as critical for a responsible commit >>> access policy: >>> >>> >>> - Compromising a single individual's credentials must not be >> sufficient >>> to land malicious code into our products. >>> - Two-factor auth must be a requirement for all users approving or >>> pushing a change. >>> - The change that gets pushed must be the same change that was >> approved. >>> - Broken commits must be rejected automatically as a part of the >> commit >>> process. >>> >>> >>> In order to achieve these goals, I propose that we commit to making the >>> following changes to all Firefox product repositories: >>> >>> >>> - Direct commit access to repositories will be strictly limited to >>> sheriffs and a subset of release engineering. >>> - Any direct commits by these individuals will be limited to fixing >>> bustage that automation misses and handling branch merges. >>> - All other changes will go through an autoland-based workflow. >>> - Developers commit to a staging repository, with scripting that >>> connects the changeset to a Bugzilla attachment, and integrates >>> with review >>> flags. >>> - Reviewers and any other approvers interact with the changeset as >>> today (including ReviewBoard if preferred), with Bugzilla flags as >> the >>> canonical source of truth. >>> - Upon approval, the changeset will be pushed into autoland. >>> - If the push is successful, the change is merged to >> mozilla-central, >>> and the bug updated. >>> >>> I know this is a major change in practice from how we currently operate, >>> and my ask is that we work together to understand the impact and >> concerns. >>> If you find yourself disagreeing with the goals, let's have that >> discussion >>> instead of arguing about the solution. If you agree with the goals, but >>> not the solution, I'd love to hear alternative ideas for how we can >> achieve >>> the outcomes outlined above. >>> >>> -- Mike >> >> >> How will uplifts work? Can only sheriffs land them? >> >> This would do-away with "r+ with comments addressed". Reviewers typically >> only say this for patches submitted by people they trust. So removing "r+ >> with comments" would mean reviewers would need to re-review code, causing >> an extra delay and extra reviewing load. Is there some way we can keep the >> "r+ with comments addressed" behaviour for trusted contributors? >> >> _______________________________________________ >> dev-platform mailing list >> dev-platform@lists.mozilla.org >> https://lists.mozilla.org/listinfo/dev-platform >> > _______________________________________________ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
