geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Owen Nichols <onich...@pivotal.io>
Subject Re: [DISCUSS] Disable merge for failing pull requests
Date Tue, 04 Jun 2019 23:06:28 GMT
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 <klund@apache.org> wrote:
> 
> I'm changing my vote to -1 for disallowing merge until precheck passes.
> 
> The reason is that it's rare for me to see a 100% clean precheckin for any
> of my PRs. There seems to always be some failure unrelated to my PR. For
> example PR #3042 just hit GEODE-6008. If we make the change to disable the
> merge button, then my PR could potentially be blocked indefinitely.
> 
> After we get it more consistently GREEN, I would be willing to change my
> vote to +1.
> 
> On Fri, Dec 21, 2018 at 10:36 AM Kirk Lund <klund@apache.org> wrote:
> 
>> I was responding to Udo's comment:
>> 
>> "Could one not configure the button that the owner of the PR cannot merge
>> the PR?"
>> 
>> I'm +1 for disallowing merge until precheck passes.
>> I'm -1 for disallowing the owner of the PR to merge it.
>> 
>> On Fri, Dec 21, 2018 at 9:28 AM Helena Bales <hbales@pivotal.io> wrote:
>> 
>>> Kirk, this change would not require you to get someone to merge it. It
>>> would just require that your PR pass CI before it can be merged.
>>> 
>>> On Thu, Dec 20, 2018 at 2:38 PM Kirk Lund <klund@apache.org> wrote:
>>> 
>>>> I have enough trouble just getting other developers to review my PR. I
>>>> don't want to have to struggle to find someone to merge it for me, too.
>>>> 
>>>> On Mon, Nov 19, 2018 at 4:09 PM Udo Kohlmeyer <udo@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 <rmcmahon@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 <jiliao@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 <dsmith@pivotal.io>
>>> wrote:
>>>>>>>> 
>>>>>>>>> To be clear, I don't think we have an issue of untrustworthy
>>>>> committers
>>>>>>>>> pushing code they know is broken to develop.
>>>>>>>>> 
>>>>>>>>> The problem is that it is all to easy to look at a PR
with some
>>>>>>> failures
>>>>>>>>> and *assume* your code didn't cause the failures and
merge it
>>>> anyway.
>>>>> I
>>>>>>>>> think we should all be at least rerunning the tests and
not
>>> merging
>>>>> the
>>>>>>>> PR
>>>>>>>>> until everything passes. If the merge button is greyed
out, that
>>>> might
>>>>>>>> help
>>>>>>>>> communicate that standard to everyone.
>>>>>>>>> 
>>>>>>>>> Looking at the OpenJDK 8 metrics, it looks to me like
most of the
>>>>>>> issues
>>>>>>>>> are recently introduced (builds 81-84 and the EvictionDUnitTest),
>>>> not
>>>>>>> old
>>>>>>>>> flaky tests. So I think we were a little more disciplined
always
>>>>>>>> listening
>>>>>>>>> to what the checks are telling us we would have less
noise in the
>>>> long
>>>>>>>> run.
>>>>>>>>> 
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>>> 
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__concourse.apachegeode-2Dci.info_teams_main_pipelines_apache-2Ddevelop-2Dmetrics_jobs_GeodeDistributedTestOpenJDK8Metrics_builds_23&d=DwIBaQ&c=lnl9vOaLMzsy2niBC8-h_K-7QJuNJEsFrzdndhuJ3Sw&r=s8zALi1UpxiUlTfCkFIvTI7Yi4EtlpqAZ68hQ4JDyaI&m=EBW_QlDSlKgshy50KztUi566idyTMguNUkj6wc1jLXo&s=tgtdFeHVZtk1hlNfH-VTlrV9-WkUt_tWv_yx7MjSUdo&e=
>>>>>>>>> -Dan
>>>>>>>>> 
>>>>>>>>> On Mon, Nov 12, 2018 at 11:21 AM Udo Kohlmeyer <udo@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
<
>>>>>>>>> gosullivan@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 <
>>>>>>>>> amurmann@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 <
>>>>>>>>>> bschuchardt@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
>>>>>>>> 
>>>>> 
>>>>> 
>>>> 
>>> 
>> 


Mime
View raw message