Re: proposed change to committers policy

2019-05-24 Thread Salz, Rich
Nicely worded.  It doesn’t go as far as I’d like, but it’s a step.


Re: AW: [openssl] OpenSSL_1_1_1-stable update

2019-05-24 Thread Nicola Tuveri
I have always implicitly assumed Matt view, but I am happy to conform to
what the consensus is.

I believe this discussion is very useful and could contribute a new entry
in the commiter guidelines.

Nicola

On Fri, May 24, 2019, 07:21 Matt Caswell  wrote:

>
>
> On 24/05/2019 15:10, Richard Levitte wrote:
> > Not sure I see it as picking nits, it's rather about some fundamental
> > difference in what we thinking we're approving, and how we actually
> > act around that.
> >
> > My idea has always been that I approve a code change, i.e. essentially
> > a patch or a set of patches, without regard for exact branches it ends
> > up in.  With the in mind, the exact branches it gets applied to is a
> > *separate* question.
>
> That's not the way I've ever thought of it. In my mind an approval is for a
> change applied to a specific branch. Where a PR lists more than one branch
> in it
> and you approve the PR then effectively you are approving it multiple
> times all
> in one go - once for each branch.
>
>
> > If we go with the idea that an approval also involves approving what
> > branches it goes to, then what happens if someone realises after some
> > time that a set of commits (a PR) that was applied to master only
> > should really also be applied to 1.1.1?  Should the approval process
> > start over from scratch, i.e. all approvals that went to master should
> > be scratched and replaced with a new set of approvals (in principle)?
>
> No. If the PR was approved for master and applied to master then no
> problem - it
> stays in master. If it is later realised that it needs to be backported to
> other
> branches then, yes, new approvals need to be sought for that change to
> *those
> branches*.
>
> As far as I was aware we've always done this.
>
> This is essential in my mind. A change for one branch does not always make
> sense
> in another branch. So you can't just say "I approve this change" and *then*
> worry about what branches it applies to. A change only makes sense in the
> context of the branch it applies to.
>
> Matt
>


Re: No two reviewers from same company

2019-05-24 Thread Salz, Rich
>   In that example the potential conflict of interest comes from the 
> individual's
employment with the third party organisation, not because they are fellows.
  
Do you disagree with my contention that the OMC represents the project, and not 
the fellows?

Regardless of where the conflict of interest comes from, the end result is the 
same, and I encourage the OMC to make the same policy for *its* employees that 
it does for other companies's employees.  Or perhaps this is a matter for the 
foundation to make for its employees.




Re: AW: [openssl] OpenSSL_1_1_1-stable update

2019-05-24 Thread Matthias St. Pierre

I forgot one word:

On 24.05.19 17:45, Matthias St. Pierre wrote:

(Otherwise, the missing approvers need to be from the Reviewed-by list
and additional approvals would be needed). 


need to be _removed_ from the Reviewed-by list



Re: AW: [openssl] OpenSSL_1_1_1-stable update

2019-05-24 Thread Matthias St. Pierre




On 24.05.19 16:54, Richard Levitte wrote:

On Fri, 24 May 2019 16:39:51 +0200,
Matt Caswell wrote:



On 24/05/2019 15:30, Richard Levitte wrote:

Not in practice.  We *do* ask on the PR in question if it should be
cherry-picked to 1.1.1 and seek approval for that action, but then it
hasn't at all been clear what should happen regarding Received-By
tags.

I have personally never touched them when cherry-picking, even in this
scenario.  I do not know what others do in that case...>


In principle I agree with Matt, his arguments are reasonable.
Maybe a compromise would be to allow the practice of asking
for an informal reconfirmation when cherry-pick tags were forgotten,
but only if one receives reconfirmation from _all_ original reviewers?
(Otherwise, the missing approvers need to be from the Reviewed-by list
and additional approvals would be needed).

Matthias



Re: AW: [openssl] OpenSSL_1_1_1-stable update

2019-05-24 Thread Richard Levitte
On Fri, 24 May 2019 16:39:51 +0200,
Matt Caswell wrote:
> 
> 
> 
> On 24/05/2019 15:30, Richard Levitte wrote:
> > 
> > Not in practice.  We *do* ask on the PR in question if it should be
> > cherry-picked to 1.1.1 and seek approval for that action, but then it
> > hasn't at all been clear what should happen regarding Received-By
> > tags.
> > 
> > I have personally never touched them when cherry-picking, even in this
> > scenario.  I do not know what others do in that case...>
> 
> In the vast majority of the cases the reviewers are the same.

Yes, because cherry-picking as an after-though rarely happens.  It's
mostly been along the lines of "hey, why was this bug-fix only applied
to master???"

> I wouldn't want other people putting my name in a reviewed-by tag
> where I have not approved it and I have not considered the
> implications of that change in that branch. What if it resulted in a
> critical CVE?

I haven't given possible CVEs much though, quite frankly.

But tell you what, I can certainly change my ways if this is what we
all think is the way to go.  Not a problem.  And I'm glad that we
talked about it, rather than staying with "I thought everyone else did
the same".

Cheers,
Richard

-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/


Re: AW: [openssl] OpenSSL_1_1_1-stable update

2019-05-24 Thread Matt Caswell



On 24/05/2019 15:30, Richard Levitte wrote:
> On Fri, 24 May 2019 16:20:59 +0200,
> Matt Caswell wrote:
>> On 24/05/2019 15:10, Richard Levitte wrote:
>>> If we go with the idea that an approval also involves approving what
>>> branches it goes to, then what happens if someone realises after some
>>> time that a set of commits (a PR) that was applied to master only
>>> should really also be applied to 1.1.1?  Should the approval process
>>> start over from scratch, i.e. all approvals that went to master should
>>> be scratched and replaced with a new set of approvals (in principle)?
>>
>> No. If the PR was approved for master and applied to master then no problem 
>> - it
>> stays in master. If it is later realised that it needs to be backported to 
>> other
>> branches then, yes, new approvals need to be sought for that change to *those
>> branches*.
>>
>> As far as I was aware we've always done this.
> 
> Not in practice.  We *do* ask on the PR in question if it should be
> cherry-picked to 1.1.1 and seek approval for that action, but then it
> hasn't at all been clear what should happen regarding Received-By
> tags.
> 
> I have personally never touched them when cherry-picking, even in this
> scenario.  I do not know what others do in that case...>

In the vast majority of the cases the reviewers are the same. In the rare
circumstances where they are different I have always changed them. I thought
everyone did. IMO that is the correct action. By putting your name as a reviewer
against a PR you are effectively saying "I have reviewed this and agree that it
is appropriate for this to be merged". I wouldn't want other people putting my
name in a reviewed-by tag where I have not approved it and I have not considered
the implications of that change in that branch. What if it resulted in a
critical CVE?

Matt


Re: AW: [openssl] OpenSSL_1_1_1-stable update

2019-05-24 Thread Richard Levitte
On Fri, 24 May 2019 16:20:59 +0200,
Matt Caswell wrote:
> On 24/05/2019 15:10, Richard Levitte wrote:
> > If we go with the idea that an approval also involves approving what
> > branches it goes to, then what happens if someone realises after some
> > time that a set of commits (a PR) that was applied to master only
> > should really also be applied to 1.1.1?  Should the approval process
> > start over from scratch, i.e. all approvals that went to master should
> > be scratched and replaced with a new set of approvals (in principle)?
> 
> No. If the PR was approved for master and applied to master then no problem - 
> it
> stays in master. If it is later realised that it needs to be backported to 
> other
> branches then, yes, new approvals need to be sought for that change to *those
> branches*.
> 
> As far as I was aware we've always done this.

Not in practice.  We *do* ask on the PR in question if it should be
cherry-picked to 1.1.1 and seek approval for that action, but then it
hasn't at all been clear what should happen regarding Received-By
tags.

I have personally never touched them when cherry-picking, even in this
scenario.  I do not know what others do in that case...

-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/


AW: [openssl] OpenSSL_1_1_1-stable update

2019-05-24 Thread Dr. Matthias St. Pierre
Matt and Richard, I think you are mixing up cherry-picking and nit-picking 
here. (Sorry for the pun ;-)

Matthias

> To be picky, may I assume that you meant a reviewed-by tag for you
> > should be *added*?  The commit itself (its contents) having been
> > reviewed by those already there, I cannot see a reason why they should
> > be removed.
> 
> You approved for master but did not approve for 1.1.1. This commit was merged 
> to
> 1.1.1 and so strictly speaking was not approved by you for that branch.
> Therefore you should have been removed, and I should have been added.
> 
> Matt



AW: proposed change to committers policy

2019-05-24 Thread Dr. Matthias St. Pierre


> > IMHO an important clarification needs to be made: The rule should not be 
> > about
> > _prohibiting_ double approvals. It should only be about _counting_ them.
> > I would not deprive people of the right to state their opinions.
> 
> Isn't that an implementation detail?

You're right, it should be. But current discussions and current practice show 
that Pauli
avoids to approve on GitHub when Shane is involved and vice versa. IMHO it 
should
be valid to approve (and also recorded in the commit message), even if the vote 
is not
counted. Because it documents that both have reviewed the code.

Matthias



Re: proposed change to committers policy

2019-05-24 Thread Richard Levitte
On Fri, 24 May 2019 11:21:18 +0200,
Matthias St. Pierre wrote:
> 
> >   In considering approvals, the combined approvals must come
> >   from individuals who work for separate organisations.
> >   This condition does not apply where the organisation is the
> >   OSF or OSS.
> 
> 
> IMHO an important clarification needs to be made: The rule should not be about
> _prohibiting_ double approvals. It should only be about _counting_ them.
> I would not deprive people of the right to state their opinions.

Isn't that an implementation detail?

-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/


Re: proposed change to committers policy

2019-05-24 Thread Tim Hudson
On Fri, May 24, 2019 at 7:34 PM Matt Caswell  wrote:

> On 24/05/2019 10:28, SHANE LONTIS wrote:
> > It doesn’t stop us both reviewing a PR. That doesn’t mean we both need
> to approve.
>
> Right...but in Matthias's version if you raise a PR, and then Pauli
> approves it,
> then you only then need to get a second committer approval. Otherwise you
> would
> need to get an OMC approval.
>

It works that way in the original wording too - which is more simply stated
IMHO.
You choose which approvals you combine. If there are three - select which
ever two make the set you want to "combine" as such.

I also didn't see a need for a separate OMC approval if a committer
submitted something and a same-organisation OMC member approved it - it
just needs one more -  so the combination of approvals can be made from
not-the-same-organisation.

Tim.


Re: proposed change to committers policy

2019-05-24 Thread Matt Caswell
On 24/05/2019 10:28, SHANE LONTIS wrote:
> It doesn’t stop us both reviewing a PR. That doesn’t mean we both need to 
> approve.

Right...but in Matthias's version if you raise a PR, and then Pauli approves it,
then you only then need to get a second committer approval. Otherwise you would
need to get an OMC approval.

That sounds ok to me.

Matt

> 
> 
>> On 24 May 2019, at 7:21 pm, Matthias St. Pierre > > wrote:
>>
>>
>>
>> >   In considering approvals, the combined approvals must come
>> >   from individuals who work for separate organisations.
>> >   This condition does not apply where the organisation is the
>> >   OSF or OSS.
>>
>>
>> IMHO an important clarification needs to be made: The rule should not be 
>> about
>> _prohibiting_ double approvals. It should only be about _counting_ them.
>> I would not deprive people of the right to state their opinions.
>>
>> Also, if Shane has already approved, then Pauli should still have the right
>> to approve, since his approval counts as OMC approval and Shane's not.
>>
>> How about using a formulation like the following?
>>
>>     In considering approvals, approvals from individuals being
>>         employed by the same third party company or organization should
>>         be counted as a single approval. If one of the individuals is
>>         an OMC member, the combined approval counts as an OMC approval.
>>     This condition does not apply where the organisation is the
>>     OSF or OSS.
>>
>>
>> An editorial comment: I think that the amendmend should be made inside the
>> unordered list, not following it.
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openssl_web_blob_master_policies_committers.html-23L72-2DL78=DwIDaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=b1aL1L-m41VGkedIk-9Q7taAEKIshTBwq95Iah07uCk=DYHGYucybgbeMHQn9pS8SBhh_dvJkYrDgKu9gU2l9H4=jT3VTidhpkkMAwUuiu_JgqW9mBpD4PP-3Qc8D3KZDdU=
>>
>> Apart from that: Tim, could you (or someone else of the OMC) please raise a 
>> PR
>> for your proposal? That would make it easier to discuss the details.
>>
>> Matthias
>>
>>
> 


Re: proposed change to committers policy

2019-05-24 Thread SHANE LONTIS
It doesn’t stop us both reviewing a PR. That doesn’t mean we both need to 
approve.


> On 24 May 2019, at 7:21 pm, Matthias St. Pierre 
>  wrote:
> 
> 
> 
> >   In considering approvals, the combined approvals must come
> >   from individuals who work for separate organisations.
> >   This condition does not apply where the organisation is the
> >   OSF or OSS.
> 
> 
> IMHO an important clarification needs to be made: The rule should not be about
> _prohibiting_ double approvals. It should only be about _counting_ them.
> I would not deprive people of the right to state their opinions.
> 
> Also, if Shane has already approved, then Pauli should still have the right
> to approve, since his approval counts as OMC approval and Shane's not.
> 
> How about using a formulation like the following?
> 
> In considering approvals, approvals from individuals being
> employed by the same third party company or organization should
> be counted as a single approval. If one of the individuals is
> an OMC member, the combined approval counts as an OMC approval.
> This condition does not apply where the organisation is the
> OSF or OSS.
> 
> 
> An editorial comment: I think that the amendmend should be made inside the
> unordered list, not following it.
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openssl_web_blob_master_policies_committers.html-23L72-2DL78=DwIDaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=b1aL1L-m41VGkedIk-9Q7taAEKIshTBwq95Iah07uCk=DYHGYucybgbeMHQn9pS8SBhh_dvJkYrDgKu9gU2l9H4=jT3VTidhpkkMAwUuiu_JgqW9mBpD4PP-3Qc8D3KZDdU=
> 
> Apart from that: Tim, could you (or someone else of the OMC) please raise a PR
> for your proposal? That would make it easier to discuss the details.
> 
> Matthias
> 
>