geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Udo Kohlmeyer <...@apache.com>
Subject Re: [DISCUSS] Blocking merge button in PR
Date Mon, 21 Oct 2019 18:53:37 GMT
BIG +1 (Yes, I'm changing my -1)

@Naba, thank you for the offline chat. It seems that the proposal of 
Github enforcing our good practices is a great option.

2 merged PR's without a full green pipeline, since 18 Oct 2019, shocking 
and disgusting!!

I would just like to reiterate to ALL committers, you have a 
responsibility towards the project to commit/merge only the best code 
possible. If things break, fix them. Don't merge and hope that it goes 
away and someone else fixes it.

I really don't want to support a mechanism where the project has become 
so prescriptive and restrictive, all because we have a few committers 
who will not follow the established and agreed processes. BUT, once 
again, the masses are impacted due to a few.

Thank you Naba for following this through.

--Udo

On 10/21/19 11:05 AM, Nabarun Nag wrote:
> @Karen
>
> Thank you so much for the feedback. I understand that committers must trust
> each other and I agree entirely with that. This proposal is just preventing
> us from making mistakes. Its just guard rails. As you can see in the email
> chain that this mistake was made multiple times and this has let to a lot
> of engineers give up their time and detecting and fixing this issue. We
> still trust our committers, we are just enabling some features to avoid
> time being wasted on avoidable mistakes and use that time in
> improving Geode. I hope I can persuade you to change your opinion.
>
> @Owen
> Thank you for accepting the baby step proposal. Yes, let's see in some time
> if this is successful. We can always undo this if needed.
>
> @Rest
> - Blaming people etc. will be very detrimental to the community, I do not
> propose that.
> - This proposal was not just my idea but from feedback from lot of
> developers who contribute to Geode on a frequent basis.
> - It is very* unfair *to the engineers to go and investigate and find out
> what caused the failure and then send emails etc, their time can be used in
> doing something more valuable.
> - This is not something new, we have had this issue for over 3 years now,
> and maintaining this as the status quo is not healthy. Let us try this
> solution, the committers, and developers who contribute on a regular basis
> will immediately see if it works or does not work and can revert it if this
> does not.
> - There is a problem and this is an attempt at the solution. If it does not
> work, we can revert it. For the sake of all the developers who are in pain
> and helping them spend the time on more productive tasks, let us just give
> this proposal a chance. If there are any issues we can revert it.
>
>
> *Reiterating the proposal:*
> Github branch protection rule for :
> - at least one review
> - Passing build, unit and stress test.
>
>
> In our opinion, no committer would want to check-in code with failing any
> of the above.
>
> Regards
> Nabarun
>
> On Mon, Oct 21, 2019 at 10:28 AM Alexander Murmann <amurmann@apache.org>
> wrote:
>
>> Udo, I think you are making a great point! I am fully bought into trusting
>> our committers to know how many reviews are needed for their PRs. We should
>> be able to have the same trust into knowing when it's OK to merge. If a
>> committer wants to they can after all, always merge and push without a PR.
>> That's because we trust them.
>>
>> If we are seeing that our committers merge without sufficient review (via
>> human or automated means), is this an education problem we can solve?
>>
>> On Mon, Oct 21, 2019 at 10:18 AM Udo Kohlmeyer <udo@apache.com> wrote:
>>
>>> I must agree with @Karen here..
>>>
>>> All committers are trusted to do the right thing. One of those things is
>>> to commit (or not commit) PR's.
>>>
>>> Now we are discussing disabling the button when tests are failing. Why
>>> stop there? Why not, that the submitter of the said commit does not get
>>> to merge their own PR?
>>>
>>> Now, that of course is taking it to the extreme, that we don't want (at
>>> least I don't want to be THAT over prescriptive).. So why do we want to
>>> now limit when I can merge? It remains the committers responsibility to
>>> merge commits into the project, that are of the expected quality. IF it
>>> so happens that one, by accident, has merged a PR before it was green,
>>> revert it. All committers have the power to do so.
>>>
>>> So from my perspective, a -1 on disabling the Merge button, just because
>>> someone was not careful in merging and without following our protocol of
>>> waiting for an "All green".
>>>
>>> --Udo
>>>
>>> On 10/21/19 10:11 AM, Ernest Burghardt wrote:
>>>> +1 to enacting this immediately... just this weekend a PR was merged
>> with
>>>> failures on all of the following:
>>>> * concourse-ci/DistributedTestOpenJDK11 * Concourse CI build failure
>>>> * concourse-ci/UnitTestOpenJDK11 * Concourse CI build failure
>>>> * concourse-ci/UnitTestOpenJDK8 * Concourse CI build failure
>>>> * concourse-ci/UpgradeTestOpenJDK11 * Concourse CI build failure
>>>>
>>>>
>>>> Thanks,
>>>> EB
>>>>
>>>> On Mon, Oct 21, 2019 at 10:01 AM Karen Miller <kmiller@pivotal.io>
>>> wrote:
>>>>> I have (more than once) committed docs changes for typo fixes without
>>>>> review.  I generally label the commits
>>>>> with a bold "Commit then Review" message.  But, I am bringing this up
>>> as I
>>>>> have purposely not followed what
>>>>> looks to be a positively-received proposed policy, since I have not
>>> gotten
>>>>> reviews. If all feel that we need a
>>>>> rule for everyone to follow (instead of a guideline that PRs shall
>> have
>>> at
>>>>> least 1 review), I will follow the rule,
>>>>> but I'm a -0 on the process. I get it, and I understand its purpose
>> and
>>>>> intent, but I personally prefer to trust that each
>>>>> comitter takes personal responsibility for the code they commit WRT
>>> waiting
>>>>> for tests and/or obtaining reviews.
>>>>> Karen
>>>>>
>>>>>
>>>>> On Mon, Oct 21, 2019 at 6:24 AM Joris Melchior <jmelchior@pivotal.io>
>>>>> wrote:
>>>>>
>>>>>> +1 to the revised approach. I think requiring at least one review
is
>>>>>> important. More eyes make for better code.
>>>>>>
>>>>>> Cheers, Joris.
>>>>>>
>>>>>> On Mon, Oct 21, 2019 at 8:11 AM Ju@N <jujoramos@gmail.com>
wrote:
>>>>>>
>>>>>>> +10 to Naba's proposal, it seems the right thing to do and will
help
>>> us
>>>>>> to
>>>>>>> prevent accidentally breaking *develop* while keeping focus on
>> people
>>>>>>> instead of processes.
>>>>>>> I'd add, however, that the *Merge Pull Request* button should
remain
>>>>>>> disabled until *all CIs have finished*, and only enable it once
the
>>>>>> *Build,
>>>>>>> Unit, Stress Tests and LGTM are green *(that is, force the committer
>>> to
>>>>>>> wait at least until all CIs are done)*. *I also agree in that
that
>> we
>>>>>>> should require *at least one* official approval.
>>>>>>> Cheers.
>>>>>>>
>>>>>> --
>>>>>> *Joris Melchior *
>>>>>> CF Engineering
>>>>>> Pivotal Toronto
>>>>>> 416 877 5427
>>>>>>
>>>>>> “Programs must be written for people to read, and only incidentally
>> for
>>>>>> machines to execute.” – *Hal Abelson*
>>>>>> <https://en.wikipedia.org/wiki/Hal_Abelson>
>>>>>>

Mime
View raw message