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