-
Notifications
You must be signed in to change notification settings - Fork 212
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
Remove codeowners validator from pre-commit #3762
Conversation
The docker image has issues running on arm64, so we will instead run it in CI using the GitHub action, with an utility just recipe to run the same checks locally as needed
Full-stack documentation: https://docs.openverse.org/_preview/3762 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. Changed files 🔄: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and runs locally! Thanks for adding the documentation too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the workaround, documentation and separate just
command.
just lint-codeowners
does not work for me for the same reasons:
just lint-codeowners 1 ✘ 06:53:54
docker run --rm -u 1000:1000 -v $PWD:/src:rw,Z --workdir=/src -e REPOSITORY_PATH="." -e CHECKS="files,duppaterns,syntax" ghcr.io/mszostok/codeowners-validator:v0.7.4
WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
qemu: uncaught target signal 11 (Segmentation fault) - core dumped
Yes, that's precisely the issue with arm64 that Staci and Dhruv ran into that caused us to need to switch away from running it locally in pre-commit. The recipe is there for folks who can run it to be able to easily do so, which the documentation notes:
(Emphasis mine) In other words, that's intended behaviour :) Thanks for the reviews, y'all! |
Fixes
Fixes and issue reported by @dhruvkb and @stacimc they had running the pre-commit hook on their local machines.
Description
The docker image has issues running on arm64, so we will instead run it in CI using the GitHub action, with an utility just recipe to run the same checks locally as needed.
I also updated the CI/CD documentation to explain the new job and document the
lint-codeowners
recipe.'Marked high because it is affecting the ability for folks to work on the codebase without this tedious interruption.
Testing Instructions
CI should pass with the new job and the
just lint-codeowners
recipe should work, includingjust lint-codeowners all
to enable the experimental checks.Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin