Just to clarify, that flaky EvictionDUnitTest is old flaky. The PR to refactor the test passed all checks, even the repeatTest as well. I had a closed PR that just touched the "un-refactored" EvictionDUnitTest, it wouldn't even pass the repeatTest at all.
On Mon, Nov 12, 2018 at 2:04 PM Dan Smith <dsm...@pivotal.io> wrote: > To be clear, I don't think we have an issue of untrustworthy committers > pushing code they know is broken to develop. > > The problem is that it is all to easy to look at a PR with some failures > and *assume* your code didn't cause the failures and merge it anyway. I > think we should all be at least rerunning the tests and not merging the PR > until everything passes. If the merge button is greyed out, that might help > communicate that standard to everyone. > > Looking at the OpenJDK 8 metrics, it looks to me like most of the issues > are recently introduced (builds 81-84 and the EvictionDUnitTest), not old > flaky tests. So I think we were a little more disciplined always listening > to what the checks are telling us we would have less noise in the long run. > > > https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-metrics/jobs/GeodeDistributedTestOpenJDK8Metrics/builds/23 > > -Dan > > On Mon, Nov 12, 2018 at 11:21 AM Udo Kohlmeyer <u...@apache.org> wrote: > > > 0 > > > > Patrick does make a point. The committers on the project have been voted > > in as "responsible, trustworthy and best of breed" and rights and > > privileges according to those beliefs have been bestowed. > > > > We should live those words and trust our committers. In the end.. If > > there is a "rotten" apple in the mix this should be addressed, maybe not > > as public flogging, but with stern communications. > > > > On the other side, I've also seen the model where the submitter of PR is > > not allowed to merge + commit their own PR's. That befalls to another > > committer to complete this task, avoiding the whole, "I'll just quickly > > commit without due diligence". > > > > --Udo > > > > > > On 11/12/18 10:23, Patrick Rhomberg wrote: > > > -1 > > > > > > I really don't think this needs to be codified. If people are merging > > red > > > PRs, that is a failing as a responsible developer. Defensive > programming > > > is all well and good, but this seems like it's a bit beyond the pale in > > > that regard. I foresee it making the correction of a red main pipeline > > > must more difficult that it needs to be. And while it's much better > than > > > it had been, I am (anecdotally) still seeing plenty of flakiness in my > > PRs, > > > either resulting from infra failures or test classes that need to be > > > refactored or reevaluated. > > > > > > If someone is merging truly broken code that has failed precheckin, > that > > > seems to me to be a discussion to have with that person. <s> If it > > > persists, perhaps a public flogging would be in order. </s> But at the > > end > > > of the day, the onus is on us to be responsible developers, and no > amount > > > of gatekeeping is going to be a substitute for that. > > > > > > On Mon, Nov 12, 2018 at 9:38 AM, Galen O'Sullivan < > gosulli...@pivotal.io > > > > > > wrote: > > > > > >> I'm in favor of this change, but only if we have a way to restart > > failing > > >> PR builds without being the PR submitter. Any committer should be able > > to > > >> restart the build. The pipeline is still flaky enough and I want to > > avoid > > >> the situation where a new contributor is asked repeatedly to push > empty > > >> commits to trigger a PR build (in between actual PR review) and their > PR > > >> gets delayed by days if not weeks. That's a real bad experience for > the > > >> people we want to be inviting in. > > >> > > >> On Mon, Nov 12, 2018 at 9:23 AM Alexander Murmann < > amurm...@pivotal.io> > > >> wrote: > > >> > > >>> What's the general consensus on flakiness of the pipeline for this > > >> purpose? > > >>> If there is consensus that it's still too flaky to disable the merge > > >> button > > >>> on failure, we should probably consider doubling down on that issue > > >> again. > > >>> It's hard to tell from just looking at the dev pipeline because you > > >> cannot > > >>> see easily what failures were legitimate. > > >>> > > >>> On Mon, Nov 12, 2018 at 8:47 AM Bruce Schuchardt < > > bschucha...@pivotal.io > > >>> > > >>> wrote: > > >>> > > >>>> I'm in favor of this. > > >>>> > > >>>> Several times over the years we've made a big push to get precheckin > > to > > >>>> reliably only to see rapid degradation due to crappy code being > pushed > > >>>> in the face of precheckin failures. We've just invested another > half > > >>>> year doing it again. Are we going to do things differently now? > > >>>> Disabling the Merge button on test failure might be a good start. > > >>>> > > >>>> On 11/9/18 12:55 PM, Dan Smith wrote: > > >>>> > > >>>>> Hi all, > > >>>>> > > >>>>> Kirks emails reminded me - I think we are at the point now with our > > >> PR > > >>>>> checks where we should not be merging anything to develop that > > >> doesn't > > >>>> pass > > >>>>> all of the PR checks. > > >>>>> > > >>>>> I propose we disable the merge button unless a PR is passing all of > > >> the > > >>>>> checks. If we are in agreement I'll follow up with infra to see how > > >> to > > >>>> make > > >>>>> that happen. > > >>>>> > > >>>>> This would not completely prevent pushing directly to develop from > > >> the > > >>>>> command line, but since most developers seem to be using the github > > >>> UI, I > > >>>>> hope that it will steer people towards getting the PRs passing > > >> instead > > >>> of > > >>>>> using the command line. > > >>>>> > > >>>>> Thoughts? > > >>>>> -Dan > > >>>>> > > >>>> > > > > > -- Cheers Jinmei
