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