+1 @Naba thanks for seeing this through! On Mon, Oct 21, 2019 at 1:51 PM Nabarun Nag <n...@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 <alind...@pivotal.io> > wrote: > >> +1 >> >> - Aaron >> >> >> On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer <u...@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 < >> amurm...@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 <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> >> > >>>>>> >> > >> >
