geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Ross <br...@pivotal.io>
Subject Re: [DISCUSS] Blocking merge button in PR
Date Mon, 21 Oct 2019 19:13:45 GMT
+1

On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer <udo@apache.com> wrote:

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

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