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 <u...@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 <kmil...@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 <jmelch...@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 <jujora...@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> > >>> >
