I’d love to know what is our *current* process before discussing changing it. Is there a link, or are new folks expected to read through every email from the past 5 years to establish context?
> On Oct 25, 2019, at 11:49 AM, Kirk Lund <kl...@apache.org> wrote: > > We've been operating under a different process than what you are quoting -- > I'm not saying that our current process is correct (or even wrong), however > it has been different than the quoted process in some ways. > > We previously reached consensus for our own process during the first couple > years of this community simply by doing what we are now doing (right or > wrong). I assume that changing it requires some sort of formalized > discussion on this dev-list. Please don't assume or state that we're > following the quoted process (even if that's what the Apache websites say > we should be doing). Instead, please propose following the quoted Apache > process with your interpretation, so that we can establish that as the new > process by consensus. > > On Fri, Oct 25, 2019 at 11:21 AM Owen Nichols <onich...@pivotal.io> wrote: > >> Successful Apache projects value a broad and collaborative community over >> the details of the code itself. I don’t think the goal is to hammer out >> rules like “you can’t merge if someone has requested changes” or “anyone >> has the right to overrule someone’s request for changes”. >> >> Think of “request for changes” as an opportunity to collaborate. From >> https://www.freecodecamp.org/news/code-review-the-ultimate-guide-aa45c358bbf5/ >> < >> https://www.freecodecamp.org/news/code-review-the-ultimate-guide-aa45c358bbf5/> >> : >> >> “Code reviews are discussions, not dictation — You can think of most code >> review feedback as a suggestion more than an order. It’s fine to disagree >> with a reviewer’s feedback but you need to explain why and give them an >> opportunity to respond." >> >> For the “rules" that we have discussed and agreed on, it would be really >> helpful if someone could collect them on a single wiki page somewhere, >> otherwise newcomers to the project face a daunting task of combing through >> years of email archives to reconstruct all of what has been discussed and >> decided by the community. >> >> -Owen >> >>> On Oct 25, 2019, at 9:52 AM, Aaron Lindsey <alind...@pivotal.io> wrote: >>> >>> @Owen I'm fine with following the "requesting changes is the same as -1" >>> rule, but I don't think there is consensus from the whole community on >> this >>> yet. I was previously told that contributors should make every effort to >>> address the requested changes, but unless a committer actually comments >>> "-1" on the PR, the author retains the ability to reject the requested >>> changes. This probably deserves a separate discussion at some point. >>> >>> My original concern has been addressed since Helena pointed out that a >>> review can be dismissed. I agree this power should probably only be used >> if >>> the reviewer cannot be contacted. >>> >>> - Aaron >>> >>> >>> On Thu, Oct 24, 2019 at 5:04 PM Owen Nichols <onich...@pivotal.io >> <mailto:onich...@pivotal.io>> wrote: >>> >>>>> >>>>> @Owen It's unclear to me whether "requesting changes" is the same thing >>>> as >>>>> a -1 vote. I had previously discussed this with some other committers >> who >>>>> were under the impression that they were not the same thing. >>>>> >>>> >>>> If that’s not a -1, what is? >>>> >>>> Many apache projects started out long ago when patches were submitted by >>>> email and voted over email. We have adopted modern GitHub tools to >>>> streamline this process, but the concept is still the same: reviewers >> can >>>> give a +1 (green check) review, or a -1 (with explanation of what it >> would >>>> take to get their approval). >>>> >>>> One time when I was a relatively new committer, I merged a PR while >>>> someone still had changes requested on it, and I was admonished: “it >> sets a >>>> bad precedent for merges to occur on PRs that have unresolved reviews. < >>>> https://github.com/apache/geode/pull/3597#issuecomment-493624748 < >> https://github.com/apache/geode/pull/3597#issuecomment-493624748>>”. >>>> GitHub will also permanently record that the PR was merged in spite of >> an >>>> outstanding request for changes, leading to life-long shame. >>>> >>>> If someone has requested changes on your PR, you should make every >> effort >>>> to understand and address their concern. If you have pushed additional >>>> changes since their review, remind them by clicking the re-request >> review >>>> button next to their name. >>>> >>>> On the flip side, if you request changes on a PR, please clearly explain >>>> what it would take to gain your approval, and please make an effort to >>>> re-review in a timely fashion after changes are made. More guideline >> for >>>> good PR review practices can be found here < >>>> >> https://medium.freecodecamp.org/unlearning-toxic-behaviors-in-a-code-review-culture-b7c295452a3c >> < >> https://medium.freecodecamp.org/unlearning-toxic-behaviors-in-a-code-review-culture-b7c295452a3c >>> >>>>> . >>>> >>>> The option to dismiss a review should be a last resort, for example if >> the >>>> reviewer went on vacation and cannot be contacted. >>>> >>>> -Owen >>>> >>>>> - Aaron >>>>> >>>>> >>>>> On Thu, Oct 24, 2019 at 3:02 PM Nabarun Nag <n...@apache.org> wrote: >>>>> >>>>>> @Aaron : which PR are you referring to? I can only see "GEODE-7326: >> Add >>>>>> cache gets timers" which can be merged? I can get some more idea when >> I >>>> can >>>>>> see whats going on. >>>>>> >>>>>> Regards >>>>>> Naba >>>>>> >>>>>> @Kirk : let me run some experiments. >>>>>> >>>>>> Regards >>>>>> Naba >>>>>> >>>>>> >>>>>> On Thu, Oct 24, 2019 at 2:57 PM Helena Bales <hba...@pivotal.io> >> wrote: >>>>>> >>>>>>> To Kirk's point, there is actually a way to dismiss requests for >>>> review. >>>>>>> Info here: >>>>>>> >>>>>>> >>>>>> >>>> >> https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/dismissing-a-pull-request-review >>>>>>> There's instructions in there for how to dismiss a request for >> changes. >>>>>> Not >>>>>>> everyone can do that, so if you aren't a contributor yet you'll >>>> probably >>>>>>> have to hit up a current contributor to get any requests for changes >>>>>>> dismissed. >>>>>>> >>>>>>> On Thu, Oct 24, 2019 at 2:52 PM Kirk Lund <kl...@apache.org> wrote: >>>>>>> >>>>>>>> One side effect is that any single request for changes will now >>>>>>> completely >>>>>>>> block merging the PR. I'm not certain this was intentional? One >> rogue >>>>>>>> developer could block the merging of any or every PR. I'm not sure >> one >>>>>>>> person should have that much power... >>>>>>>> >>>>>>>> On Thu, Oct 24, 2019 at 2:25 PM Nabarun Nag <n...@apache.org> >> wrote: >>>>>>>> >>>>>>>>> Hi, Geode dev Community, >>>>>>>>> >>>>>>>>> This is an announcement that the GitHub branch protection rules are >>>>>>> *now >>>>>>>>> active* on develop branch for Apache Geode. >>>>>>>>> >>>>>>>>> The following rules are currently active : >>>>>>>>> - Require pull request reviews before merging - at least 1 >>>>>>>>> - Require status checks to pass before merging >>>>>>>>> [Only for >>>>>>>>> - concourse-ci/Build >>>>>>>>> - concourse-ci/UnitTestOpenJDK11 >>>>>>>>> - concourse-ci/UnitTestOpenJDK8 >>>>>>>>> - concourse-ci/StressNewTestOpenJDK11] >>>>>>>>> >>>>>>>>> After we stabilize the remaining test suites, we can add them to >>>>>> these >>>>>>>> rule >>>>>>>>> sets. >>>>>>>>> >>>>>>>>> Also reminding the community to use squash merge while closing pull >>>>>>>>> requests. >>>>>>>>> >>>>>>>>> Regards >>>>>>>>> Naba >> >>
