+1 to blocking the "merge" button
On Mon, Nov 19, 2018 at 5: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://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 > >>> > >
