Committer Policy #8

paulidale · 2022-02-02T22:30:06Z

Based over #6 to share the definitions.

Aiming to replace: https://www.openssl.org/policies/committers.html

paulidale · 2022-02-02T22:30:41Z

Markdown conversion and basic import of the current policy (with a few minor corrections).
Links and other updates to occur in due course.

policies/committer-policy.md

paulidale · 2022-02-03T06:54:02Z

Still some more definitions/cross referencing to be done.

policies/committer-policy.md

paulidale · 2022-02-04T04:06:36Z

Feedback addressed.

policies/committer-policy.md

mattcaswell · 2022-02-04T09:40:11Z

policies/committer-policy.md

+the author's review does count towards the two needed.
+
+In the case where two committers make a joint submission, they can review
+each other's code but not their own.  A third reviewer will be required.


So, to be clear, does this mean that in the case where a single PR contains commits from 2 different committers, we only need one additional reviewer? The 2 authors each review the commits authored by the other person, and a 3rd committer reviews all of them?

Yes. Or a 4th also reviews everything (in which case there is no issue at all).

I had trouble capturing this intent without creating a word jam of undue complexity.

I'm not sure that this exception actually makes sense in practical terms. It would require a clearer view of who does what and a separate set of reviews for each commit and in practical terms that doesn't really seem to happen in PRs in general. I would leave this whole concept out and always have two reviewers who are not the author(s) and see how that works out in practical terms. i.e. this is a premature optimisation for a problem we haven't yet experienced with the changed review rule. We should IMHO put in the simple rule and adjust based on experience with the new rule over time.

I'm in total agreement that wording isn't ideal as it currently stands.

I also believe that two completely independent reviewers will be problematic in pretty much any instance where this clause applies.

I don't actually see this changing anything, 3 people are needed for the normal case and this case.

I'm with @paulidale that to require 2 additional people to review something done by joint work of 2 committers would be seriously problematic. And I do not think the current wording is unclear or anything.

policies/committer-policy.md

Fixes openssl#7

…r own work Instead there must be two independent reviews for each submission. With some special cases to handle more complicated but less common situations.

paulidale · 2022-02-17T02:49:23Z

Unless Kurt has further objections, I'd like to call a vote on this:

Topic: Accept the committer policy as of 3766d6ba2648e716e973af6e1821687ad46ee57c
Proposed by: Pauli
Issue link: https://github.com/openssl/general-policies/issues/8
Public: yes
Opened: 2022-02-17
Closed: YYYY-MM-DD
Accepted:  yes/no  (for: X, against: Y, abstained: Z, not voted: W)

  Kurt       [  ]
  Mark       [  ]
  Matt       [  ]
  Pauli      [+1]
  Richard    [  ]
  Tim        [  ]

paulidale · 2022-02-17T02:49:33Z

Paul: [+1]

t-j-h · 2022-02-17T03:18:14Z

Vote: [+1]

t8m · 2022-02-17T08:26:07Z

The bigger number of formal reviews will be required by the process the more formal and less thorough those reviews will be. IMO that is inevitable. However I think this change in requirements to always have two independent reviewers of a code someone wrote is still well within reasonable limits. I would just not expect this change to have as big impact on the code quality as perhaps someone might expect.

mspncp · 2022-02-17T08:45:48Z

policies/committer-policy.md

+All submissions must be reviewed and approved by at least two committers,
+one of whom must also be an OTC member.  Neither of the reviewers can
+be the author of the submission.


As a non-OMC member I ask myself: What is the rationale behind this significant tightening of the rules? 🤔

Mostly it represents a slackness in the current review process.

Well... I'm afraid it will not add much energy to it.

It's about improving quality and ensuring enough eyes look at every change.

I understand intentions but have doubts about results.

I had the same doubts when we introduced the original review process - but I would not go back now. We always have the option of reverting this change if it doesn't work out well.

I agree with the original process, but I think this change will not increase the quality but just processing time.
Could we establish some metrics?

Feasibly we could establish metrics for review time (e.g. measure time from PR Open to PR Close). I'm not sure how you would establish metrics for quality.

In some specific cases, it would be possible to improve the quality of the review by not just looking at the code but also download and build it locally, and then test it independently of the author. But I doubt that such an option could be turned into in a general requirement.

mattcaswell · 2022-02-17T11:20:31Z

policies/committer-policy.md

+each other's code but not their own.  A third reviewer will be required.
+
+An OMC member may apply a _hold: needs OMC decision_ label to a submission.
+An OTC member may apply a _hold: needs OTC decision_ to a submission.


Didn't think of this until now (so probably something to think about after the vote) - but do we mean to restrict things so that only OMC can place an OMC hold and only OTC can place an OTC hold? Shouldn't it be possible for any committer to refer a decision to OMC or OTC if they think it is appropriate?

Only OMC/OTC members should be able to force an OMC/OTC vote by placing a hold. Anyone else (committer or normal community member) still has the possibility to convince one or more OMC/OTC members of his opinion and get them to place the hold.

mattcaswell · 2022-02-17T11:27:04Z

Accept the committer policy as of 3766d6b

Question: This PR seems to have a new unrelated commit in it to define perlasm. Its the same commit as is present in #10. The vote that has been called mentions a commit hash which includes that new commit. Was this intentional? I don't object to the new commit per se but it seems strange to be including it in a vote about committer policy.

mspncp · 2022-02-17T12:47:16Z

Question: This PR seems to have a new unrelated commit in it to define perlasm. Its the same commit as is present in #10. The vote that has been called mentions a commit hash which includes that new commit. Was this intentional? I don't object to the new commit per se but it seems strange to be including it in a vote about committer policy.

I don't see such a commit in this PR. Where did you notice it?

mspncp · 2022-02-17T12:48:45Z

Ah! I understand: Paul referred to the wrong commit id in the vote topic.

paulidale · 2022-02-18T02:08:45Z

It should have been 76bb566.
I don't object to 3766d6b also going in, one less vote is good :)

mattcaswell · 2022-02-18T09:54:10Z

If the vote is actually for 76bb566 then I vote +1. If it is for 3766d6b then I vote 0 since I don't think its a good idea to be mixing votes like this, but I don't actively object to the content.

kroeckx · 2022-02-19T23:54:54Z

I'm voting +1

paulidale · 2022-02-20T23:04:15Z

I went with 76bb566 for simplicity #10 has the other text.

Topic: Accept the committer policy as of 76bb566cf401e05bc54cd81ce429e83e2827278a
Proposed by: Pauli
Issue link: https://github.com/openssl/general-policies/issues/8
Public: yes
Opened: 2022-02-17
Closed: 2022-02-21
Accepted:  yes  (for: 4, against: 0, abstained: 0, not voted: 2)

  Kurt       [+1]
  Mark       [  ]
  Matt       [+1]
  Pauli      [+1]
  Richard    [  ]
  Tim        [+1]

levitte · 2022-03-01T12:34:34Z

Vote: 0

levitte · 2022-03-01T12:35:12Z

(the label "ready to vote" was never set on this one...)

iamamoose · 2022-03-30T10:40:12Z

[0]

paulidale added the policy change A change to a policy is being proposed label Feb 2, 2022

paulidale self-assigned this Feb 2, 2022

paulidale force-pushed the committers branch from a0cc720 to d5e8aca Compare February 3, 2022 06:49

paulidale commented Feb 3, 2022

View reviewed changes

policies/committer-policy.md Outdated Show resolved Hide resolved

t-j-h reviewed Feb 3, 2022

View reviewed changes

policies/committer-policy.md Outdated Show resolved Hide resolved

t-j-h reviewed Feb 3, 2022

View reviewed changes

policies/committer-policy.md Outdated Show resolved Hide resolved

t-j-h reviewed Feb 3, 2022

View reviewed changes

policies/committer-policy.md Outdated Show resolved Hide resolved

paulidale force-pushed the committers branch 3 times, most recently from f445ba7 to 416fcbf Compare February 4, 2022 04:04

paulidale force-pushed the committers branch 3 times, most recently from b70f426 to 62717c2 Compare February 4, 2022 05:47

mattcaswell requested changes Feb 4, 2022

View reviewed changes

paulidale force-pushed the committers branch 3 times, most recently from fed6ea5 to c684118 Compare February 7, 2022 00:46

mattcaswell mentioned this pull request Feb 7, 2022

Version policy #6

Closed

paulidale force-pushed the committers branch from 1019c12 to 148ea6a Compare February 7, 2022 22:53

kroeckx reviewed Feb 16, 2022

View reviewed changes

policies/committer-policy.md Show resolved Hide resolved

kroeckx reviewed Feb 16, 2022

View reviewed changes

policies/committer-policy.md Show resolved Hide resolved

paulidale added 4 commits February 17, 2022 10:31

Committer policy

4e118be

Fixes openssl#7

Committer policy cross linking and minor corrections

7fb4e99

Change the review requirements to not allow an author to approve thei…

8e58262

…r own work Instead there must be two independent reviews for each submission. With some special cases to handle more complicated but less common situations.

Add a disclaimer to the glossary

76bb566

paulidale force-pushed the committers branch from 32938a8 to 76bb566 Compare February 16, 2022 23:40

mspncp reviewed Feb 17, 2022

View reviewed changes

mattcaswell reviewed Feb 17, 2022

View reviewed changes

revote vote outcome

ab2bac8

paulidale force-pushed the committers branch from 3766d6b to ab2bac8 Compare February 20, 2022 23:00

paulidale closed this Feb 20, 2022

paulidale deleted the committers branch May 9, 2022 01:01

Committer Policy #8

Committer Policy #8

Conversation

paulidale commented Feb 2, 2022

paulidale commented Feb 2, 2022

paulidale commented Feb 3, 2022

paulidale commented Feb 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale commented Feb 17, 2022

paulidale commented Feb 17, 2022

t-j-h commented Feb 17, 2022

t8m commented Feb 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mspncp Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

mattcaswell commented Feb 17, 2022

mspncp commented Feb 17, 2022

mspncp commented Feb 17, 2022

paulidale commented Feb 18, 2022

mattcaswell commented Feb 18, 2022

kroeckx commented Feb 19, 2022

paulidale commented Feb 20, 2022 • edited Loading

levitte commented Mar 1, 2022

levitte commented Mar 1, 2022

iamamoose commented Mar 30, 2022

mspncp Feb 17, 2022 •

edited

Loading

paulidale commented Feb 20, 2022 •

edited

Loading