Re: [DISCUSS] Disable merge for failing pull requests
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 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 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 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 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 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 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=is%3Apr+is%3Amerged+status%3Afailure >>> >>> -Dan >>> >>> On Tue, Nov 13, 2018 at 10:19 AM Ryan McMahon >> 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
Re: [DISCUSS] Disable merge for failing pull requests
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 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 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 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 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 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=is%3Apr+is%3Amerged+status%3Afailure >> >> -Dan >> >> On Tue, Nov 13, 2018 at 10:19 AM Ryan McMahon > 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 wrote: >>> Just to clarify, that flaky EvictionDUnitTest is old
Re: [DISCUSS] Disable merge for failing pull requests
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 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 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 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 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=is%3Apr+is%3Amerged+status%3Afailure >> > > > >> > > > -Dan >> > > > >> > > > On Tue, Nov 13, 2018 at 10:19 AM Ryan McMahon >> > > 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 >> > 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 >> 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
Re: [DISCUSS] Disable merge for failing pull requests
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 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 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 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=is%3Apr+is%3Amerged+status%3Afailure > > > > > > > > -Dan > > > > > > > > On Tue, Nov 13, 2018 at 10:19 AM Ryan McMahon > > > 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 > > 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 > 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=DwIBaQ=lnl9vOaLMzsy2niBC8-h_K-7QJuNJEsFrzdndhuJ3Sw=s8zALi1UpxiUlTfCkFIvTI7Yi4EtlpqAZ68hQ4JDyaI=EBW_QlDSlKgshy50KztUi566idyTMguNUkj6wc1jLXo=tgtdFeHVZtk1hlNfH-VTlrV9-WkUt_tWv_yx7MjSUdo= > > > -Dan > > > > > > On Mon, Nov 12, 2018 at 11:21 AM Udo Kohlmeyer > > > wrote: > > > > > > > 0 > > > > > > > > Patrick does make a
Re: [DISCUSS] Disable merge for failing pull requests
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 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 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=is%3Apr+is%3Amerged+status%3Afailure > > > > > > -Dan > > > > > > On Tue, Nov 13, 2018 at 10:19 AM Ryan McMahon > > 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 > 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 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=DwIBaQ=lnl9vOaLMzsy2niBC8-h_K-7QJuNJEsFrzdndhuJ3Sw=s8zALi1UpxiUlTfCkFIvTI7Yi4EtlpqAZ68hQ4JDyaI=EBW_QlDSlKgshy50KztUi566idyTMguNUkj6wc1jLXo=tgtdFeHVZtk1hlNfH-VTlrV9-WkUt_tWv_yx7MjSUdo= > > -Dan > > > > On Mon, Nov 12, 2018 at 11:21 AM Udo Kohlmeyer > > 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 > >
Re: [DISCUSS] Disable merge for failing pull requests
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 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=is%3Apr+is%3Amerged+status%3Afailure > > > > -Dan > > > > On Tue, Nov 13, 2018 at 10:19 AM Ryan McMahon > 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 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 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 > 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 >
Re: [DISCUSS] Disable merge for failing pull requests
+1 to blocking the "merge" button On Mon, Nov 19, 2018 at 5:09 PM Udo Kohlmeyer 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=is%3Apr+is%3Amerged+status%3Afailure > > > > -Dan > > > > On Tue, Nov 13, 2018 at 10:19 AM Ryan McMahon > 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 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 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 > 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
Re: [DISCUSS] Disable merge for failing pull requests
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=is%3Apr+is%3Amerged+status%3Afailure -Dan On Tue, Nov 13, 2018 at 10:19 AM Ryan McMahon 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 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 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 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. If it persists, perhaps a public flogging would be in order. 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
Re: [DISCUSS] Disable merge for failing pull requests
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=is%3Apr+is%3Amerged+status%3Afailure -Dan On Tue, Nov 13, 2018 at 10:19 AM Ryan McMahon 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 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 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 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. If it > > > > > persists, perhaps a public flogging would be in order. But at > > the > > > > end > > > > > of the day, the onus is on us to be responsible developers, and no
Re: [DISCUSS] Disable merge for failing pull requests
+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 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 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 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. If it > > > > persists, perhaps a public flogging would be in order. 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
Re: [DISCUSS] Disable merge for failing pull requests
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 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 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. If it > > > persists, perhaps a public flogging would be in order. 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
Re: [DISCUSS] Disable merge for failing pull requests
In that case, why don't we make the rule that the owner of the PR is not allowed to merge it. Another committer must then accept and merge it. --Udo On 11/12/18 14:03, Dan Smith 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 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. If it persists, perhaps a public flogging would be in order. 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 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 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
Re: [DISCUSS] Disable merge for failing pull requests
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 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. If it > > persists, perhaps a public flogging would be in order. 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 > > > 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 > >> 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 > >
Re: [DISCUSS] Disable merge for failing pull requests
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. If it persists, perhaps a public flogging would be in order. 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 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 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 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
Re: [DISCUSS] Disable merge for failing pull requests
-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. If it persists, perhaps a public flogging would be in order. 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 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 > 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 > > > 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 > > > > > > > > > > > > >
Re: [DISCUSS] Disable merge for failing pull requests
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 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 > 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 > > > > > > > >
Re: [DISCUSS] Disable merge for failing pull requests
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 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 > > > >
Re: [DISCUSS] Disable merge for failing pull requests
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
Re: [DISCUSS] Disable merge for failing pull requests
-1 I think there is still plenty of flakiness in the tests that PRs can go red due to something flaky. Blocking a merge when the issue is known to be flaky would be disruptive to progress. I think it will also just encourage someone to use direct pushing to work around false reds. This might lead to even worse transgressions, like merging changes without PRs. Do we really have a problem with someone merging legitimately broken PRs? On Fri, Nov 9, 2018 at 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 >
Re: [DISCUSS] Disable merge for failing pull requests
+1 > On Nov 9, 2018, at 2:07 PM, Helena Bales wrote: > > +1 > As far as the failure rate goes, even if the rate is 25% restarting the > pipeline usually gives a passing run. And this would make us less likely to > incorrectly dismiss a failure as not related. If the second build also > fails with the same failures, those should be investigated and resolved in > some way before the PR is merged. > > On Fri, Nov 9, 2018 at 1:49 PM Anthony Baker wrote: > >> It looks like the current failure rate (post-PR, including all types of >> failures) for DistributedTest is around 25%. Do most people experience >> similar failure rates on the *-pr pipeline? I’m specifically wondering >> about failures unrelated to your changes. >> >> Anthony >> >> >>> On Nov 9, 2018, at 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 >> >>
Re: [DISCUSS] Disable merge for failing pull requests
+1 As far as the failure rate goes, even if the rate is 25% restarting the pipeline usually gives a passing run. And this would make us less likely to incorrectly dismiss a failure as not related. If the second build also fails with the same failures, those should be investigated and resolved in some way before the PR is merged. On Fri, Nov 9, 2018 at 1:49 PM Anthony Baker wrote: > It looks like the current failure rate (post-PR, including all types of > failures) for DistributedTest is around 25%. Do most people experience > similar failure rates on the *-pr pipeline? I’m specifically wondering > about failures unrelated to your changes. > > Anthony > > > > On Nov 9, 2018, at 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 > >
Re: [DISCUSS] Disable merge for failing pull requests
It looks like the current failure rate (post-PR, including all types of failures) for DistributedTest is around 25%. Do most people experience similar failure rates on the *-pr pipeline? I’m specifically wondering about failures unrelated to your changes. Anthony > On Nov 9, 2018, at 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
Re: [DISCUSS] Disable merge for failing pull requests
+1 On Fri, Nov 9, 2018 at 12:58 PM Robert Houghton wrote: > +1 > > On Fri, Nov 9, 2018, 12:55 Dan Smith > > 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 > > >
Re: [DISCUSS] Disable merge for failing pull requests
+1 assuming “all of the PR checks” will soon include Java 11 Implementation-wise should be as simple at getting someone with admin privileges on the geode repo to change the settings for “develop” branch to "Require status checks to pass before merging." > On Nov 9, 2018, at 12:58 PM, Robert Houghton wrote: > > +1 > > On Fri, Nov 9, 2018, 12:55 Dan Smith >> 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 >>
Re: [DISCUSS] Disable merge for failing pull requests
+1 On Fri, Nov 9, 2018 at 12:58 PM, Robert Houghton wrote: > +1 > > On Fri, Nov 9, 2018, 12:55 Dan Smith > > 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 > > >
Re: [DISCUSS] Disable merge for failing pull requests
+1 On Fri, Nov 9, 2018, 12:55 Dan Smith 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 >
[DISCUSS] Disable merge for failing pull requests
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
