On Wed, Nov 11, 2015 at 4:22 PM, Lars Bergstrom <larsb...@mozilla.com> wrote:
> I've been assuming the only time that we would use `delegate` is for > people like you (Bobby), whom we trust, but who have not gone through > the process to become a Servo reviewer. Sure. From a personal standpoint, my ask here is to make that decision explicit and global, rather than leaving it per-(reviewer, PR), because I want to remove these (minor) barriers to my productivity. >From a project standpoint, I am trying to blaze the trail of the committer-but-not-reviewer role, because I think it can be a useful one - for example, Ms2ger did a ton of great work on Gecko in this capacity back in the day. I have a personal preference for not doing a bunch of Servo reviews right now (mostly related to the 6-8 Gecko modules where I'm currently shirking my review obligations). And while I'm sure I could review a few token patches and cajole people into making me a reviewer, I'd rather advocate to improve support for this role and remove barriers that I think are unnecessary. If that fails, I'll probably resort to cajoling. ;-) > Honestly, I'd rather not have > this feature at all --- I would prefer that a reviewer (even if it's > the PR author) at least glance at all squashes / rebases in Reviewable > to verify that nothing slipped in, because we all mess up `git rebase` > from time to time. > This suggests that you trust Servo Reviewers to self-check against mis-merges, but do not trust anyone else. I think that is unnecessarily restrictive, and that there is a class of contributors (like myself, the Ms2ger-of-old, and the hypothetical RyanVM-of-Servo) who can qualify for the "merge-er" badge without qualifying for the "reviewer" badge. > But, until we have closer to 24/7 reviewer > coverage, like Rust does, I was in favor of adding this support so > that you are not blocked during the 1--2AM US Pacific slot that > appears to be both the "deadzone" for our reviewer coverage and your > prime patch-landing time :-) > To be clear, #servo support is phenomenal, and Manish is basically awake all the time anyway. It is nevertheless frustrating to be required to ask for help to complete basic tasks when that requirement appears ill-conceived. Imagine that you needed to ping someone in order to, say, clobber your objdir - that restriction would not likely be a major barrier to practical development (since Manish is always there, and you don't clobber your objdir _that_ often), but it would still feel annoying and unnecessary. Put another way: I already have access to the green merge button, and therefore write access to the repo. I don't use it because I know I'm supposed to go through bors-servo instead, which is why it feels silly that bors-servo is programmed to treat me as untrusted. I haven't been a fan of "commit" access in the past because the Servo > process tends to focus on doing a lot of before-commit work (running > the full test suite, doing full reviews, etc.) to avoid the need to > ever close tree or back anything out. The bors model certainly obviates the need for manual landings (c/f the disused green merge button). But it also creates an arbitrarily-long window between "submitted for landing" and "landed" during which new merge conflicts may arise, and puts the burden of resolving trivial merge conflicts on the contributor (rather than any sort of sheriff figure). This increases number of times that a patch author needs to rebase, and consequently the value of being able to rebase without round-tripping through IRC. > But, I can definitely see both > your argument and have heard from other open source people (especially > consultants and employees at BigCos) that having a "commit" level of > access that's given out much more freely is a nice thing for them to > be able to show to their bosses or use to market themselves, at least > in the WebKit and Blink worlds. > FWIW, I think that's a somewhat-unfortunate overloading that we probably shouldn't be optimizing for. bholley > - Lars > > > > On Wed, Nov 11, 2015 at 5:47 PM, Bobby Holley <bobbyhol...@gmail.com> > wrote: > > Thanks for working on this Manish! > > > > As mentioned previously, I think that scoping this bit of trust to the PR > > is sub-optimal. This kind of delegation is effectively unsupervised write > > access to the repository, with an understanding that the committer is > > trustworthy enough to ask for review on any significant not-yet-approved > > changes. It doesn't really have anything to do with the particular PR, > and > > has everything to do with the person being granted that access. > > > > This kind of trust is a governance issue, and making it a per-reviewer > > per-PR preference introduces the following problems: > > * The committer needs to establish the trust relationship with each > > individual reviewer, which doesn't scale for either party. > > * Reviewers need to make an important trust decision for every PR, and > may > > not always make decisions that are consistent with what they and others > > have decided previously. > > * Reviewers may not always remember to set the delegate flag. > > > > The combination of all three of the above means that, in practice, a > > contributor is likely to find themselves with carry privileges in some > > situations but not others. Is that ever desirable? > > > > bholley > > > > On Wed, Nov 11, 2015 at 3:01 AM, Manish Goregaokar < > manishsm...@gmail.com> > > wrote: > > > >> After some > >> < > https://github.com/servo/servo/wiki/Meeting-2015-11-02#review-carry-over> > >> discussion > >> < > >> > http://logs.glob.uno/?c=mozilla%23servo&s=10+Nov+2015&e=10+Nov+2015&h=delegate#c298415 > >> > > >> I've landed <https://github.com/barosl/homu/pull/111> and enabled > review > >> delegation for homu. > >> > >> This means that any homu command containing `delegate=USER` will give > that > >> user full reviewer status for the scope of that PR (i.e., they can use > all > >> homu commands for that PR). You can also use `delegate+` as a shortcut > to > >> delegate to the PR author. `delegate-` undoes delegation. > >> > >> Reviewers: In case of "r=me with minor nit" situations (where the > author is > >> trusted enough to not add our infra to a botnet or whatever), try to use > >> `@bors-servo delegate+` so that the author can finish up the PR on their > >> own. This should ease up the workflow for non-reviewers. > >> > >> You can see it in action here <https://github.com/servo/saltfs/pull/160 > >. > >> > >> Thanks, > >> -Manish Goregaokar > >> _______________________________________________ > >> dev-servo mailing list > >> dev-servo@lists.mozilla.org > >> https://lists.mozilla.org/listinfo/dev-servo > >> > > _______________________________________________ > > dev-servo mailing list > > dev-servo@lists.mozilla.org > > https://lists.mozilla.org/listinfo/dev-servo > _______________________________________________ dev-servo mailing list dev-servo@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-servo
