AW: Flaw in our process for dealing with trivial changes

[Matt Caswell]

On 12/12/2019 11:15, Dr. Matthias St. Pierre wrote:
> 
>> 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.
> 
> Reusing Tim's words I'd like to argue that we should avoid rushing for a 
> premature optimization.
> 
> - Just adding new labels does not solve anything, at least as long as those labels
>   are not enforced automatically. (via GitHub bot & git hook)

I think part of the problem with "CLA: trivial" handling at the moment
is that it is not *visible*. A PR with a "CLA: trivial" header in the
commit, and with no CLA on file, looks the same as a PR where the author
does have a CLA on file. It's easy to miss that "CLA: trivial" is there
at all.

I agree that having an automatically enforced solution is the way to go.


> 
> - This is the first time in quite a while that a pull request with a questionable
>   trivial CLA has slipped in

As far as we know!

> and it is no problem to revert a commit if necessary.
>   So I wouldn't consider it a significant problem. The best countermeasure IMHO
>   is to adopt the habit of skimming over the last GitHub messages of the PR and
>   to reread the final commit messages (after the "Reviewed-by:" annotations have
>   been added) before pushing the final commit.
> 
> 
> As for a possible semi-automated solution:
> 
> The problem is more fundamental: currently both the GitHub bot and the git commit hook
> only watch out for the 'CLA: trivial' marker. But this marker is intended to be added
> by the *contributor* himself, so it expresses only his opinion, not ours. Only if all three,
> the contributor and the two reviewers agree, then the pull request can be considered
> trivial.
> 
> One possible solution to this problem could be the following procedure: 
> 
> Add three mutually exclusive [cla: *] labels:
> 
>   [cla: ok]                 (green)
>   [cla: trivial]           (green)
>   [cla: missing]        (red)
> 
> The CLA bot  *always* sets the [cla: ok] label if it finds a  CLA on file. Otherwise, it sets the
> [cla: missing] label, unless the [cla: trivial] label is already set.
> 
> The [cla: trivial] label can only be set manually by a committer, and only after the consent
> between contributor and both reviewers has been reached.

This solution could work.

> 
> The server-side commit hook ensures that
> 
> - the "CLA: trivial" annotation is present in *all* commits of the PR if and only if the [cla: trivial]
>   label is set.
> - the [cla: ok] label is set if and only if the CLA is on file
> - the pull request is accepted only if the [cla: ok] or [cla: trivial] label is set


One issue with this bit is that it requires the git hooks to connect to
github to check the labels. AFAIK we don't do that at present. Does it
add too much complexity to the hooks?

Matt