Venus: How do contributors become maintainers

[mishmosh]

Merging to go-filecoin requires an approval from 1 of {@phritz, @dignifiedquire, @whyrusleeping} (call them _maintainers_ for this discussion) + 1 other team member.

In the 5/29 retro we agreed that:

• increasing the number of maintainers is a long-term goal
• everyone currently working on the project sh/could become a maintainer if they choose
• we want to write down the responsibilities & expectations of maintainers, so we can nudge more people that way.
  (Thanks @aboodman for the proposal + discussion.)

So, what are responsibilities & expectations of maintainers? What would I need to demonstrate to earn the role? I think we'll want more specific criteria at the end of this process, but starting broadly:

• Know the entire system very well, keeping an eye towards unintended consequences
• Know when you _don't_ know something, and use good judgment to refer to another reviewer (maybe they have particular knowledge about ci or security or...)
• Use reviews as a chance to teach rather than ding.

Please chime in!

[phritz]

Notes from https://github.com/filecoin-project/go-filecoin/issues/712 which I marked as a dup:

Our vision for working groups is that they'll become self-sufficient. To be self-sufficient, they must be able to design, implement, merge, and ship -- meaning that each WG should have 1 or more maintainers.

The current model is that we have 3 maintainers for the entire Filecoin project. This won't scale with our WG plan.

Maintainers need:

• An excellent understanding of how the system fits together
• A willingness to enable other people, and review in a timely fashion
• Ability to write constructive reviews, that both ensure quality code and grow knowledge of frequent contributors.
  They do not necessarily need to understand 100% of the codebase, esp. if they are maintainers for subsections. They do need the judgement and humility to call on others' expertise when needed.

What else?
How to identify and establish new maintainers:

• Should current maintainer cabal nominate people?
• Should we establish maintainers per repo or even folder (this is possible in Github)?
• Should we create a trial pathway, where WG captains are provisional maintainers, and if things aren't horribly wrong after 2 months, they are confirmed as full maintainers?

Notes from 8/7 dev meeting:

Clarify partial maintainership?

What's the extent to which paranoia and potential for bad actors should influence our design? (What's our threat model? How do we defend against those? There's an actual history of bad actors masquerading as good contributors for a while, then inserting backdoors. Also physical threats.)

Could we split trust from progress? @dignifiedquire proposes a possibility that merging to master & shipping new releases may be slow, but committing code could be faster.

[mishmosh]

Interim notes and findings as we work towards a maintainership process:

1. One of the best ways to prove maintainer ability is by doing maintainer responsibilities (the stuff under "Maintainers need", above).
2. Decision should take into account the maintainer role within a selected governance structure(s): BDFL, earned voting/meritocracy, or liberal contribution/lazy consensus.

Some answers to prior questions:

Should current maintainer cabal nominate people?

No. Even if the current maintainer group ultimately decides, nominations should be made by anybody, and should include accompanying evidence.

Should we establish maintainers per repo or even folder (this is possible in Github)?

Yes, for repo level. The most likely candidate for folder-level permissions is infra, and it would be cleaner to move that work out to a separate repo (cc @eefahy @gmasgras).

Should we create a trial pathway, where WG captains are provisional maintainers, and if things aren't horribly wrong after 2 months, they are confirmed as full maintainers?

Yes, we should create a trial pathway. I'm inclined to define by active committers & approvers, rather than by WG captaincy. They're the same in many cases, but committers/approvers are more cleanly tied to repo activity and permissions.

Data points: code review count by repo

_Use this data as information, and not as over-simplified verdict._

Code reviews in go-filecoin
dignifiedquire 221
whyrusleeping 170
phritz 165
frrist 116
porcuquine 69
acruikshank 42
zenground0 39
aboodman 31
eefahy 3
mishmosh 3
nicola 3

Code reviews in rust-proofs
porcuquine 50
dignifiedquire 44
laser 14
nicola 1

[mishmosh]

I'll prepare a written proposal for Glasgow. We can discuss and finalize there.

[dignifiedquire]

Thank you!

[phritz]

@mishmosh would you like me to take over wrangling this bit or would you still like to make a proposal? If you want to hand it over let me know if you have any additional source material that is not captured in this issue.

[phritz]

@mishmosh we would like to sort this out in the next month or so. Something missing is a threat model from why/juan -- what specific risks would they like to address.

[phritz]

Squinting at what has been suggested previously in this issue, here's a strawperson:

Problems we are solving:

• prevent bad persons from intentionally merging code that introduces bad things like vulnerabilities, etc
• lower the probability of accidental introduction of unintended consequences/bugs, esp wrt tricky areas of the codebase
• ensure PRs meet our standards wrt usefulness, appropriateness, testing, etc.
• ensure we have an active and engaged set of people contributing to the project in the above ways so that code merges quickly and efficiently, and that it is clear how to become one of those people
  
  

Problems we are NOT solving:

• per file/package maintainership. We will need this, but I don't think we need it yet.
  
  

Definition: a maintainer is someone

1. who is invested in and broadly familiar with a part of the filecoin project, as demonstrated by a history of material technical-, process-, and/or project-level contributions; and
2. who is actively engaged in moving the project forward by enabling others through planning, code reviews, design feedback, etc.; and
3. who is a model of trustworthiness, technical judgement, civility, and helpfulness
   Scope: a maintainer has github Write permissions for a related set of repos, meaning they can merge PRs and manage issues. The set of repos for go-filecoin maintainership is go-filecoin and the project management repo.
   
   
   
   • Not in scope: github admin access (probably need a separate process), who can release (TBD), file-level permissions (we don't have the need yet)
   
   
   • Process: someone becomes a maintainer by conclusively demonstrating they are doing the job of a maintainer. Anyone can nominate someone for go-filecoin maintainership by filing an issue in the project management repo pointing to evidence that the candidate has been doing the job and meets the three requirements above. Existing maintainers must unanimously approve the new candidate. To remove a maintainer requires either self-nomination or confirmation by 2/3 of existing maintainers.
   
   
   • Specific responsibilities:
   
   
4. give helpful code reviews in a timely and friendly fashion
5. demonstrate broad comprehension the system, especially knowing when and who to defer to as a reviewer and with an eye towards unintended consequences
6. demonstrate comprehension of project trajectory (roadmap, current plan, etc) and processes (eg, contributing guidelines)
7. demonstrate active project custodianship, eg meaningfully participate in project-level planning, propose process and/or technical improvements, shoulder responsibilities, take on hard project-wide projects, etc
8. make significant, direct technical contributions
   
   
   
   • Proposed initial set of go-filecoin maintainers: whyrusleeping, phritz, acruikshank. Also laser qualifies but is deep in proofsland so unless he wants/needs it I think let's be parsimonious.
   
   

TODO

• [ ] TODO(fritz) RFM label and process documentation; if RFM then maintainer ensures it got enough review
• [ ] TODO(fritz) not a goal to have a large number of maintainers; also keep process vague
• [ ] feedback from everyone
• [ ] tighten up github permissions:
  
  
  
  • dismiss approvals when new changes are pushed. We can't be too careful.
  
  
  • winnow admin users down to the absolute minimum (eg, why, dig, phritz). Or, if flaky tests are still a problem then only admins can really merge, so maybe make maintainers admins until test flakiness is under control?
  
  
• [ ] let infra, proofs, protocol, etc determine if they want the same model or a different bar
• [ ] TODO write access process: give to someone who has made material contributions and would need it; ask for it
• [ ] TODO clarify write vs merge

[shannonwells]

Suggest also, that there be a code of conduct, and outline reasons and a process whereby someone is relieved of their maintainer role.

[whyrusleeping]

I'm hesitant to say that maintainer == ability to merge PRs. There should probably be a bit more qualification to that.

Generally, we should pull as much as possible straight from how things are done in ipfs: https://github.com/ipfs/community/
Minimal wheel reinvention is good. That said, our threat model is quite a bit more interesting, we have to be concerned about people motivated to introduce vulnerabilities that may allow them to steal peoples money. Certain aspects of the codebase should require much stricter review (arguably, this 'stricter' review should apply everywhere).

[mishmosh]

The latest definition of maintainer + their responsibilities is really nice.

I think we should improve upon IPFS with better pathways to maintainership -- a midpoint designation.

Without clearer pathways to deeper project involvement, these 2 statements are likely to yield a chicken-and-egg 🐣situation:

1. someone becomes a maintainer by conclusively demonstrating they are doing the job of a maintainer
2. demonstrating they are doing the job of a maintainer includes demonstrating active project custodianship, meaningfully participate in project-level planning, propose process and/or technical improvements, shoulder responsibilities

The specific solution is tbd. Trial maintainership was discussed earlier in this thread. Another, very different, example is npm who define a collaborator role with minimally more github permissions.

Other comments:
@shannonwells do you mean a code of conduct for the project (which already exists) or specifically for maintainers?

[phritz]

Generally, we should pull as much as possible straight from how things are done in ipfs: https://github.com/ipfs/community/

All the ipfs community docs seem great. But I do not see a definition of maintainer or the path to get there in this repo. Am I missing something? I think (and I updated the proposal to make this explicit) that the problem we are solving is who gets write power? My suggestion is: they get that power by demonstrating that they really good custodianship of it. If there is an alternative model we want that's fine, but we need a concrete proposal.

Without clearer pathways to deeper project involvement, these 2 statements are likely to yield a chicken-and-egg 🐣situation:

I do not think it is chicken egg. You show you can do the job (1) then you get the job (2). I agree that we could make the path easier and ultimately we should -- there could be a ride-along period where someone interested shadows code reviews then when comfy takes the front seat, etc. But the question is do we need it now? My suggestion is no. We just need something really simple right now.

[shannonwells]

@mishmosh I meant for the project; I didn't know there already was one. In that case, I'd say reference it in the guidelines.

[phritz]

• Problem: 1 filecoin is tricky and it is easy to accidentally introduced a bug; 2 bad actors might try to intentionally introduce bugs; 3 ensure that what is being merged is appropriate in terms of desirability (we actually want this change right now) and Fitness (it is well tested etc). Do all this without slowing development down or overloading a small number of people with all the code review
• solution: We have a small number of maintainers who merge code. There are three maintainers:phritz, why, alexc. It is not a goal to have a large number of maintainers. Maintainers are added and removed by unanimous decision of the other maintainers.
• process: PR gets reviewed by at least one appropriate reviewer who is not a maintainer. Once those reviews are complete the author rebases and labels the pr rfm ready for merge. Maintainers look at rfm prs and decide what to do: Merge the code, request changes, or add additional reviewers. Reviewers should use maintainer feedback as an opportunity to learn about what issues to catch earlier.

Todos:

• add rfm label
• disable changes to master except for maintainers in github
• disable changes after approval in github
• and this policy 2D contributing guide
• announce new process and policy to the team

[phritz]

@mishmosh this is ready to go into gh

[laser]

disable changes after approval in github

This is going to be brutal, given how many developers are on this project and the frequency at which we need to rebase branches before merging them into master.

[phritz]

good point, lets not do that.

[mishmosh]

@phritz let's leave this open for comment for 2 more days, in case other gotchas are found. i can prep [most of] the PR ahead of time, though.

[anorth]

@phritz's proposal moves a bunch of review work off maintainers, in exchange for a bunch of mechanical work (merging branches) onto them. Distributing code review more is a good direction, but adding admin work to maintainers less so. Leaving merging to maintainers is likely to add latency to PRs and additional rebasing pain. How about this:

• "maintainers" are a small group as @phritz suggests. A PR must be approved by a maintainer before merge.
• "committers" are trusted team members, a larger group than maintainers and will grow to include community members. they have a demonstrated and continuing time commitment to the project. committers have write access to the repo, in particular for merging their own commits. committers are trusted to follow process.
• PRs are first reviewed by one committer (usually not a maintainer). After committer approval they are tagged for maintainer, who can add reviewers, request changes, or approve.
• With maintainer approval, committers rebase and merge their own PRs.
• With maintainer approval, the committer who reviewed a PR from a non-committer does the legwork of rebasing and merging.

The key diff is that maintainer approval is required, but they don't do the merge. Added complexity is the concept of a committer, which maps ~ to the current team.

We could use GitHub teams for maintainers and committers. I don't know whether GH can enforce the requirement for approval from a maintainer (rather than just committer), but it doesn't matter because we trust committers to follow process. This "human" enforcement also adds a little leeway in case something goes wrong like maintainers being unavailable, or a super-emergency when maintainers are asleep.

Edit: I haven't addressed how contributors become committers or maintainers, but for people currently external to the project, the first step is committer, which still has some oversight.

[phritz]

@anorth i like it, the only question i have is whether it adequately addresses problem 2 in the problem statement, a bad person trying to slip something in. committers will have to have a high degree of vigilance and the inclination to not merge something they dont understand, and to pay attention to late-arriving commits post approval. this is maybe made harder by rebasing. i think we just need really clear policy in this department.

we should get @whyrusleeping 's input on the above

[whyrusleeping]

Requiring a maintainers approval, and dismissing reviews after new pushes seems like a fine tradeoff to make. It does seem like strictly more work on the maintainers (who in the previous proposal simply click merge after a cursory look over the changes, ensuring the proper people reviewed and approved) but i'm not against an increase in the strictness of review.

The key requirement is that the maintainers are aware of every change being made, and always are in the position of making the judgement call of what gets in vs not.

[mishmosh]

[phritz]

• lets say at least 1 reviews for each of these. sometimes you need multiple expertise or its super tricky and you want more eyes.
• i think for committers we should just say that they are stewards of the project and not list specific requirements for it yet. main qualification is broad familiarity with the code, knowing what is tricky and whose expertise is required for different kinds of changes. this requires a lot of context.
• committers should ensure that the right reviewers are on the right code reviews

i do think its a good idea to give the goals explicity somewhere

[mishmosh]

Not all decisions finalized, but it's time to move the proposal into PR: #1832.