-
Notifications
You must be signed in to change notification settings - Fork 102
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: added pre-commit #811
ci: added pre-commit #811
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #811 +/- ##
=======================================
Coverage 35.34% 35.34%
=======================================
Files 20 20
Lines 1901 1901
=======================================
Hits 672 672
Misses 1195 1195
Partials 34 34 ☔ View full report in Codecov by Sentry. |
.pre-commit-config.yaml
Outdated
hooks: | ||
- id: prettier | ||
files: \.(md|ya?ml|json)$ | ||
exclude: ^(CHANGELOG.md|chart/templates/.*|chart/.snapshots/.*)$ |
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.
I'd rather keep the snapshots in there, if pre-commit fails on the snapshots, the templates might need a fix.
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.
Running prettier and helm template causes some conflicts. pre-commit (prettier) wants to format the yaml. In the CI we check for a diff after running helm template
, which always fails as the formatting is different.
Should we write a custom pre-commit hook to address this?
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.
Yes, this what I had in mind with #811 (comment)
There are a bunch of script in https://github.com/hetznercloud/csi-driver/tree/main/hack that could be run as part of pre-commit :) This could also remove a few CI jobs running those scripts. |
Co-authored-by: Jonas L. <[email protected]>
Co-authored-by: Jonas L. <[email protected]>
Co-authored-by: Jonas L. <[email protected]>
We added the pre-commit configuration file and simplified our CI scripts.
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.
The snapshots are ignored by prettier because it is a pain to fix the helm templates, this task can be picked up by anyone having the energy to do it.
🧹
This PR introduces a pre-commit config which is used in our ci pipeline. Furthermore, an additional formatting of all affected resources was done.
The following config was used as a baseline: https://github.com/hetznercloud/.github/blob/main/.pre-commit-config.yaml