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

feat: add pre-commit hook for running the format.sh script #3637

Merged
merged 5 commits into from May 19, 2022

Conversation

vinayakkulkarni
Copy link
Contributor

Example:
Screenshot 2022-05-18 at 8 18 48 PM

PS. This also lints the Dockerfiles using Hadolint :)

PPS. Thanks for this amazing open-source software ❤️

Signed-off-by: Vinayak Kulkarni <19776877+vinayakkulkarni@users.noreply.github.com>
Signed-off-by: Vinayak Kulkarni <19776877+vinayakkulkarni@users.noreply.github.com>
@kevinkreiser
Copy link
Member

this looks good to me, what do you say @nilsnolde ?

scripts/bash_utils.sh Outdated Show resolved Hide resolved
@@ -12,15 +12,13 @@ readonly CLANG_FORMAT_VERSION=7.0.0

source scripts/bash_utils.sh
setup_mason
setup_pre_commit
Copy link
Member

Choose a reason for hiding this comment

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

cheeky but that's a really good idea:)

@nilsnolde
Copy link
Member

jep thanks a ton @vinayakkulkarni . this should save some serious resources on circleci I hope:)

@nilsnolde
Copy link
Member

ah, and don't forget a changelog entry (bottom of the list for new entries)

Signed-off-by: Vinayak Kulkarni <19776877+vinayakkulkarni@users.noreply.github.com>
Signed-off-by: Vinayak Kulkarni <19776877+vinayakkulkarni@users.noreply.github.com>
@nilsnolde nilsnolde self-requested a review May 19, 2022 09:20
nilsnolde
nilsnolde previously approved these changes May 19, 2022
@vinayakkulkarni
Copy link
Contributor Author

ah, and don't forget a changelog entry (bottom of the list for new entries)

This gives me an idea of an automation script for doing this instead of manually updating it each time a PR is created

@nilsnolde
Copy link
Member

I thought about that recently, where my main motivation was the super annoying merge conflicts resulting from changelog entries and circleci not skipping markdown changes (no good solution came out of thinking/googling).

I understand it's a little annoying and even I forget it sometimes, but any automation would need every contributor to be really decent about either their commit messages or their PR titles I assume right? and how would that deal with merge conflicts resulting from different PRs?

@kevinkreiser
Copy link
Member

kevinkreiser commented May 19, 2022

I would just like to raise one concern. If you merge the docker lint precommit stuff but don't fix the dockerfiles the next person who runs this will be forced to deal with it. I think that's not a very good way to do it. I'd either fix the dockerfiles in this pr or disable the docker linting before merging

@vinayakkulkarni
Copy link
Contributor Author

I would just like to raise one concern. If you merge the docker lint precommit stuff but don't fix the dockerfiles the next person who runs this will be forced to deal with it. I think that's not a very good way to do it. I'd either fix the dockerfiles in this pr or disable the docker linting before merging

Hey @kevinkreiser, I decided to remove the hadolint in this PR and create a new PR for formatting the Dockerfiles, keeping this PR clean by just installing the the pre-commit CLI & running the scripts/format.sh each time users/contributors commit to their respective branches.

kevinkreiser
kevinkreiser previously approved these changes May 19, 2022
@kevinkreiser kevinkreiser dismissed stale reviews from nilsnolde and themself via a3efde5 May 19, 2022 12:41
@nilsnolde nilsnolde merged commit 5f38946 into valhalla:master May 19, 2022
@vinayakkulkarni vinayakkulkarni deleted the feat/add-pre-commit-config branch May 20, 2022 07:04
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.

None yet

3 participants