Remove CODEOWNERS.

[veluca93]

As we discussed, the consensus between active devs is that CODEOWNERS is not the best way for individual devs to be notified about specific PRs.

[wil93]

We discussed it but I don't think that we reached an agreement on it.

I see benefits in keeping this, it helps me notice active PRs that touch the schema so I can ping the OP and ask to write an updater, or write one myself (like I just did for this active PR: #1416).

I also don't see any harm in keeping this, so I oppose this change.

One great suggestion by @prandla would be to add a github action that diffs the schema and warns OP if it doesn't match, asking to add an updater if so. This would be way better than me checking manually, and I would then not need this mechanism anymore (at least for this specific use case).

(I'm happy to hear an alternative if you propose one)

[wil93]

I took a stab at implementing the Github action diff thing 😄 PTAL #1422

[gollux]

We discussed it but I don't think that we reached an agreement on it.

Procedurally speaking, this is a simple case. I understand that we agreed that every change has to get a positive review by somebody different from the author. When you introduced the CODEOWNERS file, you bypassed the review process, but I don't see why it should not apply. Has anybody reviewed your change?

[veluca93]

@wil93 friendly ping on @gollux's question

[wil93]

@gollux When I introduced the "requires 1 approval" rule, I did it to incentivize reviews (seeing the Github UI "waiting" for a review makes us more willing to give one) and it definitely worked for that.

One could argue that for changes that are inconsequential / very small / very safe (highly unlikely to make the CI tests fail) we could skip this, but I agree that it's better as a general rule to always get a review. I am happy to make this rule more strict from now on.

Consider also that some settings can only be changed via the Github UI, as they are not stored in a git-tracked file (like the very setting of "requires 1 approval", or giving access to new people). As much as it would be cool to have every setting under version control, it's not how Github works so those changes will anyway not be able to be reviewed in the usual sense. We can still enforce a rule that we need to discuss these changes (informal "review") in the dev chat before doing them. I'm in favor of that.

As for the CODEOWNERS change in particular, I believe it falls under the same category of "Github settings" that could very well be changeable via UI-only, but it just happens that it's provided via a git-tracked setting instead. As this setting does not influence anyone else's experience in contributing to CMS other than my own (i.e. it helps me notice certain PRs, but it doesn't block or even slow down anyone else's workflow) I would consider this a very minor setting change that I would not even require approval for, in case another contributor wanted to add themselves to that file.

That said, I'm more than fine with following a more "general rule" from now on and always request approvals even for small changes such as this, but I don't see this as a strong enough reason to undo this now. There were other similar "meta" settings, one random example that comes to mind is the .vscode/ folder, that I introduced without requesting approval: I don't think we should remove that folder now just because it was not reviewed when it was added. And come to think of it, for changes such as that one, you'd need to get a review from someone who also uses VS Code, as other people would not care or even be able to test the changes... Actually, this is another reason for using CODEOWNERS 😄

[veluca93]

As this setting does not influence anyone else's experience in contributing to CMS other than my own

I believe we said in the chat that we believe this is not true, as it creates an impression on contributors that you are the owner of the database-related parts of the codebase (that's literally what this file is for), and I don't believe there is agreement with the other people with write access that this is the case.

Consequently, I also disagree on this being a minor change.

I don't see this as a strong enough reason to undo this now. There were other similar "meta" settings, one random example that comes to mind is the .vscode/ folder, that I introduced without requesting approval: I don't think we should remove that folder now just because it was not reviewed when it was added.

The key difference is that nobody else with write access disagrees with the changes to the .vscode folder, while I believe from the discussion the agreement we reached with everyone else was that this is not a good change.

[wil93]

it creates an impression on contributors that you are the owner of the database-related parts of the codebase (that's literally what this file is for)

The name "CODEOWNERS" is probably misleading, yes, but when I added myself to that file I clarified both in the commit message and in a # comment in the file itself what the purpose of the addition really was.

It's relevant to mention that I found out about this by searching "automatically assign reviews github" and this methodology was the recommended one among multiple sources, including official ones: https://github.com/orgs/community/discussions/47988

Would you be fine if we added a disclaimer on top of the file, or in the repo's README, or both, better explaining what we use the CODEOWNERS file for?

[stefano-maggiolo]

William, I think the consensus for this PR is clear and it doesn't need long discussions.

A second pair of eyes looking at any commit is a standard practice. It is also reasonable that the reactions of not doing it can change depending on the nature of the change.

Let's move on from this topic towards something more productive, and try to use code reviews for all changes from now on. If anything, I don't think anyone will forget about updaters from now on!

[wil93]

Fair enough.

But I can't help but voice my concern over exclusionary attitude I'm seeing in this small community. My intention, as I made clear now in all possible languages, was to do something good for CMS by focusing my limited attention to making sure the schema changes we receive have proper updates.

I was notified of #1419 thanks to this system, and I spotted a mistake there because I cared enough to take the time to review that change and manually test it. I could just as easily decide not to care, but I didn't.

When a contributor offers their time and effort for a project's success I think it's good to appreciate it, not try to misinterpret the contributor's intentions (even after they explained themselves).