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
