Run local verifying targets too before git commit

[nawazkh]

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

• This PR update pre-commit-config.yaml to run local Makefile targets as part of pre-commit workflow.
  • verify-boilerplate
  • verify-modules
  • verify-shellcheck
  • verify-tiltfile
  • verify-codespell

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

Special notes for your reviewer:

Merge above PR before merging current PRs #3706 and #3630 before merging this PR.

• cherry-pick candidate

TODOs:

• squashed commits
• includes documentation
• adds unit tests

Release note:

Developer UX: Expand on the pre-commit workflow.

[nawazkh]

We are waiting on two PRs to merge before merging this PR.
I also need to add make verify-codespell to the local pre-commit config before merging this PR.
/hold

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: +0.76% 🎉

Comparison is base (b7a0d41) 54.09% compared to head (e54d5f4) 54.86%.
Report is 75 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3724      +/-   ##
==========================================
+ Coverage   54.09%   54.86%   +0.76%     
==========================================
  Files         186      188       +2     
  Lines       18849    19348     +499     
==========================================
+ Hits        10197    10615     +418     
- Misses       8105     8151      +46     
- Partials      547      582      +35     

see 30 files with indirect coverage changes

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

[Jont828]

Interesting change! I had a couple things to clarify:

1. Would adding these targets slow down the git commit workflow? It seems like it could get clunky to have the log lines.
2. Would it allow you to commit changes to your own branch if it fails the check? I know the shellcheck can be strict sometimes and I usually only clean up the verify issues when I'm ready for review.

[nawazkh]

Good questions! TL;DR, yes it does slow down the workflow bit.
However, IMO, it's worth the cost..?
Although, the PR is open for vote.

• Would adding these targets slow down the git commit workflow? It seems like it could get clunky to have the log lines.

Umm, it is a little subjective. How "slow" would be acceptable?
When everything is good, w.r.t the linters and checks, git commit takes around 26 seconds to complete.
Below is the output from my local

❯ git commit -m "update pre-commit file"
Detect hardcoded secrets.................................................Passed
golangci-lint........................................(no files to check)Skipped
Test shell scripts with shellcheck...................(no files to check)Skipped
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
pylint...............................................(no files to check)Skipped
Run make verify-boilerplate..............................................Passed
Run make verify-modules..................................................Passed
Run make verify-shellcheck...............................................Passed
Run make verify-tiltfile.................................................Passed
Run make verify-codespell................................................Passed
[update_pre-commit-config 18988055] add codespell to pre commit config
 1 file changed, 7 insertions(+)

However, when any of the files is wrong(w.r.t linters) or is being run for the first time, git commit could get slow. But it is usually a one-off occurrence.
Output from my local:
I had an error in the file being committed and the codespell bin also had to be installed too since verify-codespell was being run the first time.
It took 49s to complete.

❯ git commit -m "add codespell to pre commit config"
Detect hardcoded secrets.................................................Passed
golangci-lint........................................(no files to check)Skipped
Test shell scripts with shellcheck...................(no files to check)Skipped
fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing .pre-commit-config.yaml

trim trailing whitespace.................................................Passed
pylint...............................................(no files to check)Skipped
Run make verify-boilerplate..............................................Passed
Run make verify-modules..................................................Passed
Run make verify-shellcheck...............................................Passed
Run make verify-tiltfile.................................................Passed
Run make verify-codespell................................................Passed

[nawazkh]

• Would it allow you to commit changes to your own branch if it fails the check? I know the shellcheck can be strict sometimes and I usually only clean up the verify issues when I'm ready for review.

That's understandable :)
You could always use --no-verify with git commit command to skip running pre-commit configs.

[nawazkh]

/unhold

[mboersma]

It takes a little while to finish, even with small diffs. It does make me change my usual work pattern to be more deliberate about what I commit, but overall I like the tradeoff because I'll forget to run some of these verify targets otherwise.

But it is a change: people should really try it out and see what they think.

[mboersma]

I like these checks overall, and I've gotten used to using git commit --no-verify already in the situations where it won't pass (sometimes I need to checkpoint but I know there are test or linter problems, for example).

[Jont828]

I personally would prefer to work without it -- I typically prefer to save the checks until the end as a lot of my commits early on are usually very rough drafts. Is there a way to add an env config flag or file?

[sonasingh46]

+1 to what @Jont828 says.
Having said that, this can be great way to have pre-commit settings configured into local machines for a lot of contributors but I would too prefer the git thingy a bit flexible and not enforce it. IMO, it should be left to individual contributors on how they want to set it up on their dev env.

[Jont828]

Maybe a compromise would be to move this to a .dev/ folder where folks can easily copy it to their pre-commit config? I think I would really prefer this to be an opt-in change.

[nawazkh]

Happy to hear your thoughts! :)

This is an opt-in change rather than an enforcing one.
This PR merely adds to the already existing pre-commit config added via #3703.

Contributors should be running pre-commit install to enable this workflow in their local dev. If they want to opt out they can run pre-commit uninstall and the pre-commit checks would not interfere anymore.

If someone wants to skip checks on individual commits after installing the pre-commit framework, then one can run git commit --no-verify to skip the checks run on the commit.

[Jont828]

@nawazkh I think a make target is great idea! Could we add pre-commit changes from #3703 to the make target as well?

[nawazkh]

@nawazkh I think a make target is great idea! Could we add pre-commit changes from #3703 to the make target as well?

That's a good thought! How about I follow it up with another PR? The ones in this PR already have make targets to them.

[Jont828]

Another PR sounds good to me. Also, I don't seem to see any Makefile changes in this PR, just the pre-commit-config.yaml changes.

[mboersma]

/lgtm

I've been using the pre-commit hook since it merged, and I've tried this expanded version out. It fits my workflow and doesn't add too much time to git commit IMHO. I use --no-verify whenever I need to commit work in progress that I know won't pass these checks.

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: bef1cfcb82b48a89626a0ed5cf119727ca1f2fad

[Jont828]

/lgtm

@nawazkh Just to double check, if you don't run pre-commit install, we can opt out of this feature right?

[CecileRobertMichon]

@nawazkh Just to double check, if you don't run pre-commit install, we can opt out of this feature right?

@Jont828 correct, see #3724 (comment)

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 [CecileRobertMichon]

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