Re: Flaw in our process for dealing with trivial changes

[Richard Levitte]

https://github.com/openssl/tools/pull/49

Quite an exercise, I think my python-fu has increased significantly.

I have *no* idea how to debug CGI stuff in a sensible way, suggestions
welcome!

Cheers,
Richard

On Fri, 13 Dec 2019 12:08:42 +0100,
Richard Levitte wrote:
> 
> clacheck modification coming up!
> 
> Cheers,
> Richard
> 
> On Fri, 13 Dec 2019 04:48:38 +0100,
> Dr Paul Dale wrote:
> > 
> > 
> > A better example of this problem: #10607.  Both Paul and I approved it 
> > yesterday and I merged it
> > today without noticing until too late that it was tagged “CLA: trivial” :(
> > I’ve not reverted it at this point but will if necessary.
> > 
> > Let’s get the label in.
> > 
> > Pauli
> > -- 
> > Dr Paul Dale | Distinguished Architect | Cryptographic Foundations 
> > Phone +61 7 3031 7217
> > Oracle Australia
> > 
> > On 13 Dec 2019, at 11:02 am, Richard Levitte  
> > wrote:
> >
> > On Thu, 12 Dec 2019 22:31:10 +0100,
> > Dr Paul Dale wrote:
> > 
> > A red blocker along the lines of: “Triviality Unconfirmed”. One of
> > the reviewers needs to remove this before the PR can be merged.
> >
> > It’s in our face, it prevent accidental merges and its low overhead.
> > 
> > I still think simply adding the label should be sufficient.  I dunno
> > about you, but I look at labels all the time, for all sorts of
> > reasons, and one saying [cla: trivial] would certainly attract my
> > attention.
> >
> > Let's make it bright red-orange, that'll catch anyone's eye (even mine)
> >
> > Also, removing that label will rapidly be annoying as soon as someone
> > closes and re-opens a PR...  or whatever other action that triggers
> > the "pull_request" event (and there's a lot that does that...  our
> > script is being kept busy!).
> >
> > Cheers,
> > Richard
> >
> > --
> > Richard Levitte levi...@openssl.org
> > OpenSSL Project http://www.openssl.org/~levitte/
> > 
> > 
> -- 
> Richard Levitte levi...@openssl.org
> OpenSSL Project http://www.openssl.org/~levitte/
> 
-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/

Re: Flaw in our process for dealing with trivial changes

[Richard Levitte]

clacheck modification coming up!

Cheers,
Richard

On Fri, 13 Dec 2019 04:48:38 +0100,
Dr Paul Dale wrote:
> 
> 
> A better example of this problem: #10607.  Both Paul and I approved it 
> yesterday and I merged it
> today without noticing until too late that it was tagged “CLA: trivial” :(
> I’ve not reverted it at this point but will if necessary.
> 
> Let’s get the label in.
> 
> Pauli
> -- 
> Dr Paul Dale | Distinguished Architect | Cryptographic Foundations 
> Phone +61 7 3031 7217
> Oracle Australia
> 
> On 13 Dec 2019, at 11:02 am, Richard Levitte  wrote:
>
> On Thu, 12 Dec 2019 22:31:10 +0100,
> Dr Paul Dale wrote:
> 
> A red blocker along the lines of: “Triviality Unconfirmed”. One of
> the reviewers needs to remove this before the PR can be merged.
>
> It’s in our face, it prevent accidental merges and its low overhead.
> 
> I still think simply adding the label should be sufficient.  I dunno
> about you, but I look at labels all the time, for all sorts of
> reasons, and one saying [cla: trivial] would certainly attract my
> attention.
>
> Let's make it bright red-orange, that'll catch anyone's eye (even mine)
>
> Also, removing that label will rapidly be annoying as soon as someone
> closes and re-opens a PR...  or whatever other action that triggers
> the "pull_request" event (and there's a lot that does that...  our
> script is being kept busy!).
>
> Cheers,
> Richard
>
> --
> Richard Levitte levi...@openssl.org
> OpenSSL Project http://www.openssl.org/~levitte/
> 
> 
-- 
Richard Levitte levi...@openssl.org
OpenSSL Project http://www.openssl.org/~levitte/

Re: AW: Flaw in our process for dealing with trivial changes

[Richard Levitte]

On Fri, 13 Dec 2019 08:58:27 +0100,
Dr. Matthias St. Pierre wrote:
> 
> 
> > On Thu, 12 Dec 2019 22:31:10 +0100,
> > Dr Paul Dale wrote:
> > >
> > > A red blocker along the lines of: "Triviality Unconfirmed". One of
> > > the reviewers needs to remove this before the PR can be merged.
> > >
> > > It's in our face, it prevent accidental merges and its low overhead.
> > 
> > I still think simply adding the label should be sufficient.  I dunno
> > about you, but I look at labels all the time, for all sorts of
> > reasons, and one saying [cla: trivial] would certainly attract my
> > attention.
> > 
> > Let's make it bright red-orange, that'll catch anyone's eye (even mine)
> > 
> > Also, removing that label will rapidly be annoying as soon as someone
> > closes and re-opens a PR...  or whatever other action that triggers
> > the "pull_request" event (and there's a lot that does that...  our
> > script is being kept busy!).
> > 
> > Cheers,
> > Richard
> 
> 
> This seems to be implied already by my last proposal, with just one color 
> changed:   ;-)

Yes and no...  you talked about addrev, which should not be affected
by this.

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

AW: Flaw in our process for dealing with trivial changes

[Dr. Matthias St. Pierre]

> On Thu, 12 Dec 2019 22:31:10 +0100,
> Dr Paul Dale wrote:
> >
> > A red blocker along the lines of: "Triviality Unconfirmed". One of
> > the reviewers needs to remove this before the PR can be merged.
> >
> > It's in our face, it prevent accidental merges and its low overhead.
> 
> I still think simply adding the label should be sufficient.  I dunno
> about you, but I look at labels all the time, for all sorts of
> reasons, and one saying [cla: trivial] would certainly attract my
> attention.
> 
> Let's make it bright red-orange, that'll catch anyone's eye (even mine)
> 
> Also, removing that label will rapidly be annoying as soon as someone
> closes and re-opens a PR...  or whatever other action that triggers
> the "pull_request" event (and there's a lot that does that...  our
> script is being kept busy!).
> 
> Cheers,
> Richard


This seems to be implied already by my last proposal, with just one color 
changed:   ;-)

> Add three mutually exclusive [cla: *] labels:

>   [cla: ok] (green)
>   [cla: trivial]   (orange)
>   [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.

Re: Flaw in our process for dealing with trivial changes

[Dr Paul Dale]

A better example of this problem: #10607.  Both Paul and I approved it 
yesterday and I merged it today without noticing until too late that it was 
tagged “CLA: trivial” :(
I’ve not reverted it at this point but will if necessary.

Let’s get the label in.


Pauli
-- 
Dr Paul Dale | Distinguished Architect | Cryptographic Foundations 
Phone +61 7 3031 7217
Oracle Australia




> On 13 Dec 2019, at 11:02 am, Richard Levitte  wrote:
> 
> On Thu, 12 Dec 2019 22:31:10 +0100,
> Dr Paul Dale wrote:
>> 
>> A red blocker along the lines of: “Triviality Unconfirmed”. One of
>> the reviewers needs to remove this before the PR can be merged.
>> 
>> It’s in our face, it prevent accidental merges and its low overhead.
> 
> I still think simply adding the label should be sufficient.  I dunno
> about you, but I look at labels all the time, for all sorts of
> reasons, and one saying [cla: trivial] would certainly attract my
> attention.
> 
> Let's make it bright red-orange, that'll catch anyone's eye (even mine)
> 
> Also, removing that label will rapidly be annoying as soon as someone
> closes and re-opens a PR...  or whatever other action that triggers
> the "pull_request" event (and there's a lot that does that...  our
> script is being kept busy!).
> 
> Cheers,
> Richard
> 
> -- 
> Richard Levitte levi...@openssl.org
> OpenSSL Project http://www.openssl.org/~levitte/

Re: Flaw in our process for dealing with trivial changes

[Richard Levitte]

On Thu, 12 Dec 2019 22:31:10 +0100,
Dr Paul Dale wrote:
> 
> A red blocker along the lines of: “Triviality Unconfirmed”. One of
> the reviewers needs to remove this before the PR can be merged.
> 
> It’s in our face, it prevent accidental merges and its low overhead.

I still think simply adding the label should be sufficient.  I dunno
about you, but I look at labels all the time, for all sorts of
reasons, and one saying [cla: trivial] would certainly attract my
attention.

Let's make it bright red-orange, that'll catch anyone's eye (even mine)

Also, removing that label will rapidly be annoying as soon as someone
closes and re-opens a PR...  or whatever other action that triggers
the "pull_request" event (and there's a lot that does that...  our
script is being kept busy!).

Cheers,
Richard

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

Re: AW: Flaw in our process for dealing with trivial changes

[Richard Levitte]

On Thu, 12 Dec 2019 12:15:30 +0100,
Dr. Matthias St. Pierre wrote:
> 
> 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.

Correct re the clacheck hook (that's the Github hook we're talking
about), but re the update hook on our git server, it does a little
more than that, as already shown.

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

I'm not sure why the [cla: ok] or [cla: missing] labels are needed.
We already have a [hold: cla required] label that comes up when
there's a lack of CLA and of "CLA: trivial" marker, so [cla: ok]
and [cla: missing] seem redundant.

However, contrary to you, I would have the [cla: trivial] label added
automatically!...  whenever clacheck finds a "CLA: trivial" line in
any of the commits.

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

Sounds superfluous, considering there's already a need for two
approvals, as well as the [approved: done] label set.  Yet another
manual label will make zero difference, as long as all reviewers can
clearly see that there's a "CLA: trivial" commit (i.e. that the [cla:
trivial] label has been set by clacheck).

After all, the problem we have hit is that "CLA: trivial" can go
undetected, so let's make sure it doesn't, without adding a lot of
other redundant mechanisms that only make our lives harder, yeah?

Cheers,
Richard

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

Re: Flaw in our process for dealing with trivial changes

[Richard Levitte]

On Thu, 12 Dec 2019 13:06:33 +0100,
Dr. Matthias St. Pierre wrote:
> 
> > > 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?
> 
> Actually, the consistency checks could be done entirely by the
> addrev script, which already uses the GitHub API.

Incidently, I'm looking at a rewrite of addrev to make it a
self-contained script that doesn't need the assistance of gitaddrev,
and that also computes everything in the preparation sweep, so the
filter part just needs to do the editing (i.e. should work *much*
faster).

> git commit hook
> =
> 
> Accept pull request if and only if the CLA is on file or *all*
> commits have the "CLA: trivial" annotation.
> 
> (The git commit hook would need only a minimal change, if it does
> not check *all* commits yet.)

git education: It's the post-receive or update hooks, not the commit
hook.  There are commit hooks, and they are all client side, so
talking about a commit hook in this context is *really* confusing,
especially for those who aren't qutie aware.
Ref: https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks

That being said, the update hook does what you say, on a per commit
basis.  In other words, for each commit, it will do the appropriate
checks.  This is the leading comment of that script:

  # For any revision between oldrev and newrev, print VREF/UNREVIEWED/{sha}
  # if it hasn't been reviewed enough.
  #
  # The rules for being reviewed enough are:
  #
  # - UNLESS there's a 'CLA: Trivial' line:
  #   - at least one of the author and the reviewers MUST have a CLA and MUST
  # be member of the 'omc' group.
  #   - at least two of the author and the reviewers MUST have a CLA and MUST
  # be member of the 'commit' group.
  # - IF there's a 'CLA: Trivial' line:
  #   - at least one of the reviewers MUST have a CLA and MUST be member of the
  # 'omc' group.
  #   - at least two of the reviewers MUST have a CLA and MUST be member of the
  # 'commit' group.

(gitolite works by having a denial on any output VREF/UNREVIEWED/ line)

Cheers,
Richard

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

Re: Flaw in our process for dealing with trivial changes

[Matt Caswell]

On 12/12/2019 21:31, Dr Paul Dale wrote:
> A red blocker along the lines of: “Triviality Unconfirmed”. One of the
> reviewers needs to remove this before the PR can be merged.
> 
> It’s in our face, it prevent accidental merges and its low overhead.

Sounds workable.

Matt

> 
> 
> Pauli
> -- 
> Dr Paul Dale | Distinguished Architect | Cryptographic Foundations 
> Phone +61 7 3031 7217
> Oracle Australia
> 
> 
> 
> 
>> On 13 Dec 2019, at 7:06 am, Dr Paul Dale > > wrote:
>>
>> Before we start over engineering a solution, how about we try just
>> having an automatic visual indicator for trivial PRs.
>>
>>
>> Pauli
>> -- 
>> Dr Paul Dale | Distinguished Architect | Cryptographic Foundations 
>> Phone +61 7 3031 7217
>> Oracle Australia
>>
>>
>>
>>
>>> On 13 Dec 2019, at 3:24 am, Kurt Roeckx >> > wrote:
>>>
>>> On Thu, Dec 12, 2019 at 12:10:35PM +, Matt Caswell wrote:

 But in principle I agree that addrev could be used to do this. It's not
 quite as robust as doing it in the commit hook - because you don't
 *have* to use addrev. But, AFAIK, everyone does - so that's probably
 good enough.
>>>
>>> I have never used addrev.
>>>
>>>
>>> Kurt
>>>
>>
> 

Re: Flaw in our process for dealing with trivial changes

[Dmitry Belyavsky]

Great idea.

пт, 13 дек. 2019 г., 0:31 Dr Paul Dale :

> A red blocker along the lines of: “Triviality Unconfirmed”. One of the
> reviewers needs to remove this before the PR can be merged.
>
> It’s in our face, it prevent accidental merges and its low overhead.
>
>
> Pauli
> --
> Dr Paul Dale | Distinguished Architect | Cryptographic Foundations
> Phone +61 7 3031 7217
> Oracle Australia
>
>
>
>
> On 13 Dec 2019, at 7:06 am, Dr Paul Dale  wrote:
>
> Before we start over engineering a solution, how about we try just having
> an automatic visual indicator for trivial PRs.
>
>
> Pauli
> --
> Dr Paul Dale | Distinguished Architect | Cryptographic Foundations
> Phone +61 7 3031 7217
> Oracle Australia
>
>
>
>
> On 13 Dec 2019, at 3:24 am, Kurt Roeckx  wrote:
>
> On Thu, Dec 12, 2019 at 12:10:35PM +, Matt Caswell wrote:
>
>
> But in principle I agree that addrev could be used to do this. It's not
> quite as robust as doing it in the commit hook - because you don't
> *have* to use addrev. But, AFAIK, everyone does - so that's probably
> good enough.
>
>
> I have never used addrev.
>
>
> Kurt
>
>
>
>

Re: Flaw in our process for dealing with trivial changes

[Dr Paul Dale]

A red blocker along the lines of: “Triviality Unconfirmed”. One of the 
reviewers needs to remove this before the PR can be merged.

It’s in our face, it prevent accidental merges and its low overhead.


Pauli
-- 
Dr Paul Dale | Distinguished Architect | Cryptographic Foundations 
Phone +61 7 3031 7217
Oracle Australia




> On 13 Dec 2019, at 7:06 am, Dr Paul Dale  wrote:
> 
> Before we start over engineering a solution, how about we try just having an 
> automatic visual indicator for trivial PRs.
> 
> 
> Pauli
> -- 
> Dr Paul Dale | Distinguished Architect | Cryptographic Foundations 
> Phone +61 7 3031 7217
> Oracle Australia
> 
> 
> 
> 
>> On 13 Dec 2019, at 3:24 am, Kurt Roeckx > > wrote:
>> 
>> On Thu, Dec 12, 2019 at 12:10:35PM +, Matt Caswell wrote:
>>> 
>>> But in principle I agree that addrev could be used to do this. It's not
>>> quite as robust as doing it in the commit hook - because you don't
>>> *have* to use addrev. But, AFAIK, everyone does - so that's probably
>>> good enough.
>> 
>> I have never used addrev.
>> 
>> 
>> Kurt
>> 
> 

Re: Flaw in our process for dealing with trivial changes

[Dr Paul Dale]

Before we start over engineering a solution, how about we try just having an 
automatic visual indicator for trivial PRs.


Pauli
-- 
Dr Paul Dale | Distinguished Architect | Cryptographic Foundations 
Phone +61 7 3031 7217
Oracle Australia




> On 13 Dec 2019, at 3:24 am, Kurt Roeckx  wrote:
> 
> On Thu, Dec 12, 2019 at 12:10:35PM +, Matt Caswell wrote:
>> 
>> But in principle I agree that addrev could be used to do this. It's not
>> quite as robust as doing it in the commit hook - because you don't
>> *have* to use addrev. But, AFAIK, everyone does - so that's probably
>> good enough.
> 
> I have never used addrev.
> 
> 
> Kurt
> 

Re: Flaw in our process for dealing with trivial changes

[Dmitry Belyavsky]

Dear Matt,

On Thu, Dec 12, 2019 at 1:25 PM Matt Caswell  wrote:

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

Well, then it's my misinterpretation. I saw a author's words and green
checkbox related to CLA.
My fault, sorry.


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


-- 
SY, Dmitry Belyavsky

Re: Flaw in our process for dealing with trivial changes

[Matt Caswell]

On 12/12/2019 12:06, Dr. Matthias St. Pierre wrote:
>>> 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?
> 
> Actually, the consistency checks could be done entirely by the addrev script, 
> which already uses the GitHub API.
> 
> addrev:
> =
> 
> Ensure that
> 
> - the [cla: ok] label is set if and only if the CLA is on file
> - the [cla: trivial] label is set if and only if the "CLA: trivial" 
> annotation is present in *all* commits of the PR

It's conceivable (although I don't know how likely) that a PR has mixed
commits from different authors. E.g. a complex PR from one author with a
CLA on file, but with one commit from a different author that is trivial.

But in principle I agree that addrev could be used to do this. It's not
quite as robust as doing it in the commit hook - because you don't
*have* to use addrev. But, AFAIK, everyone does - so that's probably
good enough.

Matt


> 
> git commit hook
> =
> 
> Accept pull request if and only if the CLA is on file or *all* commits have 
> the "CLA: trivial" annotation.
> 
> 
> (The git commit hook would need only a minimal change, if it does not check 
> *all* commits yet.)
> 
> 

Re: Flaw in our process for dealing with trivial changes

[Dr. Matthias St. Pierre]

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

Actually, the consistency checks could be done entirely by the addrev script, 
which already uses the GitHub API.

addrev:
=

Ensure that

- the [cla: ok] label is set if and only if the CLA is on file
- the [cla: trivial] label is set if and only if the "CLA: trivial" annotation 
is present in *all* commits of the PR

git commit hook
=

Accept pull request if and only if the CLA is on file or *all* commits have the 
"CLA: trivial" annotation.


(The git commit hook would need only a minimal change, if it does not check 
*all* commits yet.)

Re: 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

AW: Flaw in our process for dealing with trivial changes

[Dr. Matthias St. Pierre]

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

- This is the first time in quite a while that a pull request with a 
questionable
  trivial CLA has slipped in 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.

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


Matthias

Re: 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 > > 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 > > 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
> 

Re: Flaw in our process for dealing with trivial changes

[Paul Yang]

It seems we have the same thoughts...

Regards,

Paul Yang

> On Dec 12, 2019, at 5:36 PM, Dr Paul Dale  wrote:
> 
> I agree that there is a possible flaw in the workflow.  What’s saved us so 
> far is that new contributors don’t generally include the "CLA: trivial" line 
> or put it in the GitHub text.
> 
> Could we have a “trivial” tag that is added whenever the "CLA: trivial" line 
> is present?  Better would be to add it only if the submitter doesn’t have a 
> CLA on file but either works.
> 
> 
> Pauli
> --
> Dr Paul Dale | Distinguished Architect | Cryptographic Foundations
> Phone +61 7 3031 7217
> Oracle Australia
> 
> 
> 
> 
>> On 12 Dec 2019, at 7:20 pm, Matt Caswell > > 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
> 



signature.asc
Description: Message signed with OpenPGP

Re: Flaw in our process for dealing with trivial changes

[Paul Yang]

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.


Regards,

Paul Yang

> On Dec 12, 2019, at 5:29 PM, Dmitry Belyavsky  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  > 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



signature.asc
Description: Message signed with OpenPGP

Re: Flaw in our process for dealing with trivial changes

[Dr Paul Dale]

I agree that there is a possible flaw in the workflow.  What’s saved us so far 
is that new contributors don’t generally include the "CLA: trivial" line or put 
it in the GitHub text.

Could we have a “trivial” tag that is added whenever the "CLA: trivial" line is 
present?  Better would be to add it only if the submitter doesn’t have a CLA on 
file but either works.


Pauli
-- 
Dr Paul Dale | Distinguished Architect | Cryptographic Foundations 
Phone +61 7 3031 7217
Oracle Australia




> On 12 Dec 2019, at 7:20 pm, Matt Caswell  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

Re: Flaw in our process for dealing with trivial changes

[Dmitry Belyavsky]

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

Flaw in our process for dealing with trivial changes

[Matt Caswell]

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