That's all good heh. Yeah, this is going to be rolled out slowly. Once this lands, there'll be communication to the consumers of meta.maintainers to look into using it. I believe we already have that done via hasNoMaintainers covering teams (visible in the diff of this PR).

Do note that due to GitHub limitations, a good chunk of the codeowners unfortunately were not pinged, and as such really can't be blamed for missing it.

That is more of with #400458 rather than this PR. I did mention this PR several times in the NixOS Stdenv Matrix chat, though nobody objected to it. I did see that over 14 people saw it on that chat several days before this merged so it's not like this was going to happen without prior knowledge.

It applies to @mweinelt specifically, though, as ci/ is owned by the security team, and this PR touched that directory.

Huh, though the bot could've pinged 5 more people right? There's a cap of 15 reviewers, at least through the web UI, and there was 10 people in total pinged. At least philiptaron was pinged so that brings down the number of people who needed to be pinged. Not sure why touching ci/eval didn't ping more people though.

@winterqt That is a good point, and strange that didn't happen. I'd have thought codeowner pings would have handled it, but alas. ci is listed in the owners file, so I was under the assumption that the right people had been pinged. ☹️

That aside, I am up for doing whatever we think is best as a project. This change was meant to reduce boilerplate and ultimately ping @NixOS/* down the line. If we didn't pull in the right people, well, we should just make sure everyone is aware next time.

I did notice a PR that also should've pinged codeowners, #400408, but it didn't. Not sure what happened there, but it looks like maybe the ping bot or GitHub itself is flaky with pings?

I think I may have figured it out: see OWNERS and the docs. It's subtle: ci was listed without a trailing slash, so it doesn't apply recursively. @NixOS/stdenv was pinged since /pkgs/stdenv/generic/check-meta.nix was touched, that's why @philiptaron got pinged, but changing something in a subdirectory of ci didn't ping the people on the list for ci.

In this example, @doctocat owns any files in the build/logs directory at the root of the repository and any of its subdirectories.

Note the difference between this and:

In this example, @octocat owns any file in a /logs directory such as /build/logs, /scripts/logs, and /deeply/nested/logs.

So it may be a matter of adding a trailing / or a /**.

So it may be a matter of adding a trailing / or a /**.

No, that's unrelated. The codeowners file works as expected.

The reason is, because changes to this PR were added incrementally. In the beginning, not so many files were touched, yet, so the codeowners ping worked. After a while, there were too many codeowners to ping, thus the last run of CodeOwners / Request has this output:

Too many reviewers (hsjobeki ericson2314 philiptaron emilazy getpsyched risicle lesuisse reckenrode fricklerhandwerk mweinelt wolfgangwalther artturin infinisil), skipping review requests

The limit is set to 10 btw. - this limit is not a hard limit by GitHub, but one imposed by us. The idea is that once you touch as many files, it doesn't make sense to ping a ton of code owners, but the authors should explicitly select their reviewers instead.

It's unfortunate that the "request code owner reviews" job is still showing "green / succeed" in the checklist in this case. We had it failing before, but it was changed to succeed in #375354. Neither is perfect. Ideally, we'd have a "skipped" appearance, but that's not easily possible - I tried, but failed, so far.

If it were up to me these changes would be reverted in full pending further work and sufficient discussion and review.

Supportive of reverting this before branch-off.

What's the state here, will this be kept or reverted?

For completeness-sake, I now pinged the code owners of /ci manually. I wasn't aware of this PR until I diffed stable and unstable on the ci/ folder and came up surprised for a difference.

Oh, what lovely timing. lol.

What's the state here, will this be kept or reverted?

See #402991, the change is effectively unnoticeable now, as meta.maintainers now contains meta.teams entries.

It seems this broke more, see #412184.

Seems like that check was already broken, and this simply brought it to light by making the check more explicitly broken.

Seems like that check was already broken, and this simply brought it to light by making the check more explicitly broken.

The check wasn't super broken. The check only listed things as unmaintained if explicit maintainers = [] was set, with the intention of not flagging trivial builders or fixed output derivations. Arguably packages with no meta.maintainers set should have been reported as unmaintained - but objectively the correct thing to do here would have been to try and restore as close to old behavior as possible, which would have been:

  (attrs ? meta.maintainers || attrs ? meta.teams) && (attrs.meta.maintainers or [] == []) && (attrs.meta.teams or [] == [])