Consider CODEOWNERS to improve maintainer workload #12977

Open aparcar opened 4 years ago

Based on a [previous discussion] about merging the openwrt-routing feed into this repository, I want to suggest a new workflow possibly in favour of everyone.

The main concern of routing maintainer seem to be the overwhelming number of notifications from the packages repository. That's true as multiple hundreds of packages are maintained while the routing repository only contains a few dozens.

To improve that I'd like to consider the usage of a CODEOWNERS file. It contains patch within a repository and links them to individuals or teams. This concept exists both for GitHub and GitLab (in case OpenWrt ever considers moving fully to a self hosted GitLab instance).

The file contains lines like the following, marking me as a maintainer:

/utils/syncthing/ mail@aparcar.org

Whenever someone tries to change a file in my maintained package, I'll get a notification. To make thins more secure, it is possible to lock down the merging of pull requests so that it requires a approval from the code-owner to merge anything.

The CODEOWNERS file is only editable by admins and repository owners.

Using a CODEOWNERS file essentially allows to give more people commit access to packages.git, as they can only modify their maintained packages.

Additionally it is possible to set the number of required review to two, meaning even a codeowner can't upgrade their owned packages and only are able to merge if a second, likely admin reviewer approves the changes.

It's possible to create a CODEOWNERS file based on the PKG_MAINTAINER.

https://docs.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners#codeowners-syntax This part of the doc is unclear, could you confirm that if we have an email in the CODEOWNERS file that is not linked to any GitHub account the functionality continues to work.

The people you choose as code owners must have write permissions for the repository. When the code owner is a team, that team must have write permissions, even if all the individual members of the team already have write permissions directly, through organization membership, or through another team membership.

So they must have a GitHub account and also commit rights. I'll test if a folder is still blocked even if the email address is not linked to an account.

The initial file could be created via the following command:

grep -r MAINTAINER | sed -n 's/^\(.*\)\/Makefile:.*<\(.*\)>.*/\/\1 \2/p' | sort | uniq > CODEOWNERS

Here is the current output: ~~sprunge.us/ra9Iz4~~ http://sprunge.us/zA41kT

You need a / at the start of the path Also does it work when there is multiple maintainers ?

Thanks, I updated the previous comment.

The codeowners file work with multiple users/email addresses, however as the maintainer variable partly contains linebreaks I skipped the hassle to get this figured out. Instead the file could be updated over time as it also support GitHub/GitLab user names instead of mail address.

I prefer to keep email, even if I like GitHub the is the tool of the year, that might be replaced or not, email is the real handle/username ;) Can you just test my first comment in your fork, ie does it work if one of the email is not linked to a user, or linked to a user with no write rights

I just "unwatched" my own fork and added the CODEOWNERS file. Please create a PR affecting Prometheus. It correctly detects the file as "owned" by me:

A quick game of pingpong with @champtar worked as expected.

As another point, we can set that only admins can push to stable branches. If a maintainer with commit rights screws thins up badly on the development branch, we can just drop their commit rights.

On a meta level, my intention here is to make the merge of the routing repository easier.

As another point, we can set that only admins can push to stable branches. If a maintainer with commit rights screws thins up badly on the development branch, we can just drop their commit rights.

Reality check. Note that many/most of the maintainers do not have commit rights. Preventing commits to stable branches from the current committers who merge PRs would pretty much stop backporting activity, I think, as there aren't that many actual owners/admins among those who actively merge PRs. Most of us (including me) have only commit rights.

Not sure either we want to make commit rights too tight, maybe just make PR mandatory so CI run, but let people merge anyway.

The biggest feature from CODEOWNERS is selective notification, everything else is optional for me

Reality check. Note that many/most of the maintainers do not have commit rights

Yes, but I would suggest to give the maintainers from routing.git commit access to packages.git to allow a migration. As we set all those codeowners, they could not interfere with packages they don't own.

Preventing commits to stable branches from the current committers who merge PRs would pretty much stop backporting activity, I think, as there aren't that many actual owners/admins among those who actively merge PRs. Most of us (including me) have only commit rights.

I think the "long term trusted" committers can use their own group like "meta-maintainer which is then added to codeowners like "* @meta-maintainer". So they get a notification whenever someone tries to add a new file. This essentially prevents anyone with commit rights to do anything bad except to their own packages.

For backporting the CODEOWNERS file could be set to "* @meta-maintainers" to allow only meta maintainers to handle things.

Not sure either we want to make commit rights too tight, maybe just make PR mandatory so CI run, but let people merge anyway.

Fine with me

The biggest feature from CODEOWNERS is selective notification, everything else is optional for me

And that sadly only works if the person has commit rights.

Code owners are helpful, but not if there is a growing lack of package maintainers (talking about https://github.com/openwrt/packages/issues/6584) and mostly about this issue https://github.com/openwrt/packages/issues/7125 (see those mentions as well). In the past, the most active merger was @hnyman kudos for him and these days is @neheb. Sometimes there are others including me. It was tough to receive commit access. I'd say that this does not get to the bottom of this issue as it is.

Are you want to give commit access to everyone? No, you don't want to do that. Pull requests should be reviewed, compile, and run tested. Most of it is just compile tested by CI or locally, which is not good as you need to know if it works on the router as well. Who is reviewing those pull requests? @neheb. For Python packages there are two maintainers @jefferyto and @commodo , praise them as well!

However, there are some pull requests stalled (e.g https://github.com/openwrt/packages/pull/12896) and sometimes people lost interest in their packages. Looking at https://github.com/openwrt/packages/graphs/contributors these days are not many active people and you (as part of OpenWrt even though without commit access) you should motivate your community. ;-)

Oh and I am not talking about old versions of packages (unmaintained, etc.). All of this is connected to improve maintainer workload. How many people is using internal group for https://github.com/orgs/openwrt/teams/package-maintainers/discussions/ ? There was discussion for libjpeg-turbo and just three people (@neheb, @diizzyy and me) said something about that.

Currently, this just adds overhead.

I'm happy to improve the situation for maintainers as I highly honour their work. I'll create some PRs for a CI building packages in multiple architectures and we could also test compiled packages in a x86 docker containers. I don't thing we can implement a full runtime testing environment but trivial tests like prometheus -v | grep Version surely works. Maybe we can even integrate some qemu to test more exotic ~~packages~~ architectures. But this is a bit out of this issues scope.

My motivation is to merge openwrt-routing into packages, something the maintainers disliked because there is to much noise on packages.git. Adding their packages here and to CODEOWNERS allows them to keep track without being flooded with notifications.

Granting more people commit access later on because they could only break their own packages is another thing to discuss in some future.

The overhead is marginal, whenever a new package is added a single line is added to CODEOWNERS.

@aparcar that codeowners file from PR https://github.com/openwrt/packages/pull/12982 is not handling co-maintainers well; a few files [in the python-sphere], are maintained by both me & @jefferyto co-maintainers are listed with a comma-separation in the Makefile

I chose the easiest way to create the file, we could also do some make magic but then again we can just update the codeowners whenever a package is updated

I agree with @BKPepe and @hnyman

Regarding workload we still don't have any common guidelines which makes work tedius, sends mixed signals and maintaining tedius as a lot gets committed with additional patches without any upstream support etc. That said, I don't think it's ever going to change unless we create a new repo and really enforce such practices. What I also think might be a barrier is the amount of platforms we're trying to more or less enforce maintainers to maintain, there's no tier system and I can fully unstand if you don't want to support a platform you don't own and/or care about.

In short, this is a no go solution given the current setup which needs to be resolved first and discussed if that's the way we want to proceed.

Some thoughts I have after reading the comments so far:

First, thanks Paul for trying to tackle this task (make merging routing into this repo more possible).
CODEOWNERS requires commit rights, and it feels like we don't want to give all maintainers commit rights. I could be wrong but it seems like this is the biggest issue stopping the use of code owners.
Lack of package maintainers is a real issue, but it is too big to address in this task (make merging routing into this repo more possible).
In my personal experience, notifying maintainers of issues/PRs does improve maintainer engagement.

I have a different suggestion: set up a bot that watches for new PRs and mentions/assigns them to the package maintainer(s).

Ideally it can look at the files changed, find the maintainers in the package Makefiles and translate their names/email addresses into GitHub usernames, but if this is too complex I think we can settle for a simpler solution like looking up a centralized, CODEOWNERS-type file.

(This is probably specific to my use-case, but a nice-to-have feature would be if individual contributors can add keywords they are interested in and the bot can mention them when it see those keywords. I subscribe to all notifications just so I see mentions of Python or the other packages I maintain, even if I am not mentioned specifically.)

Yes, there are downsides:

This is more complicated
This may be GitHub-specific

But:

This let's us have notifications without giving everyone commit rights
This is doable
This opens the door for other triage tasks to be done by the bot / other bots

If there are better, more helpful and realistic suggestions, let's hear them.

(Side note: Sometimes I feel like this repo is missing effective leadership / an effective decision-making process. It's natural that we all have our opinions but we're not arriving at concrete goals or actions. Perhaps there are more productive conversations in the package-maintainers discussion area, but that is only open to team members and so I am not privy to these discussions.)

A custom bot would be possible but adds additional complexity I'd like to avoid. Maybe there is a misunderstanding on how the codeowner file works: It neither gives anyone automatically commit access nor does it not work if people within the file don't have commit access, those commit-less people just won't get any notifications.

For my focus on merging routing.git, we'd just give commit access to those few maintainers of routing.git. Based on how the codeowner file works, they won't be able to change anything they don't own. If they would want to break they packages without testing, they could just go ahead and do it at this very moment over at routing.git. I don't see a decrease in in quality/security here.

For those special groups concerning Python, Go, Rust, Perl or even routing, admin etc we can just add GitHub groups like @openwrt/python-maintainers and extend the codeowner file like this:

/lang/python/* @openwrt/python-maintainers

It neither gives anyone automatically commit access nor does it not work if people within the file don't have commit access, those commit-less people just won't get any notifications.

I am aware of this.

For my focus on merging routing.git, we'd just give commit access to those few maintainers of routing.git.

By "maintainers" are you referring to users with commit access to routing.git or routing package maintainers (who do not have commit access to routing.git)?

Based on how the codeowner file works, they won't be able to change anything they don't own.

The About code owners page talks about pull requests only. It doesn't prevent users listed in CODEOWNERS from pushing directly to the repo.

If you intend to combine code owners with protected branches / branch restrictions, perhaps you can describe how those settings can be configured to limit pushed changes to those paths listed in CODEOWNERS.

For those special groups concerning Python, Go, Rust, Perl or even routing, admin etc we can just add GitHub groups like @openwrt/python-maintainers and extend the codeowner file like this:

I am against "special groups" that will receive commit access - I think all package maintainers should be treated equally. I know some package maintainers have commit access but that is because they are also core OpenWrt project members.

By "maintainers" are you referring to users with commit access to routing.git or routing package maintainers (who do not have commit access to routing.git)?

I thought the maintainers and committers are pretty much the same. I might remember that wrong.

If you intend to combine code owners with protected branches / branch restrictions, perhaps you can describe how those settings can be configured to limit pushed changes to those paths listed in CODEOWNERS.

I do, from my understanding one could add a * @meta-maintainers as first line and thereby own anything which is not defined later lines to a (small) group of people to decide which new packages should be accepted.

I am against "special groups" that will receive commit access - I think all package maintainers should be treated equally. I know some package maintainers have commit access but that is because they are also core OpenWrt project members.

I don't share that opinion as that results in the awkward situation OpenWrt has right now: Having new members with a voice in votes must have commit access to git.openwrt.org. This rule is even object to changed based on a recent online meeting.

Having this binary approach of no or full access makes it much harder to accept new committers.

I thought the maintainers and committers are pretty much the same. I might remember that wrong.

There are perhaps maybe 30 committers, but over hundred maintainers. I haven't really checked/calculated that. (It might be several hundreds, as there are lots of "1-2 packages" maintainers.)

Sorry if that was unclear, I was talking about the routing repository, where I count about 30 packages.

I do, from my understanding one could add a * @meta-maintainers as first line and thereby own anything which is not defined later lines to a (small) group of people to decide which new packages should be accepted.

How does this stop someone from pushing directly to the repo, bypassing pull requests? Please elaborate on the protected branch settings required.

I don't share that opinion as that results in the awkward situation OpenWrt has right now: Having new members with a voice in votes must have commit access to git.openwrt.org. This rule is even object to changed based on a recent online meeting.

I'm not referring to who has decision making power (e.g. setting project rules and policies) in the repo (there does not seem to be an organized decision making process here, at least in the main repo there are rules around calling for votes). I'm only talking about commit access.

Commit access is a privilege; it infers a level of trust and responsibility. I'm not against only core project members having it, or if some package maintainers have it and some do not, but I would like to see some clear (ideally merit-based) rules around who gets to have commit access and who doesn't.

How does this stop someone from pushing directly to the repo, bypassing pull requests? Please elaborate on the protected branch settings required.

Setting the master branch to protected:

Require pull request reviews before merging When enabled, all commits must be made to a non-protected branch and submitted via a pull request with the required number of approving reviews and no changes requested before it can be merged into a branch that matches this rule.

Commit access is a privilege; it infers a level of trust and responsibility. I'm not against only core project members having it, or if some package maintainers have it and some do not, but I would like to see some clear (ideally merit-based) rules around who gets to have commit access and who doesn't.

Not sure what's the best way to implement this as it's a community management decision. I'd put the most active maintainers in the @meta-maintainers group allowing them to decide on new packages and e.g. the README. People working on the Makefile-magic around Golang or Python should get members of @python-maintainers etc. People maintaining a package and doing n qualified PRs to keep it updated should get their initial commit access. People adding a package for the first time should team up with a co-maintainer which already has commit access to guide the newbie until they get commit access themself.

This is just a spontaneous idea. Maybe the whole thing is irrelevant, routing.git should stay where it is and only a runtime test CI can actually lower the maintainer workload.

@openwrt/package-maintainers Lately I got requests to grant people commit access so they can maintain their own packages. I'm hesitating as I can't check every commit of those people and even if the "track record" is fine, this doesn't seem feasible as the project continues to grow.

I'd like to bump again the idea of admins that have control to add committers. Whenever a committer is added, a new line to CODEOWNERS is added, allowing them only to modify and approve the package they maintain.

openwrt / packages

Consider CODEOWNERS to improve maintainer workload #12977