No one felt one review was too much. At least one is bare minimum. Regards Naba
On Sat, Oct 19, 2019 at 11:33 AM Owen Nichols <onich...@pivotal.io> wrote: > +1 for starting with only Build, Unit, and Stress tests. > > When you say you propose to "require pull request reviews" before merging, > what number of reviews are you proposing? When we discussed this before < > https://lists.apache.org/thread.html/3e9a78a474b15d900244a86ed80358e59f4836a36e7368ae15eeeb17@%3Cdev.geode.apache.org%3E> > nearly everyone felt that “3” was way too high but also some people even > thought “1” was too high. The consensus then was to leave it up to the PR > author how many reviews, if any, they needed to feel comfortable. > > It looks like any committer can bypass there GitHub safeguards if they > wish just by committing directly to develop, so I guess that this proposal > remains compatible with “people over process”. > > -Owen > > > On Oct 18, 2019, at 4:11 PM, Alexander Murmann <amurm...@apache.org> > wrote: > > > > +1 for the 👶 steps proposal > > > > On Fri, Oct 18, 2019 at 3:30 PM Donal Evans <doev...@pivotal.io> wrote: > > > >> Big +1 from me on the revised proposal. It seems like there'd rarely be > >> a case where something needs to be merged so fast that we can't even > wait > >> for the build and unit tests to pass, and preventing the addition of > flaky > >> tests by running the StressNewTest might help address the apparent > trend > >> of increase in flakiness thats been talked about. > >> > >> On Fri, Oct 18, 2019 at 3:15 PM Nabarun Nag <n...@apache.org> wrote: > >> > >>> @Bruce Schuchardt <bschucha...@pivotal.io> > >>> I completely agree with that assessment that not all flaky tests are > >>> related to the test but may involve issues with the product itself. > >>> > >>> @Ernie > >>> As you can see with the example that you provided, even when committers > >> are > >>> expected to do their due diligence they sometimes end up doing > something > >>> that needs to be reverted. > >>> > >>> It's ok to have some safeguards. I plan to look at them like seatbelts > >> in a > >>> car, even when we are diligent, unexpected things happen. > >>> > >>> My intention with this email was *never* to blame others / point out > PRs > >>> that broke the build, etc. > >>> I want guard rails that will help even me from making mistakes. > >>> > >>> I understand that the consensus is that distributed/integration/upgrade > >>> tests at the moment are flaky, hence blocking the merge button because > of > >>> them will not be an efficient call. > >>> > >>> *NEW PROPOSAL*: baby steps. > >>> *Github's Branch Protection Rule *has the following rule settings that > I > >>> propose to activate: > >>> - Require pull request reviews before merging > >>> - Require status checks to pass before merging > >>> [Only for > >>> - concourse-ci/Build > >>> - concourse-ci/UnitTestOpenJDK11 > >>> - concourse-ci/UnitTestOpenJDK8 > >>> - concourse-ci/StressNewTestOpenJDK11] > >>> > >>> *Advantages:* > >>> - Prevent us from accidentally merging PRs without reviews, ensure that > >> we > >>> follow *the Apache way* of involving the community in what code is > going > >>> into the repo. > >>> - Our build is not flaky, hence it helps us in avoiding merging code > >> which > >>> will break the build > >>> - Avoid mistakenly merging spotless errors > >>> - Unit tests are not flaky hence they can be included > >>> - Any new tests that are being added can't be merged if they fail the > >>> stress test. > >>> > >>> > >>> Apache values people over process, I highly appreciate that sentiment > >> but > >>> they never said not to take help from tools to save time and avoid > >>> mistakes. > >>> > >>> If we search for this feature in INFRA tickets, a lot of Apache > projects > >>> have enabled this feature for their repositories. > >>> > >>> Regards > >>> Naba > >>> > >>> > >>> > >>> On Fri, Oct 18, 2019 at 1:39 PM Bruce Schuchardt < > bschucha...@pivotal.io > >>> > >>> wrote: > >>> > >>>> Given the current state of affairs I don't think we should disable the > >>>> merge button. > >>>> > >>>> Ultimately it would be nice if we fixed all of the flaky tests. > >>>> Personally I don't think all of the tests that we think are "flaky" > are > >>>> test problems. In the last few months I've fixed product problems > that > >>>> were hit by supposedly "flaky" tests. Those tests were just unable to > >>>> reproduce the product problem 100% of the time. A flickering test > >>>> doesn't always mean it's a problem with the test. > >>>> > >>>> On 10/18/19 12:46 PM, Ernest Burghardt wrote: > >>>>> I had one recently that was Approved and I merged pre-maturely and > >> had > >>> to > >>>>> be reverted: d63638e4654bc6c71a232838b745dec6ef476ec9 > >>>>> > >>>>> Subsequently I have run into some test flakiness, but if a PR > >> submitter > >>>> has > >>>>> a pre-checkin failure it could be tricky to tell that its a Flaky > >>>>> situation... In my last go at a Flaky failure in pre-checkin, I was > >>> able > >>>> to > >>>>> search the Geode Jira and found the failure was a known flaky like > >> this > >>>> one > >>>>> <https://issues.apache.org/jira/browse/GEODE-6324> > >>>>> > >>>>> I'd prefer to trust our committers to perform their due diligence and > >>>> make > >>>>> good choices. > >>>>> > >>>>> EB > >>>>> > >>>>> On Fri, Oct 18, 2019 at 12:18 PM Owen Nichols <onich...@pivotal.io> > >>>> wrote: > >>>>> > >>>>>> Do you have a recent example of a PR that was merged despite failed > >> PR > >>>>>> checks, which then broke the build? > >>>>>> > >>>>>> At last discussion, one concern raised was providing a way that > >> anyone > >>>> in > >>>>>> the community could re-trigger a failed PR check if it hit an > >>> unrelated > >>>>>> flaky failure. > >>>>>> > >>>>>> Let’s be sure we've identified the problem before assuming the > >>> solution. > >>>>>> Apache values people over process. > >>>>>> > >>>>>>> On Oct 18, 2019, at 11:48 AM, Nabarun Nag <n...@apache.org> wrote: > >>>>>>> > >>>>>>> Hi devs, > >>>>>>> > >>>>>>> A few months ago a proposal was brought up regarding blocking the > >>> merge > >>>>>>> button on the github PR page in case of failing tests in the > >>> precheck. > >>>>>>> > >>>>>>> What is the sentiment regarding this now? Do we feel that it should > >>> be > >>>>>>> implemented? > >>>>>>> > >>>>>>> Or at least take the minimal step of not allowing merge till all > >>> tests > >>>>>> are > >>>>>>> done? > >>>>>>> > >>>>>>> > >>>>>>> Regards > >>>>>>> Nabarun Nag > >>>>>> > >>>> > >>> > >> > >
