To circle back to Aled's idea regarding guidelines, I opened a PR[1] that leverages the new GitHub feature[2] to have PR templates.
Best. [1] https://github.com/apache/brooklyn-server/pull/664 [2] https://github.com/blog/2111-issue-and-pull-request-templates On Tue, 2 May 2017 at 19:22 Aled Sage <[email protected]> wrote: > Hi Alex, > > Interesting question. A few initial thoughts: > > *YOML* > YOML (PR #363) is an exceptional case - we should not use that as an > example when discussing this meta-question. The PR is 12,000 lines > (including comments/notes), and was not discussed on the mailing list > before it was submitted. I suggest we have a separate email thread > specifically about merging that PR, as there are certainly very useful > things we'd get from YOML. > > *Small PRs* > We should strongly encourage small focused PRs on a single thing, > wherever possible. That will make review faster, easier and lower risk. > For such PRs, we should strive for review+merge within days (7 days > being an upper bound in normal circumstances, hopefully). > > We can add some brief guidelines to this effect at > http://brooklyn.apache.org/developers/how-to-contribute.html > > *Changing low-level Brooklyn* > PRs that change low-level things in Brooklyn (e.g. changes to > config-inheritance etc) deserve thorough review. They are high-risk as > the unforeseen consequences of the changes can be very subtle, and break > downstream blueprints that rely on old ways of doing things. > > *Light-weight Review* > I agree with you - where PRs look sensible, low-risk and unit tested we > should take more risk and merge them sooner (even if there's not been > time for a thorough review by the community). > > Aled > > > On 02/05/2017 15:50, Duncan Johnston Watt wrote: > > Hi Alex > > > > This is probably covered already but I guess there needs to be an impact > > assessment (by submitter?) before something is waved through by default. > > > > Best > > > > Duncan > > > > On 2 May 2017 at 06:52, Alex Heneveld <[email protected]> > > wrote: > > > >> Hi Brooklyners- > >> > >> As many of you know, my YOML PR #363 [1] has been open for a while. This > >> sets up a foundation for giving better documentation and feedback and > >> hugely simplifying how we do our parsing. However it's a very big PR. > I'm > >> eager to have people spend some time using it and ideally extending it > -- > >> but here I wanted to raise a meta-question: > >> > >> > >> *W**hat should we do with a PR when we aren't able to review things in > as > >> much depth as we'd like?* > >> > >> > >> One option -- call it (A) -- is to say if we can't review things > >> thoroughly in a reasonable timeframe, we do a lighter review and if the > PR > >> looks promising and safe we merge it. > >> > >> The other option -- call it (B) -- is to leave PRs open for as long as > it > >> takes for us to do the complete review. > >> > >> I think most people have been approaching this with a mindset of (B), > and > >> while that's great for code quality and shared code understanding, if we > >> can't deliver on that quickly, it's frankly anti-social. The > contributor > >> has to deal with merge conflicts (and the rudeness of his or her > >> contribution being ignored), and Brooklyn loses velocity. My PR is an > >> extreme example but many have been affected by slow reviews, and I think > >> the expectation that reviews have to be so thorough is part of the > >> problem: it even discourages reviewers, as if you're not an expert in > an > >> area you probably don't feel qualified to review. > >> > >> We have good test coverage so product risk of (A) is small, and we have > >> great coders so I've no worry about us being able to solve problems that > >> (A) might introduce. We should be encouraging reviewers to look at any > >> area, and we need to solve the problem of slow reviews. > >> > >> > >> *I propose that the**standard we apply is that we quickly either merge > PRs > >> or identify what the contributor needs to resolve. > >> > >> > >> *I'm all for thorough reviews and shared understanding, but if we can't > do > >> this quickly I suggest we are better to sacrifice those things rather > than > >> block contributions, stifle innovation, and discourage reviews by > insisting > >> on a standards that we struggle to sustain. > >> > >> As a general rule of thumb, maybe something like: > >> > >> (1) After 7 days of no activity on a PR we go with an "eyeball test"; > >> unless the following statement is untrue we say: > >> > >> /I haven't done as much review as I'd like, but the code is clearly > >> helpful, not risky or obviously wrong or breaking compatibility, it has > >> good test coverage, and we can reasonably expect the contributor or > >> committers to maintain it. Leaving open a bit longer in case someone > else > >> wants to review more but if nothing further in the next few days, let's > >> merge. > >> > >> /(If there are committers who are likely to be specifically interested, > >> call them out as CC.) > >> > >> (2) After 3 more days, if no activity, merge it. > >> > >> And we encourage _anyone_ to review anything. If the above response is > >> the baseline, everyone in our community is qualified to do it or better > and > >> we'll be grateful! > >> > >> Best > >> Alex > >> > >> > >> [1] https://github.com/apache/brooklyn-server/pull/363 > >> > >> > > > > -- Thomas Bouron • Senior Software Engineer @ Cloudsoft Corporation • https://cloudsoft.io/ Github: https://github.com/tbouron Twitter: https://twitter.com/eltibouron
