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