I'm changing my vote to -1 for disallowing merge until precheck passes. The reason is that it's rare for me to see a 100% clean precheckin for any of my PRs. There seems to always be some failure unrelated to my PR. For example PR #3042 just hit GEODE-6008. If we make the change to disable the merge button, then my PR could potentially be blocked indefinitely.
After we get it more consistently GREEN, I would be willing to change my vote to +1. On Fri, Dec 21, 2018 at 10:36 AM Kirk Lund <kl...@apache.org> wrote: > I was responding to Udo's comment: > > "Could one not configure the button that the owner of the PR cannot merge > the PR?" > > I'm +1 for disallowing merge until precheck passes. > I'm -1 for disallowing the owner of the PR to merge it. > > On Fri, Dec 21, 2018 at 9:28 AM Helena Bales <hba...@pivotal.io> wrote: > >> Kirk, this change would not require you to get someone to merge it. It >> would just require that your PR pass CI before it can be merged. >> >> On Thu, Dec 20, 2018 at 2:38 PM Kirk Lund <kl...@apache.org> wrote: >> >> > I have enough trouble just getting other developers to review my PR. I >> > don't want to have to struggle to find someone to merge it for me, too. >> > >> > On Mon, Nov 19, 2018 at 4:09 PM Udo Kohlmeyer <u...@apache.org> wrote: >> > >> > > I don't believe "name and shame" is a hammer we should wield, but if >> we >> > > have use it... use it wisely >> > > >> > > Could one not configure the button that the owner of the PR cannot >> merge >> > > the PR? >> > > >> > > --Udo >> > > >> > > >> > > On 11/19/18 16:03, Dan Smith wrote: >> > > > 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://urldefense.proofpoint.com/v2/url?u=https-3A__concourse.apachegeode-2Dci.info_teams_main_pipelines_apache-2Ddevelop-2Dmetrics_jobs_GeodeDistributedTestOpenJDK8Metrics_builds_23&d=DwIBaQ&c=lnl9vOaLMzsy2niBC8-h_K-7QJuNJEsFrzdndhuJ3Sw&r=s8zALi1UpxiUlTfCkFIvTI7Yi4EtlpqAZ68hQ4JDyaI&m=EBW_QlDSlKgshy50KztUi566idyTMguNUkj6wc1jLXo&s=tgtdFeHVZtk1hlNfH-VTlrV9-WkUt_tWv_yx7MjSUdo&e= >> > > >>>> -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 >> > > >>> >> > > >> > > >> > >> >
