Re: [DISCUSS] require reviews before merging a PR

Thanks Patrick. That's what we're doing currently so we'll continue to do so :). On Thu, Jun 6, 2019 at 12:40 PM Patrick Rhomberg wrote: > > > > I'm not sure it's possible to do this with the "reviewers" field - if > > someone can figure out how to do this with the github IU, we can at least >

Re: [DISCUSS] require reviews before merging a PR

> > I'm not sure it's possible to do this with the "reviewers" field - if > someone can figure out how to do this with the github IU, we can at least > try filing an ticket with apache infrastructure. > According to their docs [1], "collaborator with write access" is the GitHub required criteria

Re: [DISCUSS] require reviews before merging a PR

> > Would it be possible to allow people who do not have committer status to > request reviewers on a pull request. I'm not sure it's possible to do this with the "reviewers" field - if someone can figure out how to do this with the github IU, we can at least try filing an ticket with apache

Re: [DISCUSS] require reviews before merging a PR

Would it be possible to allow people who do not have committer status to request reviewers on a pull request. In some cases we may know who should take a look at it and in that case making it official by adding these people to the pull request would be good IMO. On Thu, Jun 6, 2019 at 10:26 AM

Re: [DISCUSS] require reviews before merging a PR

As reviewers we should also feel empowered to request additional reviewers on a PR (perhaps beyond whomever the original submitter may already have requested). I think that, sometimes the complexity of a change prevents someone from commenting on just a portion of the change if they do not feel

Re: [DISCUSS] require reviews before merging a PR

> > If we trust committers, why review at all? Just commit... and we might > catch a problem, we might not. Honestly that to me would be the ideal state. However, our test coverage and code quality is nowhere near to allow for that. What I was referring to is different though. I didn't say

Re: [DISCUSS] require reviews before merging a PR

Alexander, thank you for your response. And yes, change is uncomfortable and in some cases more reviewers would not have caught issues. BUT, more people would have seen the code, maybe become more familiar with it, etc... I don't say don't trust committers, as I am one. But I also know that I

Re: [DISCUSS] require reviews before merging a PR

Udo, I agree with many of the pains you are addressing, but am pessimistic that having more reviewers will solve them. You are absolutely correct in calling out that the code is ugly complex and missing coverage. The best way to address this is to clean up the code and improve coverage. You say

Re: [DISCUSS] require reviews before merging a PR

@Kirk, I totally understand the pain that you speak of. I too agree that every line of changed code should have a test confirming that behavior was not changed. I don't believe that we need to go as far as revoking committer rights and reviewing each committer again, BUT it would be AMAZING

Re: [DISCUSS] require reviews before merging a PR

ight from a committer. Finally, I also agree on the point of increasing test coverage and follow clean code principles. De: Kirk Lund Enviado: miércoles, 5 de junio de 2019 1:49:06 Para: geode Asunto: Re: [DISCUSS] require reviews before merging a PR I'm -

Re: [DISCUSS] require reviews before merging a PR

I'm -1 for requiring N reviews before merging a commit. Overall, I support Lazy Consensus. If I post a PR that fixes the flakiness in a test, the precheckin jobs prove it, and it sits there for 2 weeks without reviews, then I favor merging it in at that point without any reviews. I'm not going to

Re: [DISCUSS] require reviews before merging a PR

I think my recent practice with Eric Shu's PR #3623 could be a good example. In this specific bug with a lot of context, it's hard for Eric Shu to find 3 persons to review. Bruce and I are the 2 right persons who know the history and context. Eric came to me many times and we had a lot of

Re: [DISCUSS] require reviews before merging a PR

I’ll be posting a PR for it later next week so y’all can review. > On May 31, 2019, at 2:02 PM, Helena Bales wrote: > > I'm happy to provide feedback on a CONTRIBUTING.md, but I don't want to > take the lead on this particular doc right now. > >> On Fri, May 31, 2019 at 1:48 PM Jacob Barrett

Re: [DISCUSS] require reviews before merging a PR

I'm happy to provide feedback on a CONTRIBUTING.md, but I don't want to take the lead on this particular doc right now. On Fri, May 31, 2019 at 1:48 PM Jacob Barrett wrote: > It is probably worthwhile to codify our “policy” so that it’s not confused > later. Simply adding something about lazy

Re: [DISCUSS] require reviews before merging a PR

It is probably worthwhile to codify our “policy” so that it’s not confused later. Simply adding something about lazy consensus model to the CONTRIBUTING.md (which I realize we are missing, already working on that) might be useful. I could take a stab at the wording based on my earlier reply

Re: [DISCUSS] require reviews before merging a PR

To Naba's point, there's an even easier way to keep track of all the PRs: https://github.com/pulls/review-requested It shows your PRs, your assigned PRs, your requested reviews, and your mentions (keep an eye on this tab, because people who aren't committers yet can't request reviews and may

Re: [DISCUSS] require reviews before merging a PR

I’m glad you raised this question because without it we wouldn’t have asked ourselves “What makes a good code review, when is it needed, and who should participate?”. Thank you! Anthony > On May 31, 2019, at 12:44 PM, Owen Nichols wrote: > > I have learned that other than the required

Re: [DISCUSS] require reviews before merging a PR

> On May 30, 2019, at 4:47 PM, Owen Nichols wrote: > > Some folks have found it really helpful to have the PR author schedule a walk-through of the changes to give reviewers more context and explain the thinking behind the changes. I always thought this is what a good commit message should be:

Re: [DISCUSS] require reviews before merging a PR

I have learned that other than the required quarterly report to the board, just about everything else about being an Apache project is just guidelines, not hard requirements. I was confused because we do adhere rigorously to every other voting guideline on

Re: [DISCUSS] require reviews before merging a PR

Hi, I agree with Dan, I would like to follow what he has suggested. Also, I will not mind anyone following the 3 reviewers for everything if they chose to do so. Just please don't make it blanket rule. Regards Naba PS: I found this filter on github to look up PRs that I have reviewed till date

Re: [DISCUSS] require reviews before merging a PR

Thank you Dan, Some guidance around how we can go about reviewing the code. I want to remind ALL committers..https://www.apache.org/dev/new-committers-guide.html#committer-responsibilities It states "/Each committer has a responsibility to monitor the changes made for potential issues, both

Re: [DISCUSS] require reviews before merging a PR

As others pointed out - I think it's really important to remember that people and community are much more important than process. That is a key part of "The Apache Way" - it's not a set very specific rules, but more of a philosophy of building an open community. I think this page has a good take

Re: [DISCUSS] require reviews before merging a PR

Thanks for the filter, Jinmei! +1 to Naba and Bruce. I will add that I think we should have a formal policy of getting *a* review (and more for large changes), and that PRs that are merged without one should include (in the PR) a comment providing a justification for this merge, so that

Re: [DISCUSS] require reviews before merging a PR

I was told that screenshot that I sent earlier got filtered out by the dev list. Basically, the filter puts "notificati...@github.com" in the "From" section, and put "review_reques...@noreply.github.com" in the "Doesn't have" section of the form. On Fri, May 31, 2019 at 10:36 AM Anthony Baker

Re: [DISCUSS] require reviews before merging a PR

Hi, I personally feel the same as how Bruce feels. - This will make life harder / inconvenient. - One approval from a person who is experienced in that part of code is more than enough for me. - The workload on the Geode developers is already too high, it is unfair to burden then with more

Re: [DISCUSS] require reviews before merging a PR

> On May 31, 2019, at 10:01 AM, Owen Nichols wrote: > > We chose to make Geode an Apache open source project for a reason. If we no > longer wish to embrace The Apache Way > , perhaps we should > reconsider. I strongly disagree with the

Re: [DISCUSS] require reviews before merging a PR

One reason for lacking reviews in the PR might be that people are filtering out notifications from github. I've had this problem for a long time before I finally figured out a way to filter out noses "other than" the review requested emails. Here is a snapshot of my filter setup. Hopefully it will

Re: [DISCUSS] require reviews before merging a PR

We chose to make Geode an Apache open source project for a reason. If we no longer wish to embrace The Apache Way , perhaps we should reconsider. If we believe that reviewing each other’s code changes is an onerous burden of no value, we should

Re: [DISCUSS] require reviews before merging a PR

When asking a question like “What is ASF policy and practice on XXX?” I have found that observing other ASF projects can be helpful. In particular, the ASF Incubator (gene...@incubator.apache.org ) covers these topics fairly frequently. Anthony > On May

Re: [DISCUSS] require reviews before merging a PR

> On May 31, 2019, at 8:52 AM, Owen Nichols wrote: > > Apache requires 3 reviews for code changes. Docs and typos likely would not > fall under that heading. > Code change == commit of any kind to the source repo(s). I agree that a strict RTC approach is as you described. ASF doesn’t

Re: [DISCUSS] require reviews before merging a PR

Jake, Owen is quoting the "VOTES ON CODE MODIFICATION" section from https://www.apache.org/foundation/voting.html . "code modification" == "every PR" is a interpretation that I think would bring the project to a grinding halt. On Fri, May 31, 2019 at 9:01 AM Jacob Barrett wrote: > > > > On May

Re: [DISCUSS] require reviews before merging a PR

+1 to Jake's interpretation of the voting system not having been adjusted yet for the new world that is GitHub. When I read the Apache voting guidelines they make sense to me for bug decisions but not for a regular PR. The inertia coming with three votes on every PR is way more costly than the

Re: [DISCUSS] require reviews before merging a PR

> On May 31, 2019, at 8:52 AM, Owen Nichols wrote: > > Apache requires 3 reviews for code changes. Docs and typos likely would not > fall under that heading. Where is this listed as a requirement? The link you sent before offered guidance on common policies within the organization.

Re: [DISCUSS] require reviews before merging a PR

Apache requires 3 reviews for code changes. Docs and typos likely would not fall under that heading. On Fri, May 31, 2019 at 8:47 AM Dave Barnes wrote: > As a writer, I'm a big user of Lazy Consensus: If no one objects, I'm > merging my change. Requiring multiple reviews discourages minor >

Re: [DISCUSS] require reviews before merging a PR

As a writer, I'm a big user of Lazy Consensus: If no one objects, I'm merging my change. Requiring multiple reviews discourages minor improvements. In the doc realm, I'm inclined to check in typo fixes and grammar corrections without even bothering with the PR process, but I do it for the

Re: [DISCUSS] require reviews before merging a PR

> On May 30, 2019, at 4:47 PM, Owen Nichols wrote: > > Some folks have found it really helpful to have the PR author schedule a > walk-through of the changes to give reviewers more context and explain the > thinking behind the changes. This can’t be policy unless the walkthrough is

Re: [DISCUSS] require reviews before merging a PR

> On May 30, 2019, at 5:00 PM, Anthony Baker wrote: > > Checkout [1] for some helpful context from the early days. > > Anthony > > [1] > https://lists.apache.org/thread.html/108602a14b422abe9c94d46b2c5d02c11a9cbb8b224db08b706c6263@1430991799@%3Cdev.geode.apache.org%3E > >

Re: [DISCUSS] require reviews before merging a PR

Checkout [1] for some helpful context from the early days. Anthony [1] https://lists.apache.org/thread.html/108602a14b422abe9c94d46b2c5d02c11a9cbb8b224db08b706c6263@1430991799@%3Cdev.geode.apache.org%3E

Re: [DISCUSS] require reviews before merging a PR

Some folks have found it really helpful to have the PR author schedule a walk-through of the changes to give reviewers more context and explain the thinking behind the changes. Yes, it’s more work to get reviews. Apache says (really, mandates) that this must be part of the process. I started

Re: [DISCUSS] require reviews before merging a PR

Maybe your experience is different but I find it hard enough to get even one person to review my pull requests.  I've resorted to merging minor changes without a review a few times due to lack of response. On 5/30/19 3:51 PM, Owen Nichols wrote: It seems common for Geode PRs to get merged

[DISCUSS] require reviews before merging a PR

It seems common for Geode PRs to get merged with only a single green checkmark in GitHub. According to https://www.apache.org/foundation/voting.html we should not be merging PRs with fewer than 3 green checkmarks. Consensus is a fundamental value in doing things The Apache Way. A single +1