Change our code review policy to increase autonomy (along with psychological safety) for engineers

[yuvipanda]

Description

Reviewing PRs in this repository is important for at least the following reasons:

1. Spreads knowledge around the team, so no single person becomes a bottleneck. It also helps develop good working relationships over time.
2. Helps catch issues before they hit production (via typos, architecture review, testing, etc). This will never be 100%, but the goal is to catch the obvious issues.

These are important reasons, and we should continue doing code review. However, we are also a very small engineering team with limited resources, and proper code review takes a lot of time. We should explicitly make time for review in our sprint planning process (I think @damianavila is going to create a different issue for that, as follow-up from #3918).. Part of that is being more discerning about what needs review, and what does not. By eliminating reviews that don't directly help with the two stated goals earlier, we can free up more capacity for reviews that do.

Scope

This issue is scoped specifically to:

1. This repository (infrastructure/) and https://github.com/2i2c-org/default-hub-homepage.
2. Clearly defining criteria for PRs that can be 'safely' self merged.

Why is this important?

1. Increasing engineering capacity is an important part of extending our organizational runway. By reducing the overall amount of things engineering has to do, we increase our capacity quite a bit :)
2. Code review is very important, and when done correctly, takes a lot of time. This allows us to focus our efforts on where code review gives us most bang for the buck, and

Measures and definition of done

This will mostly include expansion of this documentation section: https://infrastructure.2i2c.org/contributing/code-review/#self-merging-as-a-2i2c-engineer

We can call this done after 1 round of the following tasks are complete. We can re-evaluate in 3 months to see how it goes.

Tasks

Beta Give feedback

1. Restructure https://infrastructure.2i2c.org/contributing/code-review/#self-merging-as-a-2i2c-engineer to be a top level item
2. Review last 3 months of merged PRs to understand where review added value
3. Identify clear criteria for a few more kinds of PRs that can be self-merged
4. Form a shared team understanding, and update the documentation to match that

[yuvipanda]

This initiative is attached to this goal: https://github.com/2i2c-org/meta/issues/989

[yuvipanda]

I've been adding a few recently:

• Explicitly list maintenance changes that can be self merged #3981
• More explicitly list safe config changes to merge #3972
• Allow self-merging new hub PRs #3966

[choldgraf]

A quick thought here on the way to approach this topic. We recently added decision making principles that have a few relevant pieces for this topic:

• Make it safe to try
• Bias towards action
• Trust one another

That makes me feel like our guidelines for teams should start with the assumption that team members take action, and describes the specific places where they might want to slow down, rather than assuming they're moving slowly and telling them when to speed up.¹

So what do you think about approaching the same question from the opposite direction:

• Assume that most infrastructure PRs can be self-merged without review when you think they are safe to do so.
• Here are some circumstances that might bias you towards waiting for feedback:
  • You need feedback to resolve critical questions.
  • The PR probably has wide-ranging impacts on 2i2c's active infrastructure.
  • The PR makes a change that we need others on the engineering team to know about, in order to share knowledge and skills.

And iterate on the guidelines from that approach.

Footnotes

1. This is also a departure from the tone of our engineering review guidelines ↩

[yuvipanda]

I spent the last 4 days thinking about this, and I think the way you're suggesting is actually the way to go here, @choldgraf.

[damianavila]

I spent the last 4 days thinking about this, and I think the way you're suggesting is actually the way to go here, @choldgraf.

I concur, I think @choldgraf's suggestion is the right direction to follow.

This is also a departure from the tone of our engineering review guidelines ↩

Yep, we need to change the tone over there.

In general, I think this change would increase our velocity and free-up some of our capacity, and both things are benefits that outweight the costs we are gonna pay for a reduced review surface.

[yuvipanda]

I've retitled the issue to instead be a more general overhaul of our code review policies in line with #3997 (comment)

[consideRatio]

Thank you @yuvipanda, do you think its OK to let this issue capture the need for guidance on how to request review in situations when you will need it for makes sense to self-merge?

Attempted summary of this topic

• We are heading towards autonomy to decide if you self-merge or not, and this requires some guidance when it makes sense to request review help
• We need clarity on how we want to request review help when we consider review to be needed
  (This is meant to capture what @GeorgianaElena said in the retro meeting today)
• We benefit from a agreed meaning on what draft / non-draft means
  (not previously mentioned, added by me now)

Idea on a path forward

1. We use our best judgement with regards to what can be self-merged or not
2. We bootstrap documentation on strategies to get things merged, where for example a "direct self-merge" is one, and a "delayed self-review self-merge" is another", and a "request and wait for ... and then re-evaluate" etc are example strategies.
3. We define and iterate on guidance on what could make sense to request additional review for and how to balance between strategies to get things merged.

[aprilmj]

I think we fast-tracked this "initiative" during the team meeting last week, and that it may be done. @yuvipanda am I wrong about that?

[yuvipanda]

Yes i plan on working on this today

[sgibson91]

A thought I had: If we plan to pivot to more self-merging and have a deny list, should we remove the requirement in the GitHub UI for requiring a reviewer for merge?

[yuvipanda]

@sgibson91 done!

[yuvipanda]

I've done the 1:1 conversations I wanted to do about this, and it's now up at #4308