Discussion: Codify How Pull Requests are Landed #86

Activity

1. lance

   added

   type/enhancementNew feature or requestNew feature or request

   type/questionFurther information is requestedFurther information is requested

   on Apr 29, 2020
2. lance commented on Apr 29, 2020

   

   lance

   on Apr 29, 2020

   MemberAuthor

   More actions

   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.
3. lance

   added

   type/discussionIssues that need to be decided/debated/discussedIssues that need to be decided/debated/discussed

   on May 1, 2020
4. danbev commented on May 4, 2020

   

   danbev

   on May 4, 2020

   Contributor

   More actions

   • 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?
5. helio-frota commented on May 4, 2020

   

   helio-frota

   on May 4, 2020

   Member

   More actions

   Yeah probably 72 hours will fit better.
6. lance commented on May 4, 2020

   

   lance

   on May 4, 2020

   MemberAuthor

   More actions

   @danbev that sounds reasonable to me.
7. grant commented on May 4, 2020

   

   grant

   on May 4, 2020

   Member

   More actions

   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.
8. lance commented on May 6, 2020

   

   lance

   on May 6, 2020

   MemberAuthor

   More actions

   @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.
9. slinkydeveloper commented on May 12, 2020

   

   slinkydeveloper

   on May 12, 2020

   Member

   More actions

   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
10. lance commented on May 12, 2020

    

    lance

    on May 12, 2020

    Last edited by lance

    MemberAuthor

    More actions

    @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
11. slinkydeveloper commented on May 12, 2020

    

    slinkydeveloper

    on May 12, 2020

    Member

    More actions

    I agree with that second version 😄
12. lance

    added a commit that references this issue on May 21, 2020

    docs: add maintainer guidelines for landing PRs

    ...

    f3bca65
13. lance

    closed this as completedin #177on May 26, 2020
14. lance

    added a commit that references this issue on May 26, 2020

    docs: add maintainer guidelines for landing PRs (#177)

    ...

    fdc79ae