Darrel, In my opinion, Stress new tests is one of the important test suites that needs to be activated. This is only test suite that can prevent the influx of flaky tests. We have seen a heavy rise in the number of tickets being created recently.
Old tests can be avoided from running in the suite for the PR by keeping it unchanged. A separate new ticket can be created with a refactoring task and that PR should handle the flaky ness of the test along with the refactoring. Regards Naba On Tue, Oct 22, 2019 at 8:34 AM Darrel Schneider <dschnei...@pivotal.io> wrote: > I would advise against including "StressNewTestOpenJDK11". > I have had trouble with this one when doing something as simple as a method > rename. > Because that method was called from an old test, StressNewTest ran that old > test many times. Not all of our current tests can handle being rerun many > times. If all you are trying to do in a pull request is something simple > you should not be required to rework a test to survive StressNewTest. > If StressNewTest was changed to only run brand new test files then I would > be okay with it gating a PR merge. > > On Mon, Oct 21, 2019 at 4:41 PM Bruce Schuchardt <bschucha...@pivotal.io> > wrote: > > > I'd still like to see the PR rerun tool or an equivalent available to > > everyone. Sure you can push an empty commit but that reruns everything, > > but the PR tool lets you rerun only the checks that failed. > > > > On 10/21/19 3:04 PM, Ernest Burghardt wrote: > > > +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> > > >>>>>>>>>> > > >
