geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ernest Burghardt <eburgha...@pivotal.io>
Subject Re: [DISCUSS] Disable merge for failing pull requests
Date Thu, 20 Dec 2018 22:18:20 GMT
+1 to blocking the "merge" button


On Mon, Nov 19, 2018 at 5: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://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 <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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message