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

CI: pre-commit check for generated files #199

Merged
merged 9 commits into from
Sep 19, 2024

Conversation

justincoh
Copy link
Contributor

@justincoh justincoh commented Sep 18, 2024

Description

When updating a proto file last week, I forgot to update the docs. We have a github action check for this scenario but it's annoying to have to wait for such a simple fix.

This PR adds a non blocking pre-commit hook to remind the author if the docs or generated files need updating.

It does the following:

  1. If an update to a .proto file is being committed
  2. Check if the /docs and /proto/openfga/ folder are also being committed
  3. If /docs and /proto/openfga/ are being committed, hook exits successfully
  4. If one of the two above is not being committed, remind the author to update the docs but also exit successfully (non-blocking) block the commit and remind the author to update the generated files.
hook.message.mov

@justincoh justincoh force-pushed the feat/pre-commit-swagger-check branch from 5bc7b1b to a5d8b0d Compare September 18, 2024 14:29
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
README.md Show resolved Hide resolved
@justincoh justincoh marked this pull request as ready for review September 18, 2024 14:36
@justincoh justincoh requested a review from a team as a code owner September 18, 2024 14:36
@justincoh justincoh requested a review from willvedd September 18, 2024 14:36
.githooks/pre-commit Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
.githooks/pre-commit Outdated Show resolved Hide resolved
@justincoh justincoh requested a review from willvedd September 18, 2024 19:28
willvedd
willvedd previously approved these changes Sep 18, 2024
Copy link
Contributor

@willvedd willvedd left a comment

Choose a reason for hiding this comment

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

Great job 👍

@justincoh justincoh changed the title CI: pre-commit swagger check CI: pre-commit check for generated files Sep 18, 2024
@jpadilla
Copy link
Member

✨ nice!

We have a github action check for this scenario

I wonder if maybe the github action we have today can reuse the same logic introduced in the precommit script?

@rhamzeh
Copy link
Member

rhamzeh commented Sep 19, 2024

I deleted my comment b/c realized we're not modifying the github action in this PR.

In a follow up PR, can you fix that?

https://github.com/openfga/api/blob/main/.github/workflows/review.yaml#L33-L36

We don't need to limit to docs and protos, any change anywhere should error.

(not a big issue b/c at the moment, we're not likely to modify those files, but if that changes or e.g. someone updates go.mod manually and keeps go.sum not updated)

@justincoh justincoh merged commit 339f6b8 into main Sep 19, 2024
6 checks passed
@justincoh justincoh deleted the feat/pre-commit-swagger-check branch September 19, 2024 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants