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

Update pre commit hooks to check spelling and lint-fix on pre commit #5440

Merged
merged 3 commits into from
Feb 20, 2025

Conversation

bryan-cox
Copy link
Contributor

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This PR:

  • moves the pre-commit hook for spellcheck from pre-push to pre-commit
  • adds make lint-fix to the pre-commit hook

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
N/A

Special notes for your reviewer:

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests
  • cherry-pick candidate

Release note:

None

This commit moves the pre-commit hook for spellcheck from pre-push to
pre-commit.

Signed-off-by: Bryan Cox <[email protected]>
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 19, 2025
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 19, 2025
@bryan-cox
Copy link
Contributor Author

Hey @nawazkh 👋🏻 - I can't remember if we put the spellcheck on pre-push for some reason previously but this PR moves it to pre-commit so those are caught up front.

I also added make lint-fix. I was able to push go files that were not gci-ed properly. That should probably be caught before submitting a PR as well.

Copy link

codecov bot commented Feb 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.41%. Comparing base (0f04d8d) to head (30c0b16).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5440   +/-   ##
=======================================
  Coverage   52.41%   52.41%           
=======================================
  Files         272      272           
  Lines       29361    29361           
=======================================
  Hits        15390    15390           
  Misses      13165    13165           
  Partials      806      806           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bryan-cox bryan-cox changed the title Update pre commit hooks Update pre commit hooks to check spelling and lint-fix on pre commit Feb 19, 2025
@nawazkh
Copy link
Member

nawazkh commented Feb 19, 2025

A welcome change imo. Whats the total time to completion you see upon adding these two additional stages to pre-commit ?

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 19, 2025
@bryan-cox
Copy link
Contributor Author

bryan-cox commented Feb 19, 2025

Whoops I meant to add make verify-codespell initially. It's been added.

I did a quick test of 3 commits without the PR and 3 commits with the PR. It's about 6 seconds slower with the PR. The average times were:

  • without the PR 0.47s
  • with the PR 6.39s

Comment on lines 61 to 66
- id: make-verify-codespell
name: Run make verify-codespell
entry: make verify-codespell
stages: [ pre-commit ]
language: system
require_serial: true
Copy link
Member

Choose a reason for hiding this comment

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

What do you say if we move make verify-codespell to pre-push stage ?

I would love to keep the git commit time to be as short as possible.
Besides, we do run all the verify tests as part of a job. Reference conversation: #5213

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, the removed a second off the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change has been pushed

Copy link
Member

Choose a reason for hiding this comment

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

I tested make verify-lint in my local and it took the system 2m 24s to complete it.
What do you say to moving verify-lint to pre-push stage as well? Since verify-lint is not commit restricted, it should still check the linter-errors in complete changes.

Sorry, not trying to be a bum about this 😶‍🌫️ I am quite inclined on having faster commit times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow 2m 🤯

Sure I can move it to pre-push.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@bryan-cox bryan-cox force-pushed the update-pre-commit-hooks branch from c2ca0c4 to 4adcd14 Compare February 20, 2025 12:24
This commit adds make lint-fix to the pre-commit hook.

Signed-off-by: Bryan Cox <[email protected]>
This commit adds make verify-codespell to the pre-commit hook.

Signed-off-by: Bryan Cox <[email protected]>
@bryan-cox bryan-cox force-pushed the update-pre-commit-hooks branch from 4adcd14 to 30c0b16 Compare February 20, 2025 20:15
@nawazkh
Copy link
Member

nawazkh commented Feb 20, 2025

Thank you Bryan!
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 111d2c9b87c66a703e92129ee05e2cf78c18080e

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nawazkh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 20, 2025
@k8s-ci-robot k8s-ci-robot merged commit 3895a4a into kubernetes-sigs:main Feb 20, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants