geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nabarun Nag <n...@apache.org>
Subject Re: [DISCUSS] Blocking merge button in PR
Date Tue, 22 Oct 2019 15:41:01 GMT
Darrel,

In my opinion, Stress new tests is one of the important test suites that
needs to be activated. This is only test suite that can prevent the influx
of flaky tests. We have seen a heavy rise in the number of tickets being
created recently.

Old tests can be avoided from running in the suite for the PR by keeping it
unchanged. A separate new ticket can be created with a refactoring task and
that PR should handle the flaky ness of the test along with the refactoring.

Regards
Naba





On Tue, Oct 22, 2019 at 8:34 AM Darrel Schneider <dschneider@pivotal.io>
wrote:

> I would advise against including "StressNewTestOpenJDK11".
> I have had trouble with this one when doing something as simple as a method
> rename.
> Because that method was called from an old test, StressNewTest ran that old
> test many times. Not all of our current tests can handle being rerun many
> times. If all you are trying to do in a pull request is something simple
> you should not be required to rework a test to survive StressNewTest.
> If StressNewTest was changed to only run brand new test files then I would
> be okay with it gating a PR merge.
>
> On Mon, Oct 21, 2019 at 4:41 PM Bruce Schuchardt <bschuchardt@pivotal.io>
> wrote:
>
> > I'd still like to see the PR rerun tool or an equivalent available to
> > everyone.  Sure you can push an empty commit but that reruns everything,
> > but the PR tool lets you rerun only the checks that failed.
> >
> > On 10/21/19 3:04 PM, Ernest Burghardt wrote:
> > > +1
> > > @Naba thanks for seeing this through!
> > >
> > > On Mon, Oct 21, 2019 at 1:51 PM Nabarun Nag <nnag@apache.org> wrote:
> > >
> > >> Thank you all for all the valuable feedback. Robert was kind enough to
> > >> check the technical feasibility of the integration of Github Branch
> > >> Protection Rules and its compatibility with our concourse CI checks
> and
> > we
> > >> are satisfied with the settings provided and the initial tests that
> > Robert
> > >> did with a demo geode branch. Please find attached the screenshot of
> the
> > >> settings that will be sent to INFRA for enabling it on the Apache
> Geode
> > >> repository.
> > >>
> > >> Regards
> > >> Nabarun
> > >>
> > >> On Mon, Oct 21, 2019 at 12:21 PM Aaron Lindsey <alindsey@pivotal.io>
> > >> wrote:
> > >>
> > >>> +1
> > >>>
> > >>> - Aaron
> > >>>
> > >>>
> > >>> On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer <udo@apache.com>
> wrote:
> > >>>
> > >>>> BIG +1 (Yes, I'm changing my -1)
> > >>>>
> > >>>> @Naba, thank you for the offline chat. It seems that the proposal
of
> > >>>> Github enforcing our good practices is a great option.
> > >>>>
> > >>>> 2 merged PR's without a full green pipeline, since 18 Oct 2019,
> > shocking
> > >>>> and disgusting!!
> > >>>>
> > >>>> I would just like to reiterate to ALL committers, you have a
> > >>>> responsibility towards the project to commit/merge only the best
> code
> > >>>> possible. If things break, fix them. Don't merge and hope that
it
> goes
> > >>>> away and someone else fixes it.
> > >>>>
> > >>>> I really don't want to support a mechanism where the project has
> > become
> > >>>> so prescriptive and restrictive, all because we have a few
> committers
> > >>>> who will not follow the established and agreed processes. BUT,
once
> > >>>> again, the masses are impacted due to a few.
> > >>>>
> > >>>> Thank you Naba for following this through.
> > >>>>
> > >>>> --Udo
> > >>>>
> > >>>> On 10/21/19 11:05 AM, Nabarun Nag wrote:
> > >>>>> @Karen
> > >>>>>
> > >>>>> Thank you so much for the feedback. I understand that committers
> must
> > >>>> trust
> > >>>>> each other and I agree entirely with that. This proposal is
just
> > >>>> preventing
> > >>>>> us from making mistakes. Its just guard rails. As you can see
in
> the
> > >>>> email
> > >>>>> chain that this mistake was made multiple times and this has
let
> to a
> > >>> lot
> > >>>>> of engineers give up their time and detecting and fixing this
> issue.
> > >>> We
> > >>>>> still trust our committers, we are just enabling some features
to
> > >>> avoid
> > >>>>> time being wasted on avoidable mistakes and use that time in
> > >>>>> improving Geode. I hope I can persuade you to change your opinion.
> > >>>>>
> > >>>>> @Owen
> > >>>>> Thank you for accepting the baby step proposal. Yes, let's
see in
> > some
> > >>>> time
> > >>>>> if this is successful. We can always undo this if needed.
> > >>>>>
> > >>>>> @Rest
> > >>>>> - Blaming people etc. will be very detrimental to the community,
I
> do
> > >>> not
> > >>>>> propose that.
> > >>>>> - This proposal was not just my idea but from feedback from
lot of
> > >>>>> developers who contribute to Geode on a frequent basis.
> > >>>>> - It is very* unfair *to the engineers to go and investigate
and
> find
> > >>> out
> > >>>>> what caused the failure and then send emails etc, their time
can be
> > >>> used
> > >>>> in
> > >>>>> doing something more valuable.
> > >>>>> - This is not something new, we have had this issue for over
3
> years
> > >>> now,
> > >>>>> and maintaining this as the status quo is not healthy. Let
us try
> > this
> > >>>>> solution, the committers, and developers who contribute on
a
> regular
> > >>>> basis
> > >>>>> will immediately see if it works or does not work and can revert
it
> > if
> > >>>> this
> > >>>>> does not.
> > >>>>> - There is a problem and this is an attempt at the solution.
If it
> > >>> does
> > >>>> not
> > >>>>> work, we can revert it. For the sake of all the developers
who are
> in
> > >>>> pain
> > >>>>> and helping them spend the time on more productive tasks, let
us
> just
> > >>>> give
> > >>>>> this proposal a chance. If there are any issues we can revert
it.
> > >>>>>
> > >>>>>
> > >>>>> *Reiterating the proposal:*
> > >>>>> Github branch protection rule for :
> > >>>>> - at least one review
> > >>>>> - Passing build, unit and stress test.
> > >>>>>
> > >>>>>
> > >>>>> In our opinion, no committer would want to check-in code with
> failing
> > >>> any
> > >>>>> of the above.
> > >>>>>
> > >>>>> Regards
> > >>>>> Nabarun
> > >>>>>
> > >>>>> On Mon, Oct 21, 2019 at 10:28 AM Alexander Murmann <
> > >>> amurmann@apache.org>
> > >>>>> wrote:
> > >>>>>
> > >>>>>> Udo, I think you are making a great point! I am fully bought
into
> > >>>> trusting
> > >>>>>> our committers to know how many reviews are needed for
their PRs.
> We
> > >>>> should
> > >>>>>> be able to have the same trust into knowing when it's OK
to merge.
> > >>> If a
> > >>>>>> committer wants to they can after all, always merge and
push
> without
> > >>> a
> > >>>> PR.
> > >>>>>> That's because we trust them.
> > >>>>>>
> > >>>>>> If we are seeing that our committers merge without sufficient
> review
> > >>>> (via
> > >>>>>> human or automated means), is this an education problem
we can
> > solve?
> > >>>>>>
> > >>>>>> On Mon, Oct 21, 2019 at 10:18 AM Udo Kohlmeyer <udo@apache.com>
> > >>> wrote:
> > >>>>>>> I must agree with @Karen here..
> > >>>>>>>
> > >>>>>>> All committers are trusted to do the right thing. One
of those
> > >>> things
> > >>>> is
> > >>>>>>> to commit (or not commit) PR's.
> > >>>>>>>
> > >>>>>>> Now we are discussing disabling the button when tests
are
> failing.
> > >>> Why
> > >>>>>>> stop there? Why not, that the submitter of the said
commit does
> not
> > >>> get
> > >>>>>>> to merge their own PR?
> > >>>>>>>
> > >>>>>>> Now, that of course is taking it to the extreme, that
we don't
> want
> > >>> (at
> > >>>>>>> least I don't want to be THAT over prescriptive)..
So why do we
> > >>> want to
> > >>>>>>> now limit when I can merge? It remains the committers
> > >>> responsibility to
> > >>>>>>> merge commits into the project, that are of the expected
quality.
> > >>> IF it
> > >>>>>>> so happens that one, by accident, has merged a PR before
it was
> > >>> green,
> > >>>>>>> revert it. All committers have the power to do so.
> > >>>>>>>
> > >>>>>>> So from my perspective, a -1 on disabling the Merge
button, just
> > >>>> because
> > >>>>>>> someone was not careful in merging and without following
our
> > >>> protocol
> > >>>> of
> > >>>>>>> waiting for an "All green".
> > >>>>>>>
> > >>>>>>> --Udo
> > >>>>>>>
> > >>>>>>> On 10/21/19 10:11 AM, Ernest Burghardt wrote:
> > >>>>>>>> +1 to enacting this immediately... just this weekend
a PR was
> > >>> merged
> > >>>>>> with
> > >>>>>>>> failures on all of the following:
> > >>>>>>>> * concourse-ci/DistributedTestOpenJDK11 * Concourse
CI build
> > >>> failure
> > >>>>>>>> * concourse-ci/UnitTestOpenJDK11 * Concourse CI
build failure
> > >>>>>>>> * concourse-ci/UnitTestOpenJDK8 * Concourse CI
build failure
> > >>>>>>>> * concourse-ci/UpgradeTestOpenJDK11 * Concourse
CI build failure
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> Thanks,
> > >>>>>>>> EB
> > >>>>>>>>
> > >>>>>>>> On Mon, Oct 21, 2019 at 10:01 AM Karen Miller <
> kmiller@pivotal.io
> > >
> > >>>>>>> wrote:
> > >>>>>>>>> I have (more than once) committed docs changes
for typo fixes
> > >>> without
> > >>>>>>>>> review.  I generally label the commits
> > >>>>>>>>> with a bold "Commit then Review" message. 
But, I am bringing
> > >>> this up
> > >>>>>>> as I
> > >>>>>>>>> have purposely not followed what
> > >>>>>>>>> looks to be a positively-received proposed
policy, since I have
> > >>> not
> > >>>>>>> gotten
> > >>>>>>>>> reviews. If all feel that we need a
> > >>>>>>>>> rule for everyone to follow (instead of a guideline
that PRs
> > shall
> > >>>>>> have
> > >>>>>>> at
> > >>>>>>>>> least 1 review), I will follow the rule,
> > >>>>>>>>> but I'm a -0 on the process. I get it, and
I understand its
> > >>> purpose
> > >>>>>> and
> > >>>>>>>>> intent, but I personally prefer to trust that
each
> > >>>>>>>>> comitter takes personal responsibility for
the code they commit
> > >>> WRT
> > >>>>>>> waiting
> > >>>>>>>>> for tests and/or obtaining reviews.
> > >>>>>>>>> Karen
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>> On Mon, Oct 21, 2019 at 6:24 AM Joris Melchior
<
> > >>> jmelchior@pivotal.io
> > >>>>>>>>> wrote:
> > >>>>>>>>>
> > >>>>>>>>>> +1 to the revised approach. I think requiring
at least one
> > >>> review is
> > >>>>>>>>>> important. More eyes make for better code.
> > >>>>>>>>>>
> > >>>>>>>>>> Cheers, Joris.
> > >>>>>>>>>>
> > >>>>>>>>>> On Mon, Oct 21, 2019 at 8:11 AM Ju@N <jujoramos@gmail.com>
> > >>> wrote:
> > >>>>>>>>>>> +10 to Naba's proposal, it seems the
right thing to do and
> will
> > >>>> help
> > >>>>>>> us
> > >>>>>>>>>> to
> > >>>>>>>>>>> prevent accidentally breaking *develop*
while keeping focus
> on
> > >>>>>> people
> > >>>>>>>>>>> instead of processes.
> > >>>>>>>>>>> I'd add, however, that the *Merge Pull
Request* button should
> > >>>> remain
> > >>>>>>>>>>> disabled until *all CIs have finished*,
and only enable it
> once
> > >>> the
> > >>>>>>>>>> *Build,
> > >>>>>>>>>>> Unit, Stress Tests and LGTM are green
*(that is, force the
> > >>>> committer
> > >>>>>>> to
> > >>>>>>>>>>> wait at least until all CIs are done)*.
*I also agree in that
> > >>> that
> > >>>>>> we
> > >>>>>>>>>>> should require *at least one* official
approval.
> > >>>>>>>>>>> Cheers.
> > >>>>>>>>>>>
> > >>>>>>>>>> --
> > >>>>>>>>>> *Joris Melchior *
> > >>>>>>>>>> CF Engineering
> > >>>>>>>>>> Pivotal Toronto
> > >>>>>>>>>> 416 877 5427
> > >>>>>>>>>>
> > >>>>>>>>>> “Programs must be written for people
to read, and only
> > >>> incidentally
> > >>>>>> for
> > >>>>>>>>>> machines to execute.” – *Hal Abelson*
> > >>>>>>>>>> <https://en.wikipedia.org/wiki/Hal_Abelson>
> > >>>>>>>>>>
> >
>

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