Improve pre-commit UX #5320

bryan-cox · 2024-12-04T15:59:02Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This PR improves the user experience of pre-commit by:

adding default stages and default install hook types to the pre-commit configuration
adding the stages in which certain hooks should be run
adding make verify and make test to the pre-push stage

Example output:

pre-commit hook run

 % pre-commit run --hook-stage pre-commit
Detect hardcoded secrets.................................................Passed
ShellCheck v0.10.0...................................(no files to check)Skipped
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
detect private key.......................................................Passed
pylint...............................................(no files to check)Skipped
Forbid binaries......................................(no files to check)Skipped

pre-push hook run

% git push -f
Run make verify-modules..................................................Passed
Run make verify-gen......................................................Passed
Run make verify-shellcheck...............................................Passed
Run make verify-conversions..............................................Passed
Run make verify-tiltfile.................................................Passed
Run make test............................................................Passed
Enumerating objects: 5, done.

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

Special notes for your reviewer:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

NONE

/assign @nawazkh

codecov · 2024-12-04T16:11:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.52%. Comparing base (1275143) to head (b33199c).
Report is 9 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5320   +/-   ##
=======================================
  Coverage   52.52%   52.52%           
=======================================
  Files         272      272           
  Lines       29422    29422           
=======================================
  Hits        15455    15455           
  Misses      13168    13168           
  Partials      799      799

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

.pre-commit-config.yaml

This commit adds default stages and default install hook types to the pre-commit configuration. In addition, it adds the stages in which certain hooks should be run. Signed-off-by: Bryan Cox <[email protected]>

nawazkh · 2024-12-04T19:53:08Z

.pre-commit-config.yaml

+- repo: local
+  hooks:
+  - id: make-verify
+    name: Run make verify
+    entry: make verify
+    language: system
+    stages: [pre-push]


Do you think we can add a reference to Makefile's verify target in here or put up a comment?
A disclaimer saying that make verify is going to invoke a bunch of targets like
verify-boilerplate verify-modules verify-gen verify-shellcheck verify-conversions verify-tiltfile verify-codespell

Yeah we could use https://pre-commit.com/#hooks-description

Great! Let's add some text in here, as a disclaimer, make verify will run verify-boilerplate verify-modules verify-gen verify-shellcheck verify-conversions verify-tiltfile verify-codespell all these targets.

Sorry for the churn.
I see that boilerplate check is already run as a separate hook up above.
Do you think it would be beneficial to run these verify targets individually as separate hooks and drop redundant hooks like boilerplate and spellcheck ?

I see merit in either approach. Though I would lean to running the targets individually because then you can see them running one by one in a terminal rather than just one block you're waiting to finish on. Which option would you prefer?

Running them individually as separate hooks at pre-push stage sounds great to me.

PR is updated. I think it looks better with each one -

% git push -f Run make verify-modules..................................................Passed Run make verify-gen......................................................Passed Run make verify-shellcheck...............................................Passed Run make verify-conversions..............................................Passed Run make verify-tiltfile.................................................Passed Run make test............................................................Passed Enumerating objects: 5, done. ...

FWIW, the verify-gen took a similar amount of time as test so maybe test doesn't need broken up? 🤷🏻‍♂️

Thank you for pointing this out.
Both verify-gen and test seem to be running generate as a pre-req.

So maybe we don't break test target for backwards compatibility in CI and replace make test in pre-commit config with make go-test-race.
We can follow this up in another PR, non blocking comment.

nawazkh · 2024-12-04T19:58:19Z

.pre-commit-config.yaml

+- repo: local
+  hooks:
+  - id: make-test
+    name: Run make test
+    entry: make test
+    language: system
+    stages: [pre-push]


Same thought here. make test atm seems to be running generate as well. Ref:

cluster-api-provider-azure/Makefile

Line 701 in 1275143

test: generate go-test-race ## Run "generate" and "go-test-race" rules.

In my experience, make generate takes a lot of time to finish.

I wonder if we can drop generate as a dependency? Or

Maybe come up with a new make target that runs UT exclusively ? i.e. runs make go-test-race. So essentially we run make go-test-race in this hook. Although I have not tested the side affects of running make go-test-race by itself.

However, this comment can be a followed up another issue-PR.

This commit adds the same checks run in `make verify` without duplicating any previous hook runs in pre-commit to the pre-commit configuration. In addition, this commit adds `make test` to the pre-push stage of the pre-commit configuration. This will ensure `verify` and `test` are successfully passing before the commit is pushed. This will help reduce time and test resources by preventing `verify` and `test` from failing on the pull request in GitHub. Signed-off-by: Bryan Cox <[email protected]>

nawazkh · 2024-12-08T08:40:50Z

.pre-commit-config.yaml

+    name: Run make verify-shellcheck
+    entry: make verify-shellcheck
+    language: system
+    stages: [ pre-push ]


I am not sure if [ pre-push ] is any different to [pre-push] (extra space padding being added to pre-push string).

Could please paste sample output of using this .pre-commit.config in the PR description or as a comment?

This looks great to me btw!
/lgtm

I added some example output to the PR description

k8s-ci-robot · 2024-12-08T08:40:56Z

LGTM label has been added.

Git tree hash: df5e05410858492f9c3645ae91eeb71eb67d9e8b

nawazkh · 2024-12-08T08:41:40Z

You might wanna see this too, Matt :)
/cc @mboersma

mboersma

/lgtm
/approve

Thanks!

k8s-ci-robot · 2024-12-10T16:24:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mboersma

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:

~~OWNERS~~ [mboersma]

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 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. labels Dec 4, 2024

k8s-ci-robot requested review from Jont828 and jsturtevant December 4, 2024 15:59

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 4, 2024

bryan-cox force-pushed the 5318 branch from cece2ea to 7cf674b Compare December 4, 2024 16:32

bryan-cox commented Dec 4, 2024

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

bryan-cox commented Dec 4, 2024

View reviewed changes

.pre-commit-config.yaml Show resolved Hide resolved

Add default stages/install hook type to pre-commit

b596029

This commit adds default stages and default install hook types to the pre-commit configuration. In addition, it adds the stages in which certain hooks should be run. Signed-off-by: Bryan Cox <[email protected]>

nawazkh reviewed Dec 4, 2024

View reviewed changes

bryan-cox force-pushed the 5318 branch from 7cf674b to 29d45aa Compare December 4, 2024 19:55

nawazkh reviewed Dec 4, 2024

View reviewed changes

bryan-cox force-pushed the 5318 branch from 29d45aa to b33199c Compare December 6, 2024 14:06

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 6, 2024

nawazkh reviewed Dec 8, 2024

View reviewed changes

k8s-ci-robot assigned nawazkh Dec 8, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 8, 2024

k8s-ci-robot requested a review from mboersma December 8, 2024 08:41

nawazkh assigned mboersma Dec 9, 2024

mboersma approved these changes Dec 10, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 10, 2024

k8s-ci-robot merged commit 627c0fb into kubernetes-sigs:main Dec 10, 2024
23 checks passed

k8s-ci-robot added this to the v1.18 milestone Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve pre-commit UX #5320

Improve pre-commit UX #5320

bryan-cox commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

nawazkh Dec 4, 2024

bryan-cox Dec 4, 2024

nawazkh Dec 4, 2024

nawazkh Dec 4, 2024

bryan-cox Dec 5, 2024

nawazkh Dec 6, 2024

bryan-cox Dec 6, 2024

nawazkh Dec 9, 2024

nawazkh Dec 4, 2024

nawazkh Dec 8, 2024

bryan-cox Dec 8, 2024

k8s-ci-robot commented Dec 8, 2024

nawazkh commented Dec 8, 2024

mboersma left a comment

k8s-ci-robot commented Dec 10, 2024

Improve pre-commit UX #5320

Improve pre-commit UX #5320

Conversation

bryan-cox commented Dec 4, 2024 • edited Loading

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 8, 2024

nawazkh commented Dec 8, 2024

mboersma left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 10, 2024

bryan-cox commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading