> > @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>”. 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>. 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 >>>>> >>>> >>> >>
