chore: adjust code owner files for app maintainers

[edward-ly]

Summary

This change ensures that app maintainers are automatically requested for review whenever a pull request that changes code in their app is created.

cc @DaphneMuller

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[skjnldsv]

I'm honestly not sure.
Shared expertise doesn't mean we need everyone's approval for a specific app.

Expertise is also different from knowledge.
Because I'm the maintainer/expert of an app doesn't mean I have availability for a PR that covers it nor does it mean only two people are able to review it.

This will more likely creates more noise than else imho.
I would prefer to keep using the two dedicated backend/frontend teams we have for Files which randomise the reviewers and ensure a bit more distribution

[susnux]

Because I'm the maintainer/expert of an app doesn't mean I have availability for a PR that covers it nor does it mean only two people are able to review it.

For apps this makes sense (meaning apps not in server repo) as there the app owner should care about reviews and maintenance.
But for server repository it really makes sense to keep the existing randomized assignment using the two teams server-backend and server-frontend.

Theoretically something like this should work: https://github.com/nextcloud/server/pull/50991/files

[provokateurin]

IMO this is not a good idea because everyone will also receive a lot of spam.

[DaphneMuller]

hi @skjnldsv and @provokateurin , is your concern only about apps that have more than let's say 3 code owners, or is your concern about this PR in general?
We are fine with implementing @susnux 's solution for the apps that have more than 3 code owners, which would be files and perhaps the twofactor_backupcodes app .

I understand the concern about increased notifications and potential spam. At the same time, the current situation—where PRs don’t get assigned or don't get assigned to the right people — leads to delays and a lack of clear responsibility, which negatively impacts our responsiveness to the community. The code owners file was also updated about a year ago with the goal of auto-assigning the owners to pull requests that affect that code, but it turned out this is actually not what happened back then. What we try to achieve is ensuring that all community pull requests get assigned quickly.

[provokateurin]

I think the idea from @susnux is much better, although I'm not sure it will work 100%, because it assigns random engineers and there are some who are more responsive than others.

I just had the idea that we could instead have a board or search link where all community PRs are shown. Then the team leads (I guess mostly @sorbaugh) are tasked to assign all new community PRs to an engineer (note: assignment is not reviewer). Then the assigned engineer is responsible for requesting the right reviews and moving the PR forward.
Maybe this is too much manual work, but I think this could work. Shall we have a call with all interested people to check what system makes most sense? I'm not sure a discussion on this PR is the best way to share ideas.

[DaphneMuller]

@provokateurin I will talk about it with Stephan 1:1, but can we accept this pr for all the other apps that do not have many code owners?

[provokateurin]

I'm not sure. The spam will happen either way, regardless how many people are maintainer of an app.
I would rather have the PR from @susnux merged as a quick fix and then change the process later if necessary.

[susnux]

I understand the problem @DaphneMuller and I agree with it, but this changes (using the app owner as code owner) is only feasible for external apps, for server we have that both groups so backend code only gets assigned to backend engineers and frontend only to frontend engineers and its randomizes the engineer so the workload is balanced.

The problem is external apps probably get like 1-5 PRs a month, code owners of files or file_sharing would get spammed with much more, also for parts they are not experts of (e.g. frontend code as a backend engineer).

I think both ways solve the problem but as @skjnldsv assigning the server-backend / server-frontend teams will at least keep the work reasonable.

there are some who are more responsive than others

Maybe that's part of the problem and time for reviewing community PRs should be scheduled by the team lead - but in the end its makes no difference if I get assigned by app owner ship or by github team, in both cases if I do not respond then being assigned makes no difference.

[skjnldsv]

I understand the concern about increased notifications and potential spam. At the same time, the current situation—where PRs don’t get assigned or don't get assigned to the right people — leads to delays and a lack of clear responsibility, which negatively impacts our responsiveness to the community.

Please discuss this with the Files team. This looks like something to be addressed before such PR gets applied here.

[DaphneMuller]

@skjnldsv well you are part of the files team right? ;)

I've asked if there is objection to this in it's entirety, or only for doing this on apps with many code owners.

I understood from Kate that there will still be "spam" but I don't think I get the perspective here yet, because how can an e-mail that you have to review a PR in a piece of code where you are the only or one of the little number of people responsible, be spam?

Also, this PR is to fix a thing that wasn't done properly a year ago or so. Because we did some work on this a year ago to achieve exactly this

[skjnldsv]

@skjnldsv well you are part of the files team right? ;)

That's what I mean, I would prefer us to be consulted before chatting on this publicly over a PR from someone else's team. I think there are more appreciated ways to discuss options to solve the important issue you've raised :)

I understood from Kate that there will still be "spam" but I don't think I get the perspective here yet, because how can an e-mail that you have to review a PR in a piece of code where you are the only or one of the little number of people responsible, be spam?

One more reason to discuss this in person 👩. Every team have their own ways of addressing assignments and opened PRs. And it fluctuate between individuals too.
I get 400+ notifications per week. The last 12 months, I have reviewed over 2000 PRs.
It's insanely difficult to grasp the priorities when you open Github's notifications page. If I start getting automatically request for every PR that touches Files or the many other repos I have ownership on, everything will be noise (not that it isn't complicated already).

[DaphneMuller]

@skjnldsv Just for the record, this was aligned about a year ago when this was first implemented. With the team leads and Andy and all. This PR happened because we found out that that implementation wasn't working as designed. I see that now the opinions on this have quite drastically changed, which I did not expect at all, so when I have the headspace for it I will restart those discussions. I didn't expect this type of feedback which is why this is going on in public. Of course if I would have known about it I would have started the discussions in a different channel. But now here we are.

Also there are teams like the groupware team who are assuming that this system is working.

In the meantime, in an attempt to get closer to meeting your and your files team members wishes, I already offered to see if we can scope this down to only the parts of the code that have limited code owners, but I understand that is also not supported by the files team because it would generate notifications which is undesirable due to already existing quantity of notifications. While I'm not sure how that would be worked around, because this is work that has to be done by the responsible person, and I'm not sure there is another way than notifications to deal with this.

[skjnldsv]

@skjnldsv Just for the record, this was aligned about a year ago when this was first implemented

Maybe I'm just missing the original context of what "this" refers to 🤔
Sorry for that then, a year after, seeing this PR appear just raised some concerns for me 🤷

Really not trying to block for the sake of blocking :)
Will be happy to help find a solution, even if it turns out we end up merging this PR in the end 💪