Re: [DISCUSS] Criteria for PMC, committers

I appreciate the concern about bias/cronyism. If having some criteria will “level the playing field”, then let’s discuss what those criteria would be. However, in voting for a committer, a single -1 carries more weight than all the +1’s in the world. A -1 also requires explanation. This

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

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

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

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

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

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

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] Criteria for PMC, committers

I agree with Jake that the lack of guidelines is an issue that introduces our own biases. I don't think this means that we need to change our process, but rather that we need to formalize it. If a -1 vote requires a justification, then we need to have expectations defined. It's a lot harder to

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

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

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

> 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

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

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: what is the best way to update a geode pull request

I’ve almost never seen a PR where this checklist was filled out. Either the PR is created with the original boilerplate intact (or sometimes it’s deleted entirely). I feel like this wall of boilerplate discourages contributors from instead using that space to describe their change and add

Re: what is the best way to update a geode pull request

+1 to updating the checklist. About single vs multiple commits: I think either is fine as long as it is deliberate and makes it easier to understand what someone is doing. If there is a large refactor that enables a small change, it is often easier to understand when these changes are in separate

Re: what is the best way to update a geode pull request

I really don't like force-push to PRs after comments have been made. At least GitHub shows that the force-push occured now, but that still blocks us from seeing the incremental change. For needing to update a PR due to develop having progressed, I recommend using `git merge origin/develop `

Re: what is the best way to update a geode pull request

I think a single (squashed if required) commit for the start of a PR. ANY changes due to comments should be reflected as separate commits on the PR. Once approved, that PR should squash all commits into 1 commit with a message that describes what was done,etc... In the past I've heard

Re: what is the best way to update a geode pull request

+1. I would guess that it is the checklist as part of the PR that is confusing people. The other reason that history gets rewritten is when force pushing after a rebase. While fast-forwarding is necessary on occasion, this can be accomplished without rewriting history by using a merge. As part

Re: what is the best way to update a geode pull request

+1 to pushing PR changes as separate commits. Also +1 to creating PRs with multiple commits where it makes sense. -Dan On Fri, May 31, 2019 at 1:28 PM Owen Nichols wrote: > Personally, I do not force-push to my PRs once any review comments have > been accumulated, for the reasons you mention.

Re: what is the best way to update a geode pull request

+1 for initial PR needs to be a single commit +1 for making intermittent commits to the PR to show history of revision. I would only do a rebase and force-push if github tells me that there is a conflict, but my rebase would keep my original commits (no squashing). I do not like to do a merge,

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

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

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

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: what is the best way to update a geode pull request

Let’s update the checklist to match the outcome of this thread: https://github.com/apache/geode/blob/develop/.github/PULL_REQUEST_TEMPLATE.md Anthony > On May 31, 2019, at 1:31 PM, Helena Bales wrote: > > +1. I

Re: what is the best way to update a geode pull request

> Not sure if some people just force-push out of habit, or if the requirement for initial commit to be squashed creates some fear. If anyone here finds they have that habit, it would be a good one to break. > I would only do a rebase and force-push if github tells me that there is a > conflict,

Re: what is the best way to update a geode pull request

> On May 31, 2019, at 2:23 PM, Udo Kohlmeyer wrote: > I must be honest, but I am yet to find 1 developer that keeps a list of all > changes they want to be refactored separate from the bug/feature code. OR > better stated I am yet to find where this was sustainable AND productive. Challenge

Re: what is the best way to update a geode pull request

> > This sounds to me like "all my friends are jumping off bridges" and an > incorrect application or understanding of your toolset. In my own > experience, I have had much more trouble trying to track a bug through > massive squashed commits than I have identifying which branch was > responsible

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

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

what is the best way to update a geode pull request

Something I have noticed is that often when I have requested changes be made to a pull request is that the changes are force pushed ask a single commit to the pr. I would actually prefer that the changes show up as a new commit on the pr instead of everything being rebased into one commit. That

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: what is the best way to update a geode pull request

> On May 31, 2019, at 2:40 PM, Udo Kohlmeyer wrote: > > If we are concerned about the single line that can break the product, then > our testing has failed us on so many levels, that there is no hope. Sorry, I used a hyperbolic statement about looking for 1 line out of 1000. The point was

Re: [DISCUSS] Criteria for PMC, committers

@Owen Some interesting thoughts and proposals. None of which I think we could easily implement, but some that I would LOVE to have. I also apologize for restating many things that Helena has already said, but it is taking my a long time to craft this email... Jake has raised a concern that I

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: what is the best way to update a geode pull request

Personally, I do not force-push to my PRs once any review comments have been accumulated, for the reasons you mention. Not sure if some people just force-push out of habit, or if the requirement for initial commit to be squashed creates some fear. I would go a step further and suggest that

Re: what is the best way to update a geode pull request

In my opinion, I am okay will multiple commits within a PR. But please do squash them to a single commit when it is pushed to develop. This helps us a ton if it is single commit. - bisect operations are easier when it is a single commit during major failure analysis. - cherrypick is easier when

Re: what is the best way to update a geode pull request

Would that be PR of a the PR template. Might that cause a black hole to form? I’m in favor of updating this wall of text you smack your face on for each PR. Let’s pair it down to discourage people (myself strongly included) from ignoring or deleting it. -jake > On May 31, 2019, at 1:33 PM,

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: what is the best way to update a geode pull request

If we are concerned about the single line that can break the product, then our testing has failed us on so many levels, that there is no hope. But looking forward to see how long one can sustain the "factor -> commit -> make changes required to fulfill JIRA -> commit -> manual merge"...

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] Criteria for PMC, committers

Are you thinking in terms of something like this? https://cwiki.apache.org/confluence/display/GEODE/Code+of+Conduct Or something more specific to coding tasks? Thanks, Anthony > On May 31, 2019, at 2:41 AM, Owen Nichols

Re: [DISCUSS] Criteria for PMC, committers

I think it's a lot of what is under the contributing section, but I think it needs to be cleaned up. A lot of the what should be done is lost under the screen shots of things. I nice clear bullet point, with maybe links to the wiki for details, would be nice. If we collected all of this in the

Re: what is the best way to update a geode pull request

+1 to updating the PR template. I've noticed that few people actually follow it and sometimes they just remove it altogether. +1 to pushing PR changes as separate commits. This makes PR review easier. Sometimes it's helpful to me as a reviewer for the initial PR to be split into multiple commits