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