Who and how many people need to sign-off on a PR, exactly?

[handrews]

• What groups "count" towards approvals
  • of spec PRs
  • of process documentation
  • of workflow/test/javascript environment/build/etc infrastructure
• Which PRs require two approvals and which only one?
  • for two, do both need to be from the same group, or can (for example) a spec PR be merged after 1 TSC and 1 maintainer?
  • do the approval permissions of the submitter ever count towards the 2? (Obviously, they never count towards the 1 or people could just merge their own PRs immediately)
• When we write up these rules, do we want to say anything specific to encourage non-approval-privlege reviews?

[lornajane]

Some very good questions here. I'll pitch my answers but would expect others to do the same and not necessarily agree.

Approvals:

• Everything needs two approvals
• If it's a spec PR, both those approvals need to be TSC people
• Submitter doesn't count

All reviews and PR feedback are very much encouraged and considered valuable - whether your review "counts" or not! I saw discussion somewhere else of nominating experts for different areas but this seems difficult to quantify. I suggest we keep things informal, everyone with commit access is so active in the project that we know whose review we trust on the different areas.

[lornajane]

I just spotted this in DEVELOPMENT.md:

Spec changes should be approved by a majority of the committers. Approval can be given by commenting on the issue itself, for example, "Approved by @webron" however at least one formal GitHub-based flow approval must be given. After voting criteria is met, any committer can merge the PR. No change should be approved until there is documentation for it, supplied in an accompanying PR.

So actually a majority of approval is needed for spec changes, rather than two approvals. I'm not sure if we want to change that, but at least we know it is there and needs changing!

[handrews]

@lornajane definitely someplace where DEVELOPMENT.md is long out-of-date! I think the biggest problem there would be getting enough eyes on it - the new TSC will help but that would mean 5 approvals per spec PR which seems like a tall order if we want to put out multiple releases this year. Or any non-trivial release this year.

We also need a more clear policy for the non-spec things. After @earth2marsh said certain things could be done with one, I've taken to just merging the dependabot changes myself after reading all release notes on all updated packages (the one in the title and any dependencies). So we might want to document stuff like that - "For dependabot, if you have reviewed the release notes of all changes, etc."

[earth2marsh]

Good catch @lornajane -- the majority approval for spec changes needs to be updated IMO. And @handrews , agree that we should add language for changes that are less fragile than to the specc.

[miqui]

TODO: Marsh - action item - look into objections.

[lornajane]

I added a short section to the contributing file to capture our current practice on signing off pull requests https://github.com/OAI/OpenAPI-Specification/blob/main/CONTRIBUTING.md#reviewers