Broker repo governance

[Dirkjan Ochtman]

All,

I'm used to open source projects where ownership is a bit stronger
than what's currently happening with the broker. Also, since the
broker is security-sensitive, having thorough reviews is quite
important, and I feel a bit uncomfortable with stuff getting merged
before I've had a chance to review it.

I'd personally prefer a model where:

1. Everything that gets merged to master is reviewed by someone other
than the author
2. The set of portier-broker reviewers is separated from the GitHub
org committers
3. The set of people who can merge is initially just me, hope to add
Stephan soon

Finally, I personally often interpret :thumbsup: reactions to be more
like "great idea" then "I've fully reviewed this", so maybe we can be
a bit more explicit whether something is reviewed before it gets
merged.

I don't think this will slow the process down by a lot, but we can at
least ping explicitly if things take too long and start mitigating if
there are structural problems with slow reviews.

Cheers,

Dirkjan

[Dan Callahan]

Hi Dirkjan,

Thank you for raising this question, for your work building the broker,
and for your mentorship of stephank as he became familiar with
contributing to the broker. Portier would not be where it is without you.

Responses inline below.

On 10/12/16 13:43, Dirkjan Ochtman wrote:
> I'm used to open source projects where ownership is a bit stronger
> than what's currently happening with the broker.

Can you be more specific about what you mean by "what's currently
happening?"

The only activity I'm aware of that wasn't initiated or reviewed by you
in the past week has been:

1. I reviewed and merged stephank's PR at
https://github.com/portier/portier-broker/pull/19
2. I submitted and merged my PR at
https://github.com/portier/portier-broker/pull/22

Do you believe that either of these need to be reverted and revisited?

> I feel a bit uncomfortable with stuff getting merged
> before I've had a chance to review it.

I don't think that degree of individual ownership is necessarily healthy
for a communal effort. Portier belongs to all of us, and we need to be
comfortable trusting one another to act responsibly and in the best
interests of the project.

What can we do to help reinforce that mutual trust?

> I'd personally prefer a model where:
>
> 1. Everything that gets merged to master is reviewed by someone other
> than the author

I believe we already work under this principle, aside my potential
transgression at https://github.com/portier/portier-broker/pull/22,
where I believed I had an r+ from stephank based on concurrent
discussions in IRC, but I was not completely explicit about it. I
apologize for merging that without confirming the review, and I will be
more explicit in the future.

Does anything need to be done here beyond a reaffirmation that this is
our policy?

> 2. The set of portier-broker reviewers is separated from the GitHub
> org committers

For our current scale, I'm strongly opposed to that level of
granularity. I'd prefer to align ownership and rights with our
governance document at
https://github.com/portier/portier.github.io/blob/master/Governance.md.

Specifically, this means:

1. Change the Member role to only grant Read access to repositories
2. Synchronize the Owner role with Governance.md

Resulting in:

- Owners: buro9, callahad, djc, onli, skorokithakis
- Members: helixbot, jleclanche, tazetschnitzel

(I've gone ahead and done this proactively, since the helixbot account
probably shouldn't have write access to our repos.)

> 3. The set of people who can merge is initially just me, hope to add
> Stephan soon

(a) What's blocking granting a commit bit to Stephan right now?

(b) We already have a governance model for the project at
https://github.com/portier/portier.github.io/blob/master/Governance.md,
which enumerates me, you, buro9, onli, and skorokithakis as committers.

I prefer to operate from a default position of trust, and I trust each
of those individuals. Why should we deviate from that policy?

> Finally, I personally often interpret :thumbsup: reactions to be more
> like "great idea" then "I've fully reviewed this", so maybe we can be
> a bit more explicit whether something is reviewed before it gets
> merged.

As mentioned above, I apologize for merging #22 too quickly, and I'll
gladly be more explicit about that in the future.

Best,
-Dan

[Dan Callahan]

On 10/12/16 15:23, Dan Callahan wrote:
> On 10/12/16 13:43, Dirkjan Ochtman wrote:
> > 1. Everything that gets merged to master is reviewed by someone other
> > than the author
>

> Does anything need to be done here beyond a reaffirmation that this is
> our policy?

In IRC, stavros and onli brought up GitHub's recent support for "protected branches"

I've set portier-broker's master branch to protected, and set all pull requests to require a review before merging.

-Dan

[Stavros Korokithakis]

For what it's worth, I agree with DJC, and, I think, also with Dan. I
really really like requiring everything to be reviewed before merging to
master, but I don't think committers should be separate from reviewers.
If someone can be trusted to commit, why should they not be trusted to
review?

Personally, I wouldn't review a piece of code I wasn't familiar with, so
I think that, between us, the social contract is enough, I don't think
we need to enforce it by having separate reviewer groups at the GitHub
level.

I also like the idea of master being protected and everything in it
going through a review process, it has worked very well for me in the past.

That's my 5 cents!

Stavros

[Dirkjan Ochtman]

On Wed, Oct 12, 2016 at 10:23 PM, Dan Callahan <dan.ca...@gmail.com> wrote:
> Can you be more specific about what you mean by "what's currently
> happening?"
>
> The only activity I'm aware of that wasn't initiated or reviewed by you in
> the past week has been:
>
> 1. I reviewed and merged stephank's PR at
> https://github.com/portier/portier-broker/pull/19
> 2. I submitted and merged my PR at
> https://github.com/portier/portier-broker/pull/22
>
> Do you believe that either of these need to be reverted and revisited?

No, I don't want to revert or revisit anything at this point. However,
these at least made me uncomfortable enough that I'd like us to
clarify the governance model for the future.

There was also onli merging #13 in a manner that I felt was a bit
hasty, although in that case the review cycle had taken a long time.

>> I feel a bit uncomfortable with stuff getting merged
>> before I've had a chance to review it.
>
> I don't think that degree of individual ownership is necessarily healthy for
> a communal effort. Portier belongs to all of us, and we need to be
> comfortable trusting one another to act responsibly and in the best
> interests of the project.
>
> What can we do to help reinforce that mutual trust?

So it's not really about "me", per se, but about people who are
sufficiently versed in knowledge of the broker code base. In the
Portier model, the broker is a critical piece of security-sensitive
infrastructure. Thus, making changes to it should not be done lightly.
From my perspective, I feel that some of the changes mentioned above
were made without sufficient deliberation, and in that sense I did not
feel you (and to a lesser extent, onli) acted responsibly.

>> 1. Everything that gets merged to master is reviewed by someone other
>> than the author
>
> I believe we already work under this principle, aside my potential
> transgression at https://github.com/portier/portier-broker/pull/22, where I
> believed I had an r+ from stephank based on concurrent discussions in IRC,
> but I was not completely explicit about it. I apologize for merging that
> without confirming the review, and I will be more explicit in the future.
>
> Does anything need to be done here beyond a reaffirmation that this is our
> policy?

Previously I was pushing my own stuff without review, but I'd like my
future contributions to be reviewed.

>> 2. The set of portier-broker reviewers is separated from the GitHub
>> org committers
>
> For our current scale, I'm strongly opposed to that level of granularity.
> I'd prefer to align ownership and rights with our governance document at
> https://github.com/portier/portier.github.io/blob/master/Governance.md.
>
> Specifically, this means:
>
> 1. Change the Member role to only grant Read access to repositories
> 2. Synchronize the Owner role with Governance.md
>
> Resulting in:
>
> - Owners: buro9, callahad, djc, onli, skorokithakis
> - Members: helixbot, jleclanche, tazetschnitzel
>
> (I've gone ahead and done this proactively, since the helixbot account
> probably shouldn't have write access to our repos.)

It does not make sense to me to equate "generally active and trusted
within the Portier project" with "competent to review broker code
changes". These seem very different roles, and therefore I think the
second should be explicitly communicated about.

>> 3. The set of people who can merge is initially just me, hope to add
>> Stephan soon
>
> (a) What's blocking granting a commit bit to Stephan right now?

I think that's probably a good idea.

> (b) We already have a governance model for the project at
> https://github.com/portier/portier.github.io/blob/master/Governance.md,
> which enumerates me, you, buro9, onli, and skorokithakis as committers.
>
> I prefer to operate from a default position of trust, and I trust each of
> those individuals. Why should we deviate from that policy?

I think I covered this above.

Cheers,

Dirkjan

[Dan Callahan]

On Thu, 13 Oct 2016 10:53:40 +0200
Dirkjan Ochtman <dir...@ochtman.nl> wrote:

> I'd like us to clarify the governance model for the future.

Great! We agree on requiring review, and that's now enforced by GitHub.

The only remaining question is *who* is allowed to review and commit to the broker.

We all are.

We're all professionals, we mutually trust each other, and we're all aware of the limitations of our own expertise. We're also a small enough group for a social contract to be more than sufficient in this case. Obviously, reviews by yourself and Stephan will be given greater weight owing to your greater hands-on experience.

Moreover, everything we do is open and auditable. Merged pull requests can be retroactively reviewed, and if necessary, reverted. Given our tiny scale, handling this on a case-by-case basis is a far more productive use of our time.

> So it's not really about "me", per se, but about people who are
> sufficiently versed in knowledge of the broker code base.

Where "people" means myself and onli, as we're the only folks with code in the broker besides yourself and Stephan.

> It does not make sense to me to equate "generally active and trusted
> within the Portier project" with "competent to review broker code
> changes". These seem very different roles, and therefore I think the
> second should be explicitly communicated about.

At a larger scale I'd agree, but we're barely half a dozen people, only four of whom have commits in the broker. The actual delta you're asking for is "Dan and Onli, I don't believe you're 'competent to review broker code changes.'"

Respectfully, I disagree.

The whole thing is under 1K LoC, and I assure you we're not going to form a cabal and start wantonly committing each others code. We can handle it.

If there's a problem with either of our concrete actions, let's address it directly, rather than proactively worrying about my hypothetical incompetence somehow breaking Portier or rendering it unmaintainable.

> > (a) What's blocking granting a commit bit to Stephan right now?
>
> I think that's probably a good idea.

Done. Stephan now has write access to all portier repositories. Congratulations, Stephan. :)

Best,
-Dan

[Stavros Korokithakis]

+1

On 13/10/2016 05:56 μμ, Dan Callahan wrote:
> I assure you we're not going to form a cabal and start wantonly committing each others code.

So, uh... Do I return the mask, or what?