I’m still not interested until there is a solution for all community members to retrigger failed jobs. Also not excited about not having a way to override if there is a known issue that prevents a job from going green. My last PR needed multiple reruns because of a known flakey test with an active ticket. While mine eventually went green on its own it would have been annoying to keep hitting a retrigger.
-jake > On Jun 4, 2019, at 4:06 PM, Owen Nichols <onich...@pivotal.io> wrote: > > I’d like to follow up on this discussion from late last year. Six months > ago, Kirk wrote: > >> After we get it more consistently GREEN, I would be willing to change my >> vote to +1. > > While we’re still not perfect, I have noticed that PR checks go green much > more consistently now than they did six months ago. Also, Ryan wrote: > >>>>>>>> I think we'd need clear guidelines on what to do if your PR fails due >>>>>>>> to something seemingly unrelated. > > > If you still encounter a flaky failure occasionally, you can either > re-trigger all checks with an empty commit, or just ask on the dev list and > someone will be happy to help you re-trigger only your failed check. > > > The above concerns were commonly cited as reasons for not moving ahead with > the proposal to enable GitHub policy to disable merge button until checks > have passed. Even with these addressed, there is still a “people over > process” argument to be made for not imposing an enforced process (see recent > thread that rejected imposing a requirement of >0 reviews before allowing > merge). > > > So, is there any interest at all in tightening GitHub controls? Or maybe a > better way to approach the question is: are we Very Happy with our current > source control practices? > > -Owen > >> On Dec 26, 2018, at 4:03 PM, Kirk Lund <kl...@apache.org> wrote: >> >> 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 >>>>>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >
