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> > > >>>>>> > > >
