Re: [DISCUSS] Blocking merge button in PR

@owen, whilst you are technically correct, THIS is not the process that we as a Geode committer community have agreed upon. The commit process within GEODE is to raise a PR. Simple... --Udo On 10/22/19 9:53 AM, Owen Nichols wrote: This discussion has revolved around the assumption that all c

Re: [DISCUSS] Blocking merge button in PR

To Bruce's point: I don't see stress new test as something that's supposed to be easy to pass. I do agree that it should be easy to figure out what went wrong, but the point of this task was the catch issues with tests being flaky or non-repeatable for other reasons. If we reduce the scope of this

Re: [DISCUSS] Blocking merge button in PR

I also have my doubts about StressNewTest testing being required to pass.  Maybe if they just tested new tests instead of anything your changes happened to touch it would be easier to pass this check. Also, the StressNewTest check runs the selected tests in parallel and apparently in the same

Re: [DISCUSS] Blocking merge button in PR

Owen , Or any other developers *please do not push directly to Apache Geode develop code base without creating a pull request, asking for reviews and testing the code*. To rephrase, what you are saying is that now developers, in order to avoid reviews and testing their code, will now push directly

Re: [DISCUSS] Blocking merge button in PR

This discussion has revolved around the assumption that all changes go through the PR process. If you’re a committer, nothing forces you to create a PR — you can also just commit directly to develop. PRs are commonly used when the committer wants feedback (from the PR checks and/or from the co

Re: [DISCUSS] Blocking merge button in PR

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

Re: [DISCUSS] Blocking merge button in PR

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.

Re: [DISCUSS] Blocking merge button in PR

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 M

Re: [DISCUSS] Blocking merge button in PR

+1 @Naba thanks for seeing this through! On Mon, Oct 21, 2019 at 1:51 PM Nabarun Nag 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 C

Re: [DISCUSS] Blocking merge button in PR

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

Re: [DISCUSS] Blocking merge button in PR

+1 - Aaron On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer 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 20

Re: [DISCUSS] Blocking merge button in PR

+1 On Mon, Oct 21, 2019 at 11:53 AM Udo Kohlmeyer 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, shocki

Re: [DISCUSS] Blocking merge button in PR

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 commit

Re: [DISCUSS] Blocking merge button in PR

+1, very well said Anthony > On Oct 21, 2019, at 11:05 AM, Nabarun Nag wrote: > > > > *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 failin

Re: [DISCUSS] Blocking merge button in PR

@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

Re: [DISCUSS] Blocking merge button in PR

All committers have to right to commit and all committers have the right to revert. It is ALL for the good of the project. Now, of course, I imagine we won't apply scorched earth policies, but every committers should feel empowered to be able to revert a bad merge, if it hindering the progress

Re: [DISCUSS] Blocking merge button in PR

@Robert Houghton, Tbh honest, if we find that we have committers that are NOT following procedure/protocol, then there is a PMC that this needs to be escalated to. IF said committer is willfully acting in a negative manner towards the project, I am fully supportive of removing the rights and

Re: [DISCUSS] Blocking merge button in PR

One perspective to consider is, should anyone be able to revert a bad commit, or only the original committer? If we feel individual ownership of commits, we might hesitate to revert someone else’s commit, even if it broke the build. However if we feel a strong sense of community, we might be o

Re: [DISCUSS] Blocking merge button in PR

I *really* like the idea of requiring all PR checks to have *completed* before merge. Does anyone know if this GitHub offers a knob for this? > On Oct 21, 2019, at 4:53 AM, Ju@N wrote: > > +10 to Naba's proposal, it seems the right thing to do and will help us to > prevent accidentally breakin

Re: [DISCUSS] Blocking merge button in PR

@Udo Kohlmeyer What, if anything, do you propose we do to help keep our project building and running cleanly that does not force punitive or coercive behavior on our developers? "Naming names" or "shaming" are not popular choices, and everyone on the comitters list *should* follow procedure, but d

Re: [DISCUSS] Blocking merge button in PR

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

Re: [DISCUSS] Blocking merge button in PR

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 the

Re: [DISCUSS] Blocking merge button in PR

"Apache asserts that a healthy community is a higher priority than good code. Strong communities can always rectify problems with their code, whereas an unhealthy community will likely struggle to maintain a codebase in a sustainable manner.” —apache.org/theapacheway

Re: [DISCUSS] Blocking merge button in PR

+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

Re: [DISCUSS] Blocking merge button in PR

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

Re: [DISCUSS] Blocking merge button in PR

+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 wrote: > +10 to Naba's proposal, it seems the right thing to do and will help us to > prevent accidentally breaking *develop* while

Re: [DISCUSS] Blocking merge button in PR

+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

Re: [DISCUSS] Blocking merge button in PR

No one felt one review was too much. At least one is bare minimum. Regards Naba On Sat, Oct 19, 2019 at 11:33 AM Owen Nichols wrote: > +1 for starting with only Build, Unit, and Stress tests. > > When you say you propose to "require pull request reviews" before merging, > what number of review

Re: [DISCUSS] Blocking merge button in PR

+1 for starting with only Build, Unit, and Stress tests. When you say you propose to "require pull request reviews" before merging, what number of reviews are you proposing? When we discussed this before

Re: [DISCUSS] Blocking merge button in PR

+1 for the 👶 steps proposal On Fri, Oct 18, 2019 at 3:30 PM Donal Evans wrote: > Big +1 from me on the revised proposal. It seems like there'd rarely be > a case where something needs to be merged so fast that we can't even wait > for the build and unit tests to pass, and preventing the addition

Re: [DISCUSS] Blocking merge button in PR

Big +1 from me on the revised proposal. It seems like there'd rarely be a case where something needs to be merged so fast that we can't even wait for the build and unit tests to pass, and preventing the addition of flaky tests by running the StressNewTest might help address the apparent trend of i

Re: [DISCUSS] Blocking merge button in PR

+1 to Naba's proposal, I appreciate helpful tools On Fri, Oct 18, 2019 at 3:15 PM Nabarun Nag wrote: > @Bruce Schuchardt > I completely agree with that assessment that not all flaky tests are > related to the test but may involve issues with the product itself. > > @Ernie > As you can see with

Re: [DISCUSS] Blocking merge button in PR

@Bruce Schuchardt I completely agree with that assessment that not all flaky tests are related to the test but may involve issues with the product itself. @Ernie As you can see with the example that you provided, even when committers are expected to do their due diligence they sometimes end up do

Re: [DISCUSS] Blocking merge button in PR

Given the current state of affairs I don't think we should disable the merge button. Ultimately it would be nice if we fixed all of the flaky tests. Personally I don't think all of the tests that we think are "flaky" are test problems.  In the last few months I've fixed product problems that

Re: [DISCUSS] Blocking merge button in PR

I had one recently that was Approved and I merged pre-maturely and had to be reverted: d63638e4654bc6c71a232838b745dec6ef476ec9 Subsequently I have run into some test flakiness, but if a PR submitter has a pre-checkin failure it could be tricky to tell that its a Flaky situation... In my last go a

Re: [DISCUSS] Blocking merge button in PR

Do you have a recent example of a PR that was merged despite failed PR checks, which then broke the build? At last discussion, one concern raised was providing a way that anyone in the community could re-trigger a failed PR check if it hit an unrelated flaky failure. Let’s be sure we've identi

Re: [DISCUSS] Blocking merge button in PR

Hi Naba, I recall that we last time decided against blocking it because we agreed that the test suite was too flaky at the time. It's my understanding that it has only gotten more flaky. I hope we can see an effort soon to remediate that and at that point revisit the topic. I don't have a strong

[DISCUSS] Blocking merge button in PR

Hi devs, A few months ago a proposal was brought up regarding blocking the merge button on the github PR page in case of failing tests in the precheck. What is the sentiment regarding this now? Do we feel that it should be implemented? Or at least take the minimal step of not allowing merge till