Flatcar icon indicating copy to clipboard operation
Flatcar copied to clipboard

[RFE] Setup `CODEOWNERS` file

Open tormath1 opened this issue 9 months ago • 8 comments

Current situation

When an external contributor or a maintainer opens a PR, one needs to manually set flatcar-maintainers as reviewer.

Impact

  1. It's easy to miss
  2. You have to know about the different GitHub teams (nebraska-maintainers, flatcar-maintainers, etc.)

Ideal future situations

There is CODEOWNERS file to automatically set a GitHub team as a reviewer of the PR for a given repo.

Implementation options

  • we define one unique CODEOWNERS file here: https://github.com/flatcar/.github
  • there is a MAINTAINERS file here: https://github.com/flatcar/Flatcar/blob/main/MAINTAINERS.md - we can decide to maintain both file separately (one is CNCF thing, the other one is a GitHub feature) or find a way to merge them
  • flatcar-maintainers should be set as reviewers of all PRs inside the Flatcar organization
  • nebraska-maintainers should be set as reviewers of all Nebraska PRs

Additional information

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners#about-code-owners

tormath1 avatar Mar 03 '25 09:03 tormath1

If the CODEOWNERS file is static, maybe with each commit to the main branch, we could have a job that makes sure that the people present in MAINTAINERS.md are also present in CODEOWNERS and if needed appends them?

John15321 avatar Mar 03 '25 13:03 John15321

After discussing with @John15321 we thought that it might be a good idea to setup a Flatcar repo template (similar to kubernetes-sigs: https://github.com/kubernetes/kubernetes-template-project, https://github.com/kubernetes-sigs/gateway-api)

In this template we could have:

  • An initial README (with the badges)
  • A CODEOWNERS file
  • A License (CNCF compliant)

tormath1 avatar Apr 10 '25 13:04 tormath1

Hi @tormath1 please take a look at this PR and let me know if this is what you would like to have 😄 https://github.com/flatcar/flatcar-template-project/pull/1

John15321 avatar Apr 14 '25 14:04 John15321

TL; DR: Should we continue to request reviews from flatcar-maintainers on all Flatcar related repositories?

In a Nebraska PR, @John15321 raised this concern about ownership of Flatcar's related repos:

I actually wanted to discuss that, as in who should be in that CODEOWNERS file in each repo. Im just really worried that having everyone (that is flatcar-maintainers) be pinged for each PR and that would create some noise and Im wondering if there is a better way of doing that? Especially given that the flatcar-maintainers is now 15 people (14 minus the infra account). Maybe we could rethink our GitHub Teams and the way we assign them to Git repos? Im just worried that with 63 repos there will be just more and more noise


We are requesting reviews from flatcar-maintainers on all Flatcar's related repositories since a while now and I personally find it useful as it allows to have an overview on what's going-on on Flatcar. Example: there is a request on the bootengine repo, maybe I won't review it but in 3 weeks if something is broken on the bootengine I would remember that something has been done here.

I'm -1 to request reviews from individuals only (e.g Foo is requested for reviews but Foo is the author of the PR + bus factor).

Something we could imagine:

  • flatcar-maintainers: this team stays and we can review who's in this team / folks not interested to be pinged on all Flatcar's PR
  • flatcar-os: we create a team focused on what actually makes Flatcar (e.g scripts, bootengine, mantle, update_engine, etc. (TBD))
  • what about the all other projects?

That said, I would like to hear @flatcar/flatcar-maintainers opinion on this.

NOTE: This is part of the Flatcar Linux Infra redesign too as this segmentation fits in the RBAC design.

tormath1 avatar Apr 17 '25 08:04 tormath1

I like how it is too for the same reasons. I'll admit that I usually ignore the requests from Ervin as I know practically nothing about Nebraska, but I have approved one or two where they looked straightforward enough, just to be helpful.

chewi avatar Apr 17 '25 09:04 chewi

I fully agree that its important to let people have the ability of seeing that something is happening in other projects in which they do not "specialize". But I also think we could use the notion of having a few more GitHub Teams, maybe a Team per group of repos. For example:

  • flatcar-os - scripts, bootengine, mantle etc...
  • flatcar-nebraska - already exists but we could imagine having some amount of repos connected to that
  • flatcar-apps - with the growing number of apps
  • etc...

Such that all repositories have a Team assigned to them and are automatically assigned on any PR filed for any of their repositories (people can be of course in multiple Teams).

I think there are several benefits to this approach as well as edge cases in the contribution/review process that we could avoid thanks to this:

  • For smaller contributions, yes, flatcar-maintainers would usually not be tagged, therefore most people would not be aware of those smaller changes. However lets assume there is a bigger PR, for example associated with nebraska, the flatcar-maintainers could be tagged, and that way we would know that even if Im not doing anything with nebraska, and Im not part of the team flatcar-nebraska, when I get tagged using flatcar-maintainers I know its something worth looking at. That change being discussed is probably going to have some bigger impact, and/or is complicated and they could use some additional feedback,review, help etc

    At least in my personal opinion this would actually strengthen our general team knowledge as it would focus everyone's attention on key changes to all of the components of Flatcar as opposed to equally but less, focusing everyone's attention on every change/PR

  • Additional benefit of that, I think, is that whoever gets put in the CODEOWNERS get automatically tagged as a reviewer in a given PR, which is critical for external contributors. By first assigning the individual team, for example flatcar-os we essentially allow that team to triage this PR if it needs additional review. As well as, at least in my personal assumption, it would give more attention to that external contributor and that persons PR, as fewer people get assigned to the PR, each one of them feels a higher dose of responsibility to take a look at the PR and review it.

  • I think there also might exist an issue where we have a repository that has a PR for it, maybe even with a small but useful change, but, because very few people are familiar with that repository, after tagging flatcar-maintainers there might be a case where everyone individually simply skips it due to unfamiliarity or being busy with something else, whereas a specific team having this repo under its umbrella, they would feel more obligated to take a look at it. - This is just a theoretical example, of something that I have seen happen in the past, although not at Flatcar, so Im not sure how probable this issue would be here

EDIT:

Additionally, the sheer number of PR notifications each one of us would be getting per day is simply big, and in the future with the project getting bigger and bigger its going to only grow. I tried using GitHubs CLI (which is super clunky so please take the following statistics with a grain of salt and feel free double check me) to gather some statistics about the PRs in our org. For Flatcar for the last 100 days~ these are the created PRs (no matter in which state they are, that is merged, declined, or still open):

Image

This gives us 572 PRs in the last 100 days. That is ~5.72 PR per day (including the weekends). Admittedly this is slightly biased given the recent appending of Social badges to all our repositories, which create a PR for each repo, so if we subtract 60 from that that would be 512/100=~5.12 PRs per day. However I think its worth taking the 5.72 figure into account as well, given that with time, and with more standardization of our organization, org-wide changes that apply to all repos, or majority of them will become more frequent. What I would like to convey with this, is that now, and especially in the future with (hopefully) the number of contributors rising, it will become more and more overwhelming to be aware of all of those PRs and will simply create a noise issue

John15321 avatar Apr 17 '25 11:04 John15321

Okay, I'm sold. I have certainly considered the last point more than once.

chewi avatar Apr 17 '25 11:04 chewi

Hi @flatcar/flatcar-maintainers @flatcar/nebraska-maintainers @flatcar/flatcar-contributors

I hope you're all doing well. We have an ongoing discussion about setting up a CODEOWNERS file to streamline the review process for pull requests across our repositories. The goal is to ensure that the right teams are automatically notified and can efficiently manage reviews.

Key points discussed so far include:

  • The current manual process of assigning reviewers and its impact.
  • The ideal future situation with an automated CODEOWNERS file.
  • Implementation options, such as defining a unique CODEOWNERS file or maintaining both a CODEOWNERS and a MAINTAINERS.md file.
  • Concerns about the noise created by tagging all members of the flatcar-maintainers team for every PR.
  • Suggestions for creating more specific teams (e.g., flatcar-os, flatcar-nebraska, flatcar-apps, ..., etc.) to manage reviews more effectively.

We would love to hear your thoughts on this matter. Please join the discussion and share your opinions on how we can improve our review process and manage the workload more effectively.

I am also wondering if we should tag/include flatcar contributors in this conversation? I think it might be valuable to hear everyone's options on the matter 😄

(More on CODEOWNERS file: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners)

Thank you!

John15321 avatar Apr 23 '25 15:04 John15321

Every PR has been merged and every repository now has a CODEOWNERS file!

https://github.com/flatcar/Flatcar/issues/1791 - Following the resolution of this issue I think we can close this one as well 🙂 @tormath1

John15321 avatar Aug 06 '25 10:08 John15321