Flaw in our process for dealing with trivial changes

[Matt Caswell]

On 12/12/2019 09:43, Paul Yang wrote:
> Would it be better if 'CLA: trivial’ is in the commit message but no CLA
> on file, then a new label like ’warn: no CLA but trivial’ is added? This
> can inform the committer who will merge the PR for the CLA condition of
> the commits.
> 

Yes, I think that would be a really good idea. At least then its very
visible what is happening.

On 12/12/2019 09:29, Dmitry Belyavsky wrote:
> - the contributor agreed to sign the CLA and
> - there was a mark that CLA is signed and
> - all the necessary approves were present
> I decided that there is no problem to merge.

The only thing I could see in that PR was a message from the author
saying that they would submit a CLA. There doesn't seem to be a message
saying that the CLA had actually been processed.

It's not sufficient for the author to *say* they have submitted a CLA.
It must actually have been submitted, and accepted and be on file.
Sometimes there are problems with submitted CLAs (e.g. missing fields,
or a user sends an ICLA when they really also need a CCLA, etc).
Normally the git hooks would not allow you to push a commit where the
author does not have a CLA. However those checks are suppressed where
"CLA: trivial" appears in the commit description. So we need to be extra
careful in that case, i.e. perhaps we should insist that commits
containing "CLA: trivial" have the line removed if we don't think the
commit really is trivial. This ensures that the git hooks do their job
and we can be absolutely sure that the author has a registered CLA.

Matt

> 
> Regards,
> 
> Paul Yang
> 
>> On Dec 12, 2019, at 5:29 PM, Dmitry Belyavsky <beldmit at gmail.com
>> <mailto:beldmit at gmail.com>> wrote:
>>
>> Dear Matt,
>>
>> As 
>> - the contributor agreed to sign the CLA and 
>> - there was a mark that CLA is signed and
>> - all the necessary approves were present
>> I decided that there is no problem to merge.
>>
>> BTW, I am not sure the PR was trivial enough.
>>
>> Anyway, the responsibility was mine, not the git one :)
>>
>>
>> On Thu, Dec 12, 2019 at 12:20 PM Matt Caswell <matt at openssl.org
>> <mailto:matt at openssl.org>> wrote:
>>
>>     I notice that PR 10594 (Add support for otherName:NAIRealm in output)
>>     got merged yesterday:
>>     https://github.com/openssl/openssl/pull/10594
>>
>>     The commit description contained "CLA: trivial" and so the "hold: cla
>>     required" label was not automatically applied to the PR. But the
>>     discussion in the PR suggested a CLA should be submitted. But it got
>>     merged anyway! Fortunately the CLA had already been processed -
>>     just not
>>     noted in the PR. So, in this case, it makes no difference.
>>
>>     I think this points to a possible flaw in our workflow for dealing
>>     with
>>     trivial changes. Because the "CLA: trivial" header suppresses the
>>     "hold:
>>     cla required" label and the git hooks don't complain when commits get
>>     pushed with the "CLA: trivial" header and no CLA on file, it seems
>>     possible to me that we could push commit all the way through the
>>     process
>>     without the reviewers even realising that the author is claiming
>>     triviality on the commit.
>>
>>     Not sure what the solution to that is.
>>
>>     Matt
>>
>>
>>
>> -- 
>> SY, Dmitry Belyavsky
>