Skip to content

Conversation

bbonenfant
Copy link
Contributor

Per our team meeting yesterday, I'm removing CODEOWNERS.

GitHub's CODEOWNERS model focuses on the activity of code review: the idea is that each part of the codebase has a limited group of engineers who understand it (the "owners"), and PRs must be reviewed by engineers who will understand the changes. If a part of the code only has one owner, it can't be reviewed because there's no second person (the reviewer) who could understand the changes and vet them. Either a second person must learn the code, or the PR should be merged unreviewed—an honest reflection of reality. This makes sense if you believe the purpose of code review is to catch bugs, more or less.

The model we want focuses on knowledge of the code itself: each part of the codebase has a limited group of engineers whose job is to explain that code to others when needed (the "owners"). Within that group, someone should know about each change being made, and just in case that person's not available, there should be a second person who understands the change for redundancy. As the second person is a backup, they don't need to be an owner (though it's preferred if they are)—this allows portions of the codebase to exist with single owners and ensures their knowledge exists in others around the company. Catching bugs, while good, is not the goal or expectation (we aim to do this with tests). The purpose of code review is preservation of context within the company.

We'll look for a system that implements the latter model, but for now, we hope that this will spread understanding of the codebase and help us move faster. It is important that where complex sections have a de-facto owner who's built an understanding of the code (e.g. @bbonenfant and the python SDK, or @brycemcanally and much PFS code), they be included in code reviews they didn't author, to keep their understanding as fresh as possible.

Conflicts:

.github/CODEOWNERS

Per our team meeting yesterday, I'm removing CODEOWNERS.

GitHub's CODEOWNERS model focuses on the activity of code review: the
idea is that each part of the codebase has a limited group of engineers
who understand it (the "owners"), and PRs must be reviewed by engineers
who will understand the changes. If a part of the code only has one
owner, it can't be reviewed because there's no second person (the
reviewer) who could understand the changes and vet them. Either a second
person must learn the code, or the PR should be merged unreviewed—an
honest reflection of reality. This makes sense if you believe the
purpose of code review is to catch bugs, more or less.

The model we want focuses on knowledge of the code itself: each part of
the codebase has a limited group of engineers whose job is to explain
that code to others when needed (the "owners"). Within that group,
someone should know about each change being made, and just in case that
person's not available, there should be a second person who understands
the change for redundancy. As the second person is a backup, they don't
need to be an owner (though it's preferred if they are)—this allows
portions of the codebase to exist with single owners and ensures their
knowledge exists in others around the company. Catching bugs, while
good, is not the goal or expectation (we aim to do this with tests). The
purpose of code review is preservation of context within the company.

We'll look for a system that implements the latter model, but for now,
we hope that this will spread understanding of the codebase and help us
move faster. It is important that where complex sections have a de-facto
owner who's built an understanding of the code (e.g. @bbonenfant and the
python SDK, or @brycemcanally and much PFS code), they be included in
code reviews they didn't author, to keep their understanding as fresh as
possible.
# Conflicts:
#	.github/CODEOWNERS
Copy link
Contributor

@msteffen msteffen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bbonenfant bbonenfant merged commit dd0e3a1 into 2.11.x Sep 13, 2024
24 of 25 checks passed
@bbonenfant bbonenfant deleted the backport-codeowners branch September 13, 2024 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants