Discussion: Codify How Pull Requests are Landed

[lance]

Currently there are only a handful of folks in the @cloudevents/sdk-maintainers group who are actively participating in this repository. But as other contributors come on board, it might be good to codify how pull requests are landed.

For example, can a contributor with committer rights submit, approve and land their own PR all within a single day? A single hour? At the risk of overkill, here's a strawman proposal.

• No pull request may land without passing all automated checks
• All maintainers can land pull requests from outside contributors 24 hours after they have been submitted, given that it has one approval (that approval can be from the person landing the PR)
• If a maintainer has submitted a pull request and it has approval from one other maintainer, it can be landed immediately
• If a maintainer has submitted a pull request and it has not received approval from at least one other maintainer, it can be landed after 48 hours
• If an outside contributor submits a pull request that is very minor - e.g. fixing a typo in a doc - the 24 hour cooling period doesn't apply
• If a pull request has both approvals and requested changes, it can't be landed until those requested changes are resolved.

Thoughts?

[lance]

If a maintainer has submitted a pull request and it has not received approval from at least one other maintainer, it can be landed after 48 hours

By this I mean to say that if there has been no activity on the PR, including approvals. If there is active discussion but no approval it shouldn't be landed.

[danbev]

• If a maintainer has submitted a pull request and it has not received approval from at least one other maintainer, it can be landed after 48 hours

This sounds a little short to me, thinking about weekends for example. Perhaps 72 hours?

[helio-frota]

Yeah probably 72 hours will fit better.

[lance]

@danbev that sounds reasonable to me.

[grant]

I'm not sure what the current issue is. I also think we could just use existing tooling instead of having a doc with rules in presumably markdown docs. Examples:

• Required PR status check: https://help.github.com/en/github/administering-a-repository/about-required-status-checks
• Required reviews: https://help.github.com/en/github/administering-a-repository/enabling-required-reviews-for-pull-requests

I don't want to have an SLO that can't be enforced or that isn't a problem currently.

[lance]

@grant I agree that the automated checks are good. That's why my first guideline is "No pull request may land without passing all automated checks".

But I think some additional organizational structure is useful, particularly for people who live in different time zones. For example, I believe @danbev is 9 hours ahead of you. If we want to have solid engagement from contributors across the globe, it's important to give consideration to the fact that he might want to be included in the pull/review/land process. But if you submit a PR when you get up in the morning, on Friday and land it on Saturday morning, there's a good chance he will have completely missed all of that.

I actually think that these guidelines are even looser than they should be. My original idea was.

• All maintainers can land pull requests from outside contributors 24 hours after they have been submitted, given that it has one approval (that approval can be from the person landing the PR)
• If a maintainer has submitted a pull request and it has approval from one other maintainer, it can be landed after 24 hours
• If a maintainer has submitted a pull request and it has not received approval from at least one other maintainer, it can be landed after 72 hours

(well tbh, I did not have 72 in my original draft, but taking @danbev's cue)

In my view this isn't about SLO, it's about respecting the community and other maintainers, giving them an opportunity to weigh in on pull requests before they land.

[slinkydeveloper]

All maintainers can land pull requests from outside contributors 24 hours after they have been submitted, given that it has one approval (that approval can be from the person landing the PR)
If a maintainer has submitted a pull request and it has approval from one other maintainer, it can be landed after 24 hours

@lance I think we should not have a time minimum for PRs approved from at least one mantainer (other than the author). This slows down hotfixes, like it happened in past in sdk-go where i broke some stuff and we needed an urgent fix to release 😄 If a PR is approved, then it's fine. Of course, it's at discretion of the mantainer to avoid landing a big PR too soon, without proper review

[lance]

@slinkydeveloper good points. I was trying to think of situations where a maintainer might want to stop a pull request from landing but because of time zones may not see it immediately. Do you think it's overkill? There could be other ways to handle hot fixes - with labels for example. But maybe we don't need to bother with this unless it becomes an issue.

So how's this?

• No pull request may land without passing all automated checks
• All pull requests require at least one approval from a maintainer before landing, with one exception noted below
• A pull request author may approve their own PR, but will need an additional approval to land it
• If a maintainer has submitted a pull request and it has not received approval from at least one other maintainer, it can be landed after 72 hours
• If a pull request has both approvals and requested changes, it can't be landed until those requested changes are resolved

[slinkydeveloper]

I agree with that second version 😄