Enforce pre-commit to be running in github actions

[mtrqq]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR configures a github action to periodically run pre-commit on branch push and PR to validate hook states, minimal set of hooks is selected to avoid slowing down development flow. Biggest sources of generated files are explicitly excluded from the config, but not all of them - other entries should be populated as we go

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[adrianmoisey]

I'm not a fan of this, I think we should get rid of pre-commit in this repo.

There is a lot of generated or vendored content in this repo, which I don't believe pre-commit should be touching.
I also don't think we should be formatting the entire repo with a single version of a single tool (ie: go fmt). Each project within this repo could have different standards

[mtrqq]

I'm not a fan of this, I think we should get rid of pre-commit in this repo.

There is a lot of generated or vendored content in this repo, which I don't believe pre-commit should be touching. I also don't think we should be formatting the entire repo with a single version of a single tool (ie: go fmt). Each project within this repo could have different standards

I completely agree with your take, generated content shouldn't be managed/verified by pre-commit and this is easily controlled by excluded files/folders in the configuration file, I've added couple initial entries referring to vendor folders in the repository I know about and API docs of vertical pod autoscaler which conflicts due to controller-gen adding extra spaces for markdown renderer.

As for the go fmt selection, my reasoning was that it's defacto a standard and it found 0 issues in the repository as there's an existing script used in CI with pretty much same verification: https://github.com/kubernetes/autoscaler/blob/master/hack/verify-gofmt.sh. We may control target files and exclusions on the per-hook basis so if we want to exclude some project area from it - it's more than doable.

Considering all above I think pre-commit may still be useful for development flow, I personally found myself messing up the formatting of the golang file right before the commit so that it causes verification CI build to fail - this change essentially enforces automatic tool which does that for you.

[omerap12]

TBH,I agree with Adrian here. I think we should get rid of pre-commit entirely in this repo.

Considering all above I think pre-commit may still be useful for development flow, I personally found myself messing up the formatting of the golang file right before the commit so that it causes verification CI build to fail - this change essentially enforces automatic tool which does that for you.

IMO, we should stick with how k/k do things - each sub-repo should have a verify-gofmt.sh script which can verify whatever each sub repo wants.
Also, in this PR there are many changes that seemed unnecessary to me.

[adrianmoisey]

TBH,I agree with Adrian here. I think we should get rid of pre-commit entirely in this repo.

To be clear, Omer and I only really speak for the VPA section. If the other projects in this repo want pre-commit, they're welcome to have it

[mtrqq]

IMO, we should stick with how k/k do things - each sub-repo should have a verify-gofmt.sh script which can verify whatever each sub repo wants.

Contributors may not install pre-commit and it'll also work just fine as long as the validation passes so the contributor flow doesn't alter significantly, while improving developer productivity for those who decide to opt-in into installing hooks locally. Validation requirements on the CI are quite minimal and in my opinion non controversial, but I am absolutely open to dropping specific checks if some of them feel excessive.

Also, in this PR there are many changes that seemed unnecessary to me.

As for the large number of changes in this PR, I ran all the hooks across the repo intentionally. The goal was to consolidate all baseline formatting updates into a single PR so we don't pollute future feature PRs with unrelated formatting noise. Alternatively we may incrementally do them once the file is getting changed for one or another reason, but to my mind it's more intrusive option in the long term

[adrianmoisey]

Validation requirements on the CI are quite minimal and in my opinion non controversial, but I am absolutely open to dropping specific checks if some of them feel excessive.

I think the entire vertical-pod-autoscaler directory can be excluded, since we don't want to use pre-commit on it

[mtrqq]

@adrianmoisey I've excluded VPA from the pre-commit configuration, it won't be checked in CI and hooks will ignore the folder completely, do you want me to revert all the changes done to VPA folder in this PR like line endings and new lines at the end of the file?

[mtrqq]

@elmiko @jackfrancis Can you review this PR from cluster autoscaler perspective as you've recently reviewed the PR connected to pre-commit configuration?

Ref: #9129

[adrianmoisey]

do you want me to revert all the changes done to VPA folder in this PR like line endings and new lines at the end of the file?

Yup, please do, those don't bother us.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mtrqq
Once this PR has been reviewed and has the lgtm label, please assign towca for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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]

@mtrqq: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-autoscaling-e2e-gci-gce-ca-test	c39037b	link	false	/test pull-autoscaling-e2e-gci-gce-ca-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.