Closing the loop on this thread - I don't feel like there was enough agreement to go forwards with disabling the merge button, so I'm going to drop this for now.
I would like to see everyone make sure that they only merge green PRs. Maybe we can go with a name and shame approach? As in, we shouldn't see any new PRs show up in this query: https://github.com/apache/geode/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Amerged+status%3Afailure -Dan On Tue, Nov 13, 2018 at 10:19 AM Ryan McMahon <rmcma...@pivotal.io> wrote: > +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 > > >
