Re: [PSA] Github branch protection

[Aaron Lindsey]

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

Re: [PSA] Github branch protection

[Owen Nichols]

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/ 

 :

“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  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  > 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  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  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  wrote:
> 
>> One side effect is that any single request for 

Re: [PSA] Github branch protection

[Kirk Lund]

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  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  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  > 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  wrote:
> >>>
>  @Aaron : which PR are you

Re: [PSA] Github branch protection

[Owen Nichols]

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