Hi Alex. Merging fast and debug later sounds like a nice idea but IMHO, this requires 2 things to work: 1. Master is considered as unstable. 2. The test coverage must be more than extensive to cover almost everything.
For 1) we could adopt a github workflow but I don't think this is compatible with the way Apache handles projects on SVN (but I might be wrong). For 2) we do have extensive unit tests and some integration/live tests. Although, those are still not up and running at the moment if I'm not mistaken and the test coverage suffers from that. Besides, we currently have no CI/CD blueprint test deployments. Whilst the tests can pass, there is no guarantee that our blueprints (and blueprints from our users) will not fail. To take an example, the config inheritance introduced changes that had a lot of side effects on our own blueprints but all tests where passing. Swapping such a core component of Brooklyn is IMHO very scary and it becomes even more scarier to think that it could be merged without a proper review/QA. I think that merging fast just shift the problem on the side: instead of having reviews before merge, we will need to debug later on which seems counter productive in the long run. I understand how frustrating this can be and I agree on the argument that it does not send the right message to contributors. But if we are going to change such a huge core and central component, I think it's better to take the time to make it right beforehand. Best. On Tue, 2 May 2017, 15:50 Duncan Johnston Watt, < [email protected]> 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 > > > > > > > -- > > Duncan Johnston-Watt > > Founder & Chief Executive Officer > > Phone: +44 777 190 2653 | Skype: duncan_johnstonwatt > > Twitter: @duncanjw <https://twitter.com/duncanjw> | LinkedIn: > https://linkedin.com/in/duncanjohnstonwatt > > [image: Cloudsoft Logo.jpg] <https://cloudsoft.io/> > > Stay up to date with everything Cloudsoft: > > [image: Twitter_Logo_White_On_Blue.png] <https://twitter.com/cloudsoft> > [image: > YouTube-social-icon_red_48px.png] > <https://www.youtube.com/channel/UCpbLhvXrYWz8B_osUX6rn0Q> > -- Thomas Bouron • Senior Software Engineer @ Cloudsoft Corporation • https://cloudsoft.io/ Github: https://github.com/tbouron Twitter: https://twitter.com/eltibouron
