Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add codeowners pre-commit check #3570

Merged
merged 16 commits into from
Jan 16, 2024
Merged

Add codeowners pre-commit check #3570

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
4ba2b67
updating project's .pre-commit-config.yaml file to include a hook tha…
gmottajr Dec 21, 2023
7331fd6
cleaning commit
gmottajr Dec 21, 2023
b27a94d
Merge remote-tracking branch 'upstream/main' into fix/posix-path-sepa…
gmottajr Dec 21, 2023
54c6e2e
Changes requested during code review: We should add this to the exist…
gmottajr Jan 5, 2024
32ce97e
Changes requested during code review: we don't need to use a shell s…
gmottajr Jan 5, 2024
66335a6
Fixing CODEOWNERS validator hook failing. The error message indicated…
gmottajr Jan 10, 2024
6b63fdd
Merge branch 'main' into fix/posix-path-separators
gmottajr Jan 10, 2024
6033feb
Always run CODEOWNERS check if any files changed
sarayourfriend Jan 10, 2024
2567f79
Merge branch 'WordPress:main' into fix/posix-path-separators
gmottajr Jan 11, 2024
b6b2fbc
Merge branch 'main' into fix/posix-path-separators
gmottajr Jan 11, 2024
2c9de4d
Merge branch 'fix/posix-path-separators' of https://github.com/gmotta…
gmottajr Jan 11, 2024
ca3059c
Third round of the code review asked to avoid using this '*' all patt…
gmottajr Jan 11, 2024
3db8229
Third round of the code review asked to avoid using this '*' all patt…
gmottajr Jan 11, 2024
4b6117c
Merge branch 'main' into fix/posix-path-separators
gmottajr Jan 11, 2024
b690f67
Sort and add missing entries to CODEOWNERS
krysal Jan 15, 2024
fd20641
Merge branch 'main' into fix/posix-path-separators
krysal Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 1 addition & 22 deletions .github/CODEOWNERS
View file
Open in desktop
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,7 @@ archive/ @WordPress/openverse-catalog

api/ @WordPress/openverse-api
ingestion_server/ @WordPress/openverse-api
nginx/ @WordPress/openverse-api
postgres/ @WordPress/openverse-api
sample_data/ @WordPress/openverse-api
.flake8 @WordPress/openverse-api
.isort.cfg @WordPress/openverse-api
docker-compose.yml @WordPress/openverse-api
load_sample_data.sh @WordPress/openverse-api

Expand All @@ -22,21 +18,4 @@ pnpm-workspace.yaml @WordPress/openverse-frontend
prettier.config.js @WordPress/openverse-frontend
.prettierignore @WordPress/openverse-frontend

automations/ @WordPress/openverse-maintainers
brand/ @WordPress/openverse-maintainers
dead_links/ @WordPress/openverse-maintainers
docker/ @WordPress/openverse-maintainers
documentation/ @WordPress/openverse-maintainers
readme_assets/ @WordPress/openverse-maintainers
templates/ @WordPress/openverse-maintainers
utilities/ @WordPress/openverse-maintainers
.gitattributes @WordPress/openverse-maintainers
.gitignore @WordPress/openverse-maintainers
.github/ @WordPress/openverse-maintainers
.devcontainer/ @WordPress/openverse-maintainers
.pre-commit-config.yaml @WordPress/openverse-maintainers
CODE_OF_CONDUCT.md @WordPress/openverse-maintainers
CONTRIBUTING.md @WordPress/openverse-maintainers
justfile @WordPress/openverse-maintainers
LICENSE @WordPress/openverse-maintainers
README.md @WordPress/openverse-maintainers
* @WordPress/openverse-maintainers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

12 changes: 12 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,15 @@ repos:
- id: renovate-config-validator
args:
- --strict

- repo: local
hooks:
- id: codeowners-validator
name: CODEOWNERS validator
language: docker_image
pass_filenames: false
files: CODEOWNERS$
sarayourfriend marked this conversation as resolved.
Show resolved Hide resolved
entry: >
-e REPOSITORY_PATH="."
-e CHECKS="files,duppatterns,syntax"
ghcr.io/mszostok/codeowners-validator:v0.7.4
Comment on lines +148 to +151
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Collaborator

@sarayourfriend sarayourfriend Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.