+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 >>>>>> >>>> >>> >>
