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