feat: add pre commit hook that checks if make proto-all should be run #6678
Conversation
PanGan21
requested review from
colin-axner,
AdityaSripal,
crodriguezvega,
damiannolan,
chatton,
DimitrisJim,
gjermundgaraba and
bznein
as code owners
scripts/hooks/pre-commit.sh
Outdated
| echo "Error: Please run 'make proto-all' and commit the updated files." | ||
|
|
||
| # Revert the changes made by make proto-all | ||
| for file in $changed_files; do |
There was a problem hiding this comment.
A drawback I can see with this approach is that the pre commit hook becomes quite slower.
I was thinking of the approach to just check if any .proto file is changed but this doesn't guarantee whether the command should be run
There was a problem hiding this comment.
yea, pretty sure this would make it very frustrating for folks. I'd be fine with doing a check on if any protos have been modified, run it, else just leave as is.
There was a problem hiding this comment.
Refactored. Now the script checks in the unstaged .proto files. If it finds a change it runs make proto-all and logs the files that were changed. If it doesn't find a change in protos then it just exits
scripts/hooks/pre-commit.sh
Outdated
| local after_files=$(git status --porcelain | awk '{print $2}') | ||
| local changed_files=$(comm -13 <(echo "$before_files" | sort) <(echo "$after_files" | sort)) | ||
| if [[ -n "$changed_files" ]]; then | ||
| echo "Error: Please run 'make proto-all' and commit the updated files." |
There was a problem hiding this comment.
Hm, thinking the idea here was to automatically just invoke it if proto changes have been made? @bznein
There was a problem hiding this comment.
That was my idea, yeah. This is still better than what we have now, but having it ran automatically would be ideal to me
There was a problem hiding this comment.
Thanks for the review!
I changed the script to run make proto-all and do git add to the generated files. Le me know what you think :)
|
hi @PanGan21, unfortunately we realized that a pre-commit would be very time-consuming and disrupting for the dev workflow and we decided to abandon this issue. Thanks for taking to time to work on it, really appreciated! |
Description
Adds pre commit hook that checks if
make proto-allshould be run.closes: #6668
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.