Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define a policy using draft PRs when waiting on specific approvals #3802

Open
handrews opened this issue May 14, 2024 · 5 comments
Open

Define a policy using draft PRs when waiting on specific approvals #3802

handrews opened this issue May 14, 2024 · 5 comments

Comments

@handrews
Copy link
Member

We've had some understandable confusion on when PRs get merged, as we came up with some exceptions to the "2 TSC approvals" rule in TDC calls. But not everyone is at every TDC call, nor can everyone be expected to remember everything that came up. So we should put PRs into draft, even if they are otherwise ready for merging, if we want to wait for a specific approval.

Just filing this b/c I don't have time to try to update DEVELOPMENT.md but we should do that at some point.

@handrews
Copy link
Member Author

Another option is that we could define a minimum time for non-trivial PRs to remain open. (An examle of a trivial PR would be #3804, although we could use labels if we want to avoid vague heuristics).

@karenetheridge
Copy link
Member

"in draft" and "waiting for specific approval" seem contradictory to me. Is there a specific label that could be used here? Or assign the review to the tsc team? (Is there a merge policy that could be set up to require N reviews from that team before it can be merged, without getting in the way of other PRs that have a different merge policy?)

@lornajane
Copy link
Contributor

I think we have two things here:

  • we should probably set the system to required two reviews
  • sometimes we're looking for feedback from someone in particular

Draft is fair enough if the author does not believe that the PR can currently be merged (pending more changes, someone particular to review it, something else).

@miqui
Copy link
Contributor

miqui commented Sep 29, 2024

HI @lornajane , @handrews , @ralfhandl Did we ever close the loop on this on a prior TDC?

@ralfhandl
Copy link
Contributor

I think we discussed this, in a TDC call and in Slack:

  • Technically only one approval is required to allow emergency fixes by two TSC members.
  • We usually merge once there are two approvals and no objections (reviews that requested changes).
  • If there were requested changes, we usually wait until these reviewers explicitly approved even if there are two other approvals.
    • We could use the "review" label in this case to make this more prominent, or add a new specific label such as "re-review required".
  • We use Draft PRs for changes that depend on other PRs to be merged first, or for decisions to be made by the TSC

Not sure whether these rules belong into DEVELOPMENT.md or CONTRIBUTING.md.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

5 participants