geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nabarun Nag <n...@pivotal.io>
Subject Re: [DISCUSS] Blocking merge button in PR
Date Sat, 19 Oct 2019 19:18:18 GMT
No one felt one review was too much. At least one is bare minimum.

Regards
Naba


On Sat, Oct 19, 2019 at 11:33 AM Owen Nichols <onichols@pivotal.io> wrote:

> +1 for starting with only Build, Unit, and Stress tests.
>
> When you say you propose to "require pull request reviews" before merging,
> what number of reviews are you proposing?  When we discussed this before <
> https://lists.apache.org/thread.html/3e9a78a474b15d900244a86ed80358e59f4836a36e7368ae15eeeb17@%3Cdev.geode.apache.org%3E>
> nearly everyone felt that “3” was way too high but also some people even
> thought “1” was too high.  The consensus then was to leave it up to the PR
> author how many reviews, if any, they needed to feel comfortable.
>
> It looks like any committer can bypass there GitHub safeguards if they
> wish just by committing directly to develop, so I guess that this proposal
> remains compatible with “people over process”.
>
> -Owen
>
> > On Oct 18, 2019, at 4:11 PM, Alexander Murmann <amurmann@apache.org>
> wrote:
> >
> > +1 for the 👶 steps proposal
> >
> > On Fri, Oct 18, 2019 at 3:30 PM Donal Evans <doevans@pivotal.io> wrote:
> >
> >> Big +1 from me on the revised proposal. It seems like there'd rarely be
> >> a case where something needs to be merged so fast that we can't even
> wait
> >> for the build and unit tests to pass, and preventing the addition of
> flaky
> >> tests by running the StressNewTest might help address the apparent
> trend
> >> of increase in flakiness thats been talked about.
> >>
> >> On Fri, Oct 18, 2019 at 3:15 PM Nabarun Nag <nnag@apache.org> wrote:
> >>
> >>> @Bruce Schuchardt <bschuchardt@pivotal.io>
> >>> I completely agree with that assessment that not all flaky tests are
> >>> related to the test but may involve issues with the product itself.
> >>>
> >>> @Ernie
> >>> As you can see with the example that you provided, even when committers
> >> are
> >>> expected to do their due diligence they sometimes end up doing
> something
> >>> that needs to be reverted.
> >>>
> >>> It's ok to have some safeguards. I plan to look at them like seatbelts
> >> in a
> >>> car, even when we are diligent, unexpected things happen.
> >>>
> >>> My intention with this email was *never* to blame others / point out
> PRs
> >>> that broke the build, etc.
> >>> I want guard rails that will help even me from making mistakes.
> >>>
> >>> I understand that the consensus is that distributed/integration/upgrade
> >>> tests at the moment are flaky, hence blocking the merge button because
> of
> >>> them will not be an efficient call.
> >>>
> >>> *NEW PROPOSAL*: baby steps.
> >>> *Github's Branch Protection Rule *has the following rule settings that
> I
> >>> propose to activate:
> >>> - Require pull request reviews before merging
> >>> - Require status checks to pass before merging
> >>>     [Only for
> >>>                - concourse-ci/Build
> >>>               - concourse-ci/UnitTestOpenJDK11
> >>>               - concourse-ci/UnitTestOpenJDK8
> >>>               - concourse-ci/StressNewTestOpenJDK11]
> >>>
> >>> *Advantages:*
> >>> - Prevent us from accidentally merging PRs without reviews, ensure that
> >> we
> >>> follow *the Apache way* of involving the community in what code is
> going
> >>> into the repo.
> >>> - Our build is not flaky, hence it helps us in avoiding merging code
> >> which
> >>> will break the build
> >>> - Avoid mistakenly merging spotless errors
> >>> - Unit tests are not flaky hence they can be included
> >>> - Any new tests that are being added can't be merged if they fail the
> >>> stress test.
> >>>
> >>>
> >>> Apache values people over process, I highly appreciate that sentiment
> >> but
> >>> they never said not to take help from tools to save time and avoid
> >>> mistakes.
> >>>
> >>> If we search for this feature in INFRA tickets, a lot of Apache
> projects
> >>> have enabled this feature for their repositories.
> >>>
> >>> Regards
> >>> Naba
> >>>
> >>>
> >>>
> >>> On Fri, Oct 18, 2019 at 1:39 PM Bruce Schuchardt <
> bschuchardt@pivotal.io
> >>>
> >>> wrote:
> >>>
> >>>> Given the current state of affairs I don't think we should disable the
> >>>> merge button.
> >>>>
> >>>> Ultimately it would be nice if we fixed all of the flaky tests.
> >>>> Personally I don't think all of the tests that we think are "flaky"
> are
> >>>> test problems.  In the last few months I've fixed product problems
> that
> >>>> were hit by supposedly "flaky" tests.  Those tests were just unable
to
> >>>> reproduce the product problem 100% of the time.  A flickering test
> >>>> doesn't always mean it's a problem with the test.
> >>>>
> >>>> On 10/18/19 12:46 PM, Ernest Burghardt wrote:
> >>>>> I had one recently that was Approved and I merged pre-maturely and
> >> had
> >>> to
> >>>>> be reverted: d63638e4654bc6c71a232838b745dec6ef476ec9
> >>>>>
> >>>>> Subsequently I have run into some test flakiness, but if a PR
> >> submitter
> >>>> has
> >>>>> a pre-checkin failure it could be tricky to tell that its a Flaky
> >>>>> situation... In my last go at a Flaky failure in pre-checkin, I
was
> >>> able
> >>>> to
> >>>>> search the Geode Jira and found the failure was a known flaky like
> >> this
> >>>> one
> >>>>> <https://issues.apache.org/jira/browse/GEODE-6324>
> >>>>>
> >>>>> I'd prefer to trust our committers to perform their due diligence
and
> >>>> make
> >>>>> good choices.
> >>>>>
> >>>>> EB
> >>>>>
> >>>>> On Fri, Oct 18, 2019 at 12:18 PM Owen Nichols <onichols@pivotal.io>
> >>>> wrote:
> >>>>>
> >>>>>> Do you have a recent example of a PR that was merged despite
failed
> >> PR
> >>>>>> checks, which then broke the build?
> >>>>>>
> >>>>>> At last discussion, one concern raised was providing a way that
> >> anyone
> >>>> in
> >>>>>> the community could re-trigger a failed PR check if it hit an
> >>> unrelated
> >>>>>> flaky failure.
> >>>>>>
> >>>>>> Let’s be sure we've identified the problem before assuming
the
> >>> solution.
> >>>>>> Apache values people over process.
> >>>>>>
> >>>>>>> On Oct 18, 2019, at 11:48 AM, Nabarun Nag <nnag@apache.org>
wrote:
> >>>>>>>
> >>>>>>> Hi devs,
> >>>>>>>
> >>>>>>> A few months ago a proposal was brought up regarding blocking
the
> >>> merge
> >>>>>>> button on the github PR page in case of failing tests in
the
> >>> precheck.
> >>>>>>>
> >>>>>>> What is the sentiment regarding this now? Do we feel that
it should
> >>> be
> >>>>>>> implemented?
> >>>>>>>
> >>>>>>> Or at least take the minimal step of not allowing merge
till all
> >>> tests
> >>>>>> are
> >>>>>>> done?
> >>>>>>>
> >>>>>>>
> >>>>>>> Regards
> >>>>>>> Nabarun Nag
> >>>>>>
> >>>>
> >>>
> >>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message