Add codeowners pre-commit check

[gmottajr]

Fixes

Related to #3554 by @dhruvkb

Description

Integrating codeowners-validator into openverse lint hooks (managed by pre-commit in the .pre-commit-config.yaml file). In order they can use the Docker approach codeowners-validator.

[gmottajr]

hi @dhruvkb, could please give me some feedback here I was not able to add any label to this pr? Is there anything that you might me waiting from my side?

[dhruvkb]

@gmottajr the labels are automatically added but since you did not follow the PR template, the labeller could not work. I've manually applied them now, so that should be resolved 👍. We're a little short-staffed because of the holidays so reviews on the PR might be a little slow, but if you want, you are welcome to pick another issue to work on while we review this.

[dhruvkb]

I read through the requirements for the mszostok/codeowners-validator repo and it seems like the image needs GitHub credentials to work properly. Since that is not something every contributor will have access to, the better approach will be to use the package as the official GitHub Action workflow: https://github.com/mszostok/codeowners-validator/blob/main/docs/gh-action.md

[gmottajr]

Hi @dhruvkb, I would say I did not fully get what you said about the mszostok/codeowners-validator needing GitHub credentials to work properly. You can check both files, the .pre-commit-config.yaml and also codeowners-validator.sh, there is no reference for the use of credentials. And It seems had worked for me when I performed the git commit. What I'm trying to say is, do you see any chance, when I started com commit this change, the validator would have not worked properly? Would I be able to commit and push my changes if it had not?
I mean, because I did not enter any credential when I used that docker image. Initially, as my first try I used one docker image on my own. I mean, I created one new image in my docker container for that. And in that case I had to enter my credentials, in deed. The command in the file codeowners-validator.sh was like this: docker run --rm -v "$(pwd):/repo" -e REPOSITORY_PATH=/repo -e OWNERCHECKER_REPOSITORY=/repo gmottajr/codeowners-validator:latest. But I changed this approach and used a Docker image that is available publicly and does not tie to a specific user account. Unless you can't use this approach you should not need to use any credentials, I guess.
Does it make sense what I'm saying? Please correct me if you think it does not. Anyway, I'm going to give a try and use the package you mentioned (as the official GitHub Action workflow) if I find it there. Let you know asap.

[gmottajr]

I went back, took a look at the codeowner validator link you just gave me and something is not really making sense for me 🤔.
Even though that web page does not give us a detailed information about the context it is talking about, seems to me that it is actually addressing the scenario for Git Actions.
But what you had asked me was this: We need to integrate it into our lint hooks (managed by pre-commit in the [.pre-commit-config.yaml file].
You mentioned: pre-commit supports Docker hooks so we can use the [Docker approach] to add this our repo.
And that was only what I worked on, and what you have for this PR 📄.
🤔 Am I getting it correct?
I apologize for any prolixity, but I need to understand better, making sure I fully comprehend, what you are asking for, my fried. please bear with me a little longer and let me double check my understandings:
Different Approaches: Seems that the original request and the feedback represent two different approaches to achieving similar goals (ensuring CODEOWNERS file correctness).
1 - ### The pre-commit approach integrates the check into each contributor's workflow, running locally before a commit is made.
2 - ### The GitHub Action ⚙️ approach centralizes the check, running it as part of the CI/CD pipeline on GitHub, whenever code is pushed or a pull request is made.

Please @dhruvkb, I need you clarifying for me, anyway, no matter what I'm ready to adjust, and depending on the clarification you are going to give me, I may either continue with your current Docker-based pre-commit setup or switch to implementing the GitHub Action, as per your last comment. Do you know what I mean?
Your patience is greatly appreciated as I seek to clarify and confirm my understanding of the task at hand. 😊 Could you, please, review the key points I just wrote to ensure we're aligned? 📝
Thank you, again, for bearing with me.🙏
Cheers! 🎉"

[dhruvkb]

Sorry for taking so long to respond, I will need to check the codeowners-validator code annd docs myself to make sure I understand how it works before reviewing this PR.

@gmottajr I don't want to block you from contributing so feel free to pick up another issue and work on it in the meantime. Thanks for contributing this PR, we will try to review it asap.

[sarayourfriend]

I've left a comment explaining the required changes. We can bypass the concern about the access token entirely, I think. @dhruvkb, can you give a 👍 or 👎 on whether you are okay with leaving out the "owners" check (which is the one that causes the access token issue)?

Please make the changes I've requested to move the hook into the existing repo: local group, and to change it to use the pre-built image, allowing us to remove the new dockerfile and the shell script.

Thanks for the contribution!

[sarayourfriend]

We should add this to the existing group of repo: local hooks.

Additionally, we don't need to use a shell script and a custom docker file to build from. We can just use the image from the project itself: https://github.com/mszostok/codeowners-validator/pkgs/container/codeowners-validator

For that we can use the docker_image pre-commit language:

- id: codeowners-validator
  name: CODEOWNERS validator
  language: docker_image
  pass_filenames: false
  files: CODEOWNERS$
  entry: >
    -e REPOSITORY_PATH="."
    -e CHECKS="files,duppatterns,syntax"
    ghcr.io/mszostok/codeowners-validator:v0.7.4

We can leave out the "owners" check. I don't think it's much of a concern, and the original issue is only trying to fix the issue where files move around and are no longer included in the codeowners. If we want to include it, that's fine, and it should work because the GITHUB_ACCESS_TOKEN is not required, and all our codeowner teams are public. It would only cause an issue if someone hit the rate limit locally. In CI we can use it from the environment variable like we do other things... but I really don't think the owners check is a real concern for us and we can ignore the access token issue altogether.

My only outstanding question is whether we should enable the two experimental checks, notowned, which ensures there are no "unowned" files, and "avoid-shadowing", which helps make the codeowners file easier to reason about. I don't think they're necessary for this PR though. Getting the basic checks in would be better than delaying this to decide about the experimental ones.

[openverse-bot]

Based on the low urgency of this PR, the following reviewers are being gently reminded to review this PR:

@krysal
@AetherUnbound
@dhruvkb
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend¹ days, this PR was ready for review 10 day(s) ago. PRs labelled with low urgency are expected to be reviewed within 5 weekday(s)².

@gmottajr, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings.

Footnotes

1. Specifically, Saturday and Sunday. ↩

2. For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the operation that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range. ↩

[krysal]

@gmottajr, I'm drafting this until we get confirmation that you read Sara's comments.

[gmottajr]

Hi @sarayourfriend, I do appreciate you assistance over this matter.
Thank you very much.
I completed the changes as per your request and uploaded them.
Best regards,
Gerson Jr.

[sarayourfriend]

Great, thanks @gmottajr. It looks like there are some fixes we can apply to the CODEOWNERS file 🎉

Here's the output I get locally, which matches that of the CI job:

 ==> Executing Duplicated Pattern Checker (25.177µs)
    Check OK
==> Executing Valid Syntax Checker (243.614µs)
    Check OK
==> Executing File Exist Checker (521.044µs)
    [err] line 6: "nginx/" does not match any files in repository
    [err] line 7: "postgres/" does not match any files in repository
    [err] line 9: ".flake8" does not match any files in repository
    [err] line 10: ".isort.cfg" does not match any files in repository
    [err] line 27: "dead_links/" does not match any files in repository

3 check(s) executed, 1 failure(s)

To fix these, I think the best thing to do would be to remove the explicit files assigned to @WordPress/openverse-maintainers and use a catch-all * @WordPress/openverse-maintainers at the end. That removes some of the entries above. Finally, we can remove the nginx/ and postgres/ patterns.

I tried using the notowned check locally, but you can't use it in the pre-commit stage because it won't work with a dirty git environment (files must be committed). I think we should consider adding a second hook for it with stages: [manual] that we then run in CI. Of course, if we do that, we should remove the wildcard at the end and instead add all files manually, otherwise it is a redundant check.

@dhruvkb What do you think? Should we just use a wildcard at the end to catch everything that isn't explicitly assigned to one of the three sub-groups or should we require explicit assignments of all files to one of the groups and add a second notowned hook? To my mind, we can do this as a follow-up issue and the wildcard is sufficient for now.

So to wrap this up @gmottajr please do the following:

1. Address the issues the validator is raising by removing all explicit assignments to @WordPress/openverse-maintainers in favour of a single wildcard at the end of the file and by removing the nginx and postgres entries from the openverse-api section.
2. Remove the unnecessary __BlaDockerfile file
3. Move the new hook into the existing repo: local block.

[gmottajr]

I have completed and pushed all change requests from @sarayourfriend. This PR is ready for another round of review.
Please this is with you guys now, let me know your finds.
Once again thank you very much @sarayourfriend for your assistance here, please let me know whether or not it is good to go.
Best regards,
Gerson Jr.

[sarayourfriend]

This looks good to me now, thanks @gmottajr. I left a comment with one change and immediately committed it via the GitHub UI, it's just a small change to make sure this runs in the correct cases (I had this wrong in my example code I shared earlier).

Thanks very much for your effort on this contribution 🙏

Changes addressed

[gmottajr]

Just double checking guys, is there anything you might be expecting from my side to be done for this change? @sarayourfriend , @dhruvkb ? May I have a feedback about what is going to come next? I mean, is it good to be merged?

[sarayourfriend]

It looks great, we're just waiting for a second maintainer to review the change. A lot of the core maintainers are on holiday or sick, so there's been a delay in getting things reviewed.

@krysal Do you think you'd be able to review this PR soon?

[krysal]

We want to avoid using this * all pattern because together with the previous lines it will make require reviews for more people than intended. Please leave the specific lines for the rest of files and folders associated with the @WordPress/openverse-maintainers group.

[sarayourfriend]

If we put the all at the top, would the more specific patterns not over-ride them? Or does it just combine them?

If that is the case, then it would indeed be good to add the notowned check so we can check that all files are owned, but it adds a lot of complexity to this check because of the reasons I mentioned in my other comments.

[krysal]

It'd be nice to include the checks for notowned and avoid-shadowing once they pass the experimental phase. Also, the description of the latter suggest than the * pattern may work as we want if it's specified before the more specific line but I haven't confirmed it 🤔

[sarayourfriend]

The notowned check requires the branch to not have any uncommited changes, and so cannot run in our normal pre-commit stage. It's flaky to run it in pre-push as well because it will still complain if there are uncommitted changes (like you have something you would have otherwise stashed).

It's a good check but not currently easily usable. We'd need to add a separate step for it altogether, probably just in CI. I couldn't find an easy way to do that in pre-commit other than adding it with the manual stage and then adding it to the CI lint job.

[krysal]

Thanks for enumerating the quirks of the notowned check. I assumed there would be some complications, but nothing so disrupting 🤯 Anyway, my comment here was just a desire (though at first, I thought it was the whole reason to add it to pre-commit); I wasn't requesting changes for this to be clear.

[gmottajr]

Hello guys, I hope you all are having a marvelous day!
I have updated according to what @krysal mentioned, please have a look and let me know.
I hope I got right what she wants.
Thank you!

[krysal]

I added some folders and files and sorted the entries by file type and name. This looks great, thank you @gmottajr!

[gmottajr]

Hey guys (@dhruvkb , @krysal , @sarayourfriend),
Awesome to hear the update was well-received – glad to be a part of it!
Working on integrating the Codeowners validator into our project was an insightful experience (I have never heard about it before, actually).
I would say that tool can be a game-changer as it ensures our CODEOWNERS file is always accurate and up to date.
Seems to me an important addition, as it helps prevent oversight in code ownership, making sure that the right people are always reviewing the right parts of the code.
Very nice!
Other than that, It would be awesome to be continuing collaborating with your project. If there's anything coming up that I can lend a hand with, just give me a shout. Looking forward to what's next!