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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
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
31 changes: 19 additions & 12 deletions .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
@@ -1,42 +1,49 @@
catalog/ @WordPress/openverse-catalog
archive/ @WordPress/openverse-catalog
# Specific assignments for the 'openverse-catalog' group
catalog/ @WordPress/openverse-catalog
archive/ @WordPress/openverse-catalog
dag-sync.sh @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
pyproject.toml @WordPress/openverse-api

frontend/ @WordPress/openverse-frontend
packages/ @WordPress/openverse-frontend
.eslintignore @WordPress/openverse-frontend
.eslintrc.js @WordPress/openverse-frontend
.npmrc @WordPress/openverse-frontend
.pnpmfile.cjs @WordPress/openverse-frontend
.prettierignore @WordPress/openverse-frontend
package.json @WordPress/openverse-frontend
pnpm-lock.yaml @WordPress/openverse-frontend
pnpm-workspace.yaml @WordPress/openverse-frontend
prettier.config.js @WordPress/openverse-frontend
.prettierignore @WordPress/openverse-frontend
setup_plausible.sh @WordPress/openverse-frontend
tsconfig.base.json @WordPress/openverse-frontend

# Specific assignments for the 'openverse-maintainers' group
.codespell/ @WordPress/openverse-maintainers
.devcontainer/ @WordPress/openverse-maintainers
.github/ @WordPress/openverse-maintainers
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
.dockerignore @WordPress/openverse-maintainers
.editorconfig @WordPress/openverse-maintainers
.git-blame-ignore-revs @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
docker-compose.yml @WordPress/openverse-maintainers
env.template @WordPress/openverse-maintainers
justfile @WordPress/openverse-maintainers
11 changes: 11 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,14 @@ repos:
- id: renovate-config-validator
args:
- --strict

- repo: local
hooks:
- id: codeowners-validator
name: CODEOWNERS validator
language: docker_image
pass_filenames: false
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

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.