You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Nabarun Nag <nn...@apache.org> on 2019/10/18 18:48:16 UTC
[DISCUSS] Blocking merge button in PR
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
Re: [DISCUSS] Blocking merge button in PR
Posted by Alexander Murmann <am...@apache.org>.
Hi Naba,
I recall that we last time decided against blocking it because we agreed
that the test suite was too flaky at the time. It's my understanding that
it has only gotten more flaky. I hope we can see an effort soon to
remediate that and at that point revisit the topic.
I don't have a strong opinion on blocking the button till tests are
completed. Given the noise in the test suite, I'd expect someone who now
merges without waiting, to merge without taking a close look at failed
tests. So to me the place to get to is the reliable test suite.
On Fri, Oct 18, 2019 at 11:48 AM Nabarun Nag <nn...@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
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Ernest Burghardt <eb...@pivotal.io>.
+1 to Naba's proposal, I appreciate helpful tools
On Fri, Oct 18, 2019 at 3:15 PM Nabarun Nag <nn...@apache.org> wrote:
> @Bruce Schuchardt <bs...@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 <bs...@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 <on...@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 <nn...@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
> > >>
> >
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Owen Nichols <on...@pivotal.io>.
"Apache asserts that a healthy community is a higher priority than good code. Strong communities can always rectify problems with their code, whereas an unhealthy community will likely struggle to maintain a codebase in a sustainable manner.” —apache.org/theapacheway <http://www.apache.org/theapacheway/>
Do the technical solutions we are proposing help make our community tighter, or do they have the opposite effect?
On one hand, a strong community must trust each other to do the right thing (e.g. vet before merge, but also monitor after merge).
On the other hand, requiring at least one review might force encourage a little more community togetherness.
Let’s try this “baby steps” proposal, and follow up in a few months to reflect on this experiment.
> On Oct 21, 2019, at 10:00 AM, Karen Miller <km...@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 <jm...@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 <ju...@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>
>>
Re: [DISCUSS] Blocking merge button in PR
Posted by Udo Kohlmeyer <ud...@apache.com>.
@Robert Houghton,
Tbh honest, if we find that we have committers that are NOT following
procedure/protocol, then there is a PMC that this needs to be escalated to.
IF said committer is willfully acting in a negative manner towards the
project, I am fully supportive of removing the rights and privileges for
that committer, maybe from the project in totality.
But if this is merely an educational effort then that will be provided
as well.
But we all know that we can employ as many defensive mechanisms as we'd
like, but in the end we are only end up hurting the project. As all the
extra defensive red tape will eventually inhibit the project. That is
how all bureaucracy starts.. small innocent.. and eventually it becomes
this heavy beast that no-one can master.
--Udo
On 10/21/19 10:30 AM, Robert Houghton wrote:
> @Udo Kohlmeyer <ma...@pivotal.io> What, if anything, do
> you propose we do to help keep our project building and running
> cleanly that does not force punitive or coercive behavior on our
> developers? "Naming names" or "shaming" are not popular choices, and
> everyone on the comitters list *should* follow procedure, but doesn't.
>
> What would you do?
>
> On Mon, Oct 21, 2019 at 10:18 AM Udo Kohlmeyer <udo@apache.com
> <ma...@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 <ma...@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 <ma...@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
> <ma...@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>
> >>>
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Udo Kohlmeyer <ud...@apache.com>.
All committers have to right to commit and all committers have the right
to revert.
It is ALL for the good of the project. Now, of course, I imagine we
won't apply scorched earth policies, but every committers should feel
empowered to be able to revert a bad merge, if it hindering the progress
and health of the project.
Of course, contacting the original committer to revert the merge, is
preferable, but sometimes things take too long and a kindly worded email
to the committer, letting them know the merge was reverted and for what
reason is all that is really required in the end.
--Udo
On 10/21/19 10:41 AM, Owen Nichols wrote:
> One perspective to consider is, should anyone be able to revert a bad commit, or only the original committer? If we feel individual ownership of commits, we might hesitate to revert someone else’s commit, even if it broke the build. However if we feel a strong sense of community, we might be ok with an anyone-can-revert policy. A revert does not have to imply shaming...
>
>
>> On Oct 21, 2019, at 10:30 AM, Robert Houghton <rh...@pivotal.io> wrote:
>>
>> @Udo Kohlmeyer <uk...@pivotal.io> What, if anything, do you propose we
>> do to help keep our project building and running cleanly that does not
>> force punitive or coercive behavior on our developers? "Naming names" or
>> "shaming" are not popular choices, and everyone on the comitters list
>> *should* follow procedure, but doesn't.
>>
>> What would you do?
>>
>> On Mon, Oct 21, 2019 at 10:18 AM Udo Kohlmeyer <ud...@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 <km...@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 <jm...@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 <ju...@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>
>>>>>>
Re: [DISCUSS] Blocking merge button in PR
Posted by Owen Nichols <on...@pivotal.io>.
One perspective to consider is, should anyone be able to revert a bad commit, or only the original committer? If we feel individual ownership of commits, we might hesitate to revert someone else’s commit, even if it broke the build. However if we feel a strong sense of community, we might be ok with an anyone-can-revert policy. A revert does not have to imply shaming...
> On Oct 21, 2019, at 10:30 AM, Robert Houghton <rh...@pivotal.io> wrote:
>
> @Udo Kohlmeyer <uk...@pivotal.io> What, if anything, do you propose we
> do to help keep our project building and running cleanly that does not
> force punitive or coercive behavior on our developers? "Naming names" or
> "shaming" are not popular choices, and everyone on the comitters list
> *should* follow procedure, but doesn't.
>
> What would you do?
>
> On Mon, Oct 21, 2019 at 10:18 AM Udo Kohlmeyer <ud...@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 <km...@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 <jm...@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 <ju...@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>
>>>>>
>>
Re: [DISCUSS] Blocking merge button in PR
Posted by Robert Houghton <rh...@pivotal.io>.
@Udo Kohlmeyer <uk...@pivotal.io> What, if anything, do you propose we
do to help keep our project building and running cleanly that does not
force punitive or coercive behavior on our developers? "Naming names" or
"shaming" are not popular choices, and everyone on the comitters list
*should* follow procedure, but doesn't.
What would you do?
On Mon, Oct 21, 2019 at 10:18 AM Udo Kohlmeyer <ud...@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 <km...@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 <jm...@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 <ju...@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>
> >>>
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Helena Bales <hb...@pivotal.io>.
To Bruce's point:
I don't see stress new test as something that's supposed to be easy to
pass. I do agree that it should be easy to figure out what went wrong, but
the point of this task was the catch issues with tests being flaky or
non-repeatable for other reasons. If we reduce the scope of this to only
test newly added test classes, we won't catch the case where someone adds a
new test method that is flaky, or the case where modifying a test method
makes it flaky. Both of these things commonly happen in our code base.
Regarding tests that can't be repeated, this is because they are using an
old test base or pollute their environment. I see both of these as reasons
to go modify the test to make it work properly. Some tests are a lot of
work to change, but having done this many times with tests that I've
touched, I believe it is worthwhile. If we are touching a test class that
does not meet our current standards for testing, then it should be
modernized. If only for the purpose of reducing the flakiness in our
pipelines.
In short, while stressNewTest is a pain, the current design of this tool
serves several purposes, which I believe to be worth the effort of
modernizing outdated tests when we encounter them.
On Tue, Oct 22, 2019 at 11:25 AM Bruce Schuchardt <bs...@pivotal.io>
wrote:
> I also have my doubts about StressNewTest testing being required to
> pass. Maybe if they just tested new tests instead of anything your
> changes happened to touch it would be easier to pass this check.
>
> Also, the StressNewTest check runs the selected tests in parallel and
> apparently in the same environment. Any time I've tried to use
> artifacts from one of these runs to figure out what went wrong I found
> them to be useless because of the interleaving of output.
>
>
> On 10/22/19 8:33 AM, Darrel Schneider 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 <nn...@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 <al...@pivotal.io>
> >>>> wrote:
> >>>>
> >>>>> +1
> >>>>>
> >>>>> - Aaron
> >>>>>
> >>>>>
> >>>>> On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer <ud...@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 <ud...@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 <ju...@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>
> >>>>>>>>>>>>
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Bruce Schuchardt <bs...@pivotal.io>.
I also have my doubts about StressNewTest testing being required to
pass. Maybe if they just tested new tests instead of anything your
changes happened to touch it would be easier to pass this check.
Also, the StressNewTest check runs the selected tests in parallel and
apparently in the same environment. Any time I've tried to use
artifacts from one of these runs to figure out what went wrong I found
them to be useless because of the interleaving of output.
On 10/22/19 8:33 AM, Darrel Schneider 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 <bs...@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 <nn...@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 <al...@pivotal.io>
>>>> wrote:
>>>>
>>>>> +1
>>>>>
>>>>> - Aaron
>>>>>
>>>>>
>>>>> On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer <ud...@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 <ud...@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 <ju...@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>
>>>>>>>>>>>>
Re: [DISCUSS] Blocking merge button in PR
Posted by Nabarun Nag <nn...@apache.org>.
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 <ds...@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 <bs...@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 <nn...@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 <al...@pivotal.io>
> > >> wrote:
> > >>
> > >>> +1
> > >>>
> > >>> - Aaron
> > >>>
> > >>>
> > >>> On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer <ud...@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 <ud...@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 <ju...@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>
> > >>>>>>>>>>
> >
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Darrel Schneider <ds...@pivotal.io>.
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 <bs...@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 <nn...@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 <al...@pivotal.io>
> >> wrote:
> >>
> >>> +1
> >>>
> >>> - Aaron
> >>>
> >>>
> >>> On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer <ud...@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 <ud...@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 <ju...@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>
> >>>>>>>>>>
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Bruce Schuchardt <bs...@pivotal.io>.
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 <nn...@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 <al...@pivotal.io>
>> wrote:
>>
>>> +1
>>>
>>> - Aaron
>>>
>>>
>>> On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer <ud...@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 <ud...@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 <km...@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 <ju...@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>
>>>>>>>>>>
Re: [DISCUSS] Blocking merge button in PR
Posted by Ernest Burghardt <eb...@pivotal.io>.
+1
@Naba thanks for seeing this through!
On Mon, Oct 21, 2019 at 1:51 PM Nabarun Nag <nn...@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 <al...@pivotal.io>
> wrote:
>
>> +1
>>
>> - Aaron
>>
>>
>> On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer <ud...@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 <ud...@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 <km...@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 <ju...@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>
>> > >>>>>>
>> >
>>
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Nabarun Nag <nn...@apache.org>.
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 <al...@pivotal.io> wrote:
> +1
>
> - Aaron
>
>
> On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer <ud...@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 <ud...@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 <km...@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 <ju...@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>
> > >>>>>>
> >
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Aaron Lindsey <al...@pivotal.io>.
+1
- Aaron
On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer <ud...@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 <am...@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 <ud...@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 <km...@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 <ju...@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>
> >>>>>>
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Benjamin Ross <br...@pivotal.io>.
+1
On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer <ud...@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 <am...@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 <ud...@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 <km...@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 <ju...@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>
> >>>>>>
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Udo Kohlmeyer <ud...@apache.com>.
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 <am...@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 <ud...@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 <km...@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 <jm...@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 <ju...@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>
>>>>>>
Re: [DISCUSS] Blocking merge button in PR
Posted by Udo Kohlmeyer <ud...@apache.com>.
@owen, whilst you are technically correct, THIS is not the process that
we as a Geode committer community have agreed upon.
The commit process within GEODE is to raise a PR. Simple...
--Udo
On 10/22/19 9:53 AM, Owen Nichols wrote:
> This discussion has revolved around the assumption that all changes go through the PR process.
>
> If you’re a committer, nothing forces you to create a PR — you can also just commit directly to develop. PRs are commonly used when the committer wants feedback (from the PR checks and/or from the community), while changes to docs and tools are sometimes made directly on develop.
>
> By making it harder to use the PR process, will this have the unintended side-effect of nudging more committers to skip it entirely?
>
>
>
>> On Oct 21, 2019, at 11:38 AM, Anthony Baker <ab...@pivotal.io> wrote:
>>
>> +1, very well said
>>
>> Anthony
>>
>>> On Oct 21, 2019, at 11:05 AM, Nabarun Nag <nn...@apache.org> wrote:
>>>
>>>
>>>
>>> *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
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Nabarun Nag <nn...@apache.org>.
Owen ,
Or any other developers *please do not push directly to Apache Geode
develop code base without creating a pull request, asking for reviews and
testing the code*.
To rephrase, what you are saying is that now developers, in order to avoid
reviews and testing their code, will now push directly to develop. I don't
think anyone will be that sinister.
There is no harm in asking for reviews for even trivial things like docs
and tools. A second pair of eyes will not cause extra harm.
I will try getting the infra ticket created by EOD.
Regards
Naba
On Tue, Oct 22, 2019 at 9:53 AM Owen Nichols <on...@pivotal.io> wrote:
> This discussion has revolved around the assumption that all changes go
> through the PR process.
>
> If you’re a committer, nothing forces you to create a PR — you can also
> just commit directly to develop. PRs are commonly used when the committer
> wants feedback (from the PR checks and/or from the community), while
> changes to docs and tools are sometimes made directly on develop.
>
> By making it harder to use the PR process, will this have the unintended
> side-effect of nudging more committers to skip it entirely?
>
>
>
> > On Oct 21, 2019, at 11:38 AM, Anthony Baker <ab...@pivotal.io> wrote:
> >
> > +1, very well said
> >
> > Anthony
> >
> >> On Oct 21, 2019, at 11:05 AM, Nabarun Nag <nn...@apache.org> wrote:
> >>
> >>
> >>
> >> *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
> >
>
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Owen Nichols <on...@pivotal.io>.
This discussion has revolved around the assumption that all changes go through the PR process.
If you’re a committer, nothing forces you to create a PR — you can also just commit directly to develop. PRs are commonly used when the committer wants feedback (from the PR checks and/or from the community), while changes to docs and tools are sometimes made directly on develop.
By making it harder to use the PR process, will this have the unintended side-effect of nudging more committers to skip it entirely?
> On Oct 21, 2019, at 11:38 AM, Anthony Baker <ab...@pivotal.io> wrote:
>
> +1, very well said
>
> Anthony
>
>> On Oct 21, 2019, at 11:05 AM, Nabarun Nag <nn...@apache.org> wrote:
>>
>>
>>
>> *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
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Anthony Baker <ab...@pivotal.io>.
+1, very well said
Anthony
> On Oct 21, 2019, at 11:05 AM, Nabarun Nag <nn...@apache.org> wrote:
>
>
>
> *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
Re: [DISCUSS] Blocking merge button in PR
Posted by Nabarun Nag <nn...@apache.org>.
@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 <am...@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 <ud...@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 <km...@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 <jm...@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 <ju...@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>
> > >>>
> >
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Alexander Murmann <am...@apache.org>.
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 <ud...@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 <km...@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 <jm...@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 <ju...@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>
> >>>
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Udo Kohlmeyer <ud...@apache.com>.
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 <km...@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 <jm...@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 <ju...@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>
>>>
Re: [DISCUSS] Blocking merge button in PR
Posted by Ernest Burghardt <eb...@pivotal.io>.
+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 <km...@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 <jm...@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 <ju...@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>
> >
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Karen Miller <km...@pivotal.io>.
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 <jm...@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 <ju...@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>
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Joris Melchior <jm...@pivotal.io>.
+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 <ju...@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>
Re: [DISCUSS] Blocking merge button in PR
Posted by Owen Nichols <on...@pivotal.io>.
I *really* like the idea of requiring all PR checks to have *completed* before merge. Does anyone know if this GitHub offers a knob for this?
> On Oct 21, 2019, at 4:53 AM, Ju@N <ju...@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.
Re: [DISCUSS] Blocking merge button in PR
Posted by "Ju@N" <ju...@gmail.com>.
+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.
Re: [DISCUSS] Blocking merge button in PR
Posted by Nabarun Nag <nn...@pivotal.io>.
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 <on...@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 <am...@apache.org>
> wrote:
> >
> > +1 for the 👶 steps proposal
> >
> > On Fri, Oct 18, 2019 at 3:30 PM Donal Evans <do...@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 <nn...@apache.org> wrote:
> >>
> >>> @Bruce Schuchardt <bs...@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 <on...@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 <nn...@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
> >>>>>>
> >>>>
> >>>
> >>
>
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Owen Nichols <on...@pivotal.io>.
+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 <am...@apache.org> wrote:
>
> +1 for the 👶 steps proposal
>
> On Fri, Oct 18, 2019 at 3:30 PM Donal Evans <do...@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 <nn...@apache.org> wrote:
>>
>>> @Bruce Schuchardt <bs...@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 <on...@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 <nn...@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
>>>>>>
>>>>
>>>
>>
Re: [DISCUSS] Blocking merge button in PR
Posted by Alexander Murmann <am...@apache.org>.
+1 for the 👶 steps proposal
On Fri, Oct 18, 2019 at 3:30 PM Donal Evans <do...@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 <nn...@apache.org> wrote:
>
> > @Bruce Schuchardt <bs...@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 <on...@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 <nn...@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
> > > >>
> > >
> >
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Donal Evans <do...@pivotal.io>.
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 <nn...@apache.org> wrote:
> @Bruce Schuchardt <bs...@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 <bs...@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 <on...@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 <nn...@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
> > >>
> >
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Nabarun Nag <nn...@apache.org>.
@Bruce Schuchardt <bs...@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 <bs...@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 <on...@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 <nn...@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
> >>
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Bruce Schuchardt <bs...@pivotal.io>.
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 <on...@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 <nn...@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
>>
Re: [DISCUSS] Blocking merge button in PR
Posted by Ernest Burghardt <eb...@pivotal.io>.
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 <on...@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 <nn...@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
>
>
Re: [DISCUSS] Blocking merge button in PR
Posted by Owen Nichols <on...@pivotal.io>.
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 <nn...@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