geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Owen Nichols <onich...@pivotal.io>
Subject Re: [PSA] Github branch protection
Date Fri, 25 Oct 2019 18:53:25 GMT
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 <klund@apache.org> 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 <onichols@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 <alindsey@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 <onichols@pivotal.io
>> <mailto:onichols@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 <nnag@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 <hbales@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 <klund@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 <nnag@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
>> 
>> 


Mime
View raw message