Team policy for Merging and Review

[choldgraf]

Summary

It would be helpful for us to have a team policy for when to merge PRs, so that we have the right expectations around process and ensure that we don't accidentally merge something that will negatively impact another team member.

I believe there are a couple of scenarios we should consider, though others may have ideas as well!

• PRs that affect our active hub deployments
• PRs that are development-focused but that won't affect users immediately
• PR for documentation that is particularly important to get right
• PRs that set team policy and process
• PRs for documentation that is not crucial to get right

Questions to ask

• When can a team member self-merge a PR? (e.g., after one approval? in some cases without any other approval?)
• When should a team member feel free to merge another team member's PR?

Acceptance criteria

We write up a policy/process in our development workflow page: https://team-compass.2i2c.org/en/latest/practices/development.html

Tasks to complete

• Policy to guide merges in pilot-hubs repo #213
  • Add code review guidelines for changing running infrastructure infrastructure#516
• Agree on the major kinds of PRs that we should consider
• Have a state label on PRs infrastructure#552
• Agree on a "conditions where someone can merge" policy for these kinds of PRs
• Write up in the Team Compass

[choldgraf]

Datapoint: in #177 I merged after a single "approval" because that PR is meant to provide more factually correct information, rather than changing a process or policy of some kind. I probably would have merged that PR in a day or two if nobody objected.

[choldgraf]

Another suggestion I discussed with @sgibson91 was to use a subset of labels for PRs to make it easier to quickly see what "category" a PR falls under. E.g.:

• pr: self-merge: PRs that may be self-merged in a day or so unless somebody objects.
• pr: needs merge: PRs that await any other team member to merge after review
• pr: needs approval: PRs that need approval from one or more team members (add these team members to the PR)

[sgibson91]

Another suggestion I discussed with @sgibson91 was to use a subset of labels for PRs to make it easier to quickly see what "category" a PR falls under. E.g.:

• pr: self-merge: PRs that may be self-merged in a day or so unless somebody objects.
• pr: needs merge: PRs that await any other team member to merge after review
• pr: needs approval: PRs that need approval from one or more team members (add these team members to the PR)

I think where this idea would really flourish is if we could have a board (or filter) where each column translated onto a label, then a team member with a little bit of time on their hands could work through a few PRs in the needs merge column, for example. I'm not sure if this is possible with the current GitHub board setup or if we'd have to wait for the project management release. I'm also worried about board overload...

[choldgraf]

@yuvipanda mentioned in the team meeting that he'd like to take a stab at writing up a team review/merge process, so assigning him on this one!

[yuvipanda]

The pilot-hubs repo is an infrastructure repo, so review and merging has to be slighlty different than when it's a repo for just code. So, let's separate this into two parts - review and merging.

The person doing the merge must always be the person who made the PR. Merging a PR is really doing a deployment, and it's the responsibility of the PR merger to make sure the deployment goes smoothly. If it breaks, they have to fix it. If that isn't possible for some reason (TZs, vacation, etc), someone else must be explicitly tagged in as the person to merge. 'Merge' is same as 'deploy', so merging policies should match deployment policies.

This is separate from review. Ideally, we'll get at least one other person to review the PR before it's merged - they can use the 'approve' feature. Some things probably require no review - adding user names to a list as admins, for example. Or image tag bumps for users. We should make a list of these, and users can self merge those. For others, review should be explicitly requested using the GitHub 'request reviewer' feature. Ideally, there should also be a comment in the PR explaining what the PR author wants reviewed, and by whom. This helps avoid diffusion of responsibility, and makes review easier. I think it's ok to merge pretty much any PR after a single approval.

How does this sound as a base set of principles? We can write these up in a PR, and amend them to match what will be useful for us.

[choldgraf]

This sounds good to me - how about we set up a pull request template for pilot hubs - we can add sections for the major questions we'd like answered for each PR. For example:

# Description of this change

# What the reviewer should pay attention to

# Who will merge this? (only if not the PR author)

And then we pair that with some labels similar to what @GeorgianaElena's suggested in 2i2c-org/infrastructure#552. E.g., I think we could avoid the labels that say whether the PR has been deployed or not, and instead go with a policy like "Once a PR has an approval, the PR author is free to deploy if they wish. Once they deploy the PR, they should merge it as soon as possible."

We could keep the scope: labels, since that feels like an important question to answer for a given PR. We might also think about adding a label for what "type" of PR it is, in order to signal whether it needs an approving review or not.

[damianavila]

How does this sound as a base set of principles? We can write these up in a PR, and amend them to match what will be useful for us.

Those principles sound good to me.

how about we set up a pull request template for pilot hubs - we can add sections for the major questions we'd like answered for each

I like that!

And then we pair that with some labels

And 👍 on using labels to categorize and PRs signalling (even more if we can automate stuff as @sgibson91 suggested in 2i2c-org/infrastructure#552.

[consideRatio]

Excellent discussion! I really appreciate the distinction between repos that are related to making a deployment (2i2c-org/pilot-hubs, jupyterhub/mybinder.org-deploy), and repo's that are related to code for som library or similar (jupyterhub/jupyterhub, jupyterhub/chartpress). Merging will impact them differently.

[choldgraf]

Another example to think about as it pertains to our merge/review policy! I wrote up a few questions and thoughts there as well: 2i2c-org/infrastructure#618 (comment)

[damianavila]

Those are great questions @choldgraf! I have some thoughts but I want to read Yuvi's answer first to get the whole contexts before emitting an opinion.

[yuvipanda]

@damianavila @choldgraf thoughts in my head around this seem to be kinda jumbled and oscillating, so I'll unassign this from myself for now and let someone else pick this up. Hope that's ok!

[choldgraf]

I think that we quietly completed this one and never closed this issue, so I'll close it now :-)

If someone thinks there's something else we need to follow up on, please do re-open or create a new issue!