+1 I like this idea, but I recognize that it will be a challenge when there is still some flakiness to the pipeline. I think we'd need clear guidelines on what to do if your PR fails due to something seemingly unrelated. For instance, we ran into GEODE-5943 (flaky EvictionDUnitTest) in our last PR, and saw that there was already an open ticket for the flakiness (we even reverted our change and reproduced to be sure). So we triggered another PR pipeline and it passed the second time. Is rerunning the pipeline again sufficient in this case? Or should we have stopped what we were doing and took up GEODE-5943, assuming it wasn't assigned to someone? If it was already assigned to someone, do we wait until that bug is fixed before we run through the PR pipeline again?
I think if anything this will help us treat bugs that are causing a red pipeline as critical, and I think that is a good thing. So I'm still +1 despite the flakiness. Just curious what people think about how we should handle cases where there is a known failure and it is indeed unrelated to our PR. Ryan On Mon, Nov 12, 2018 at 2:49 PM Jinmei Liao <jil...@pivotal.io> wrote: > 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 >
