feat: Add a very basic pre-commit configuration

[hawksight]

In #680 there were a couple of nits that I would personally solve by using pre-commit. I have added the most basic configuration here as a discussion topic. There may be other ways or tools to achieve the same goal of "not having to correct whitespace or newline nits"

Assuming you have this checked out:

# run once
pre-commit install
# run on any changed files
pre-commit run
# run on specific file(s)
pre-commit run --files deploy/charts/trust-manager/values.yaml

Reasons to have it

• Standard tool available pretty widely
• Sets a very low baseline of checks, to help contributors to catch empty space and newline problems.
• Fixes the issue before committing so it is easier to get right first time
• Only runs on the files you have changed / staged (unless you specify --all-files)
• It is opt in! ie. it should not affect existing users by default. You must set it up per repo once, if want to benefit.

Concerns / addressed

I've left this section to record concerns about adding this, in the hope they can be addressed.

Will talk about this on some standup 🤞

[cert-manager-prow]

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

[cert-manager-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jakexks 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

[maelvls]

Hey,

I've tested it and it works:

$ brew install pre-commit
$ pre-commit install
$ echo "    " >>deploy/charts/trust-manager/values.yaml
$ git ci -a -m test
trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing deploy/charts/trust-manager/values.yaml

fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing deploy/charts/trust-manager/values.yaml

Upon testing it, I am slightly annoyed by the slowness of git commit, especially in context of large rebases.

I'm using my editor (VS Code + trailing-spaces extension) for spotting trailing whitespace and removing them, and also for spotting missing EOF newlines.

Not sure what to do next, so leaving it up to other maintainers! 😇

[erikgb]

I am not a huge fan of pre-commit hooks, but I have no issues with them being added to our projects - if it's helpful for other contributors. I just tested this feature branch locally, and as I don't use pre-commit hooks everything seems to be working as before.

Change looks good to me, but I would like @SgtCoDFish to have an opinion on this as well.

/lgtm

[SgtCoDFish]

My starting point for reviewing this is: would this cause a large maintenance burden or would this break anything? I don't think it'll do either of those things - for most it would at worst be a slight distraction.

I wouldn't personally use this, because I really value a high level of control over my commits, but I can see why this might be valuable to others.

I'd be happy to merge this given it has very low impact - I think my gut says that this would be a good fit for a page under https://cert-manager.io/docs/contributing/ so it's not explicitly in-repo, but I don't think that's a blocker.

[cert-manager-prow]

New changes are detected. LGTM label has been removed.

[hawksight]

Adding comment to the end of the has inf323703 seemed to break functionality, so I added a proper reference in 69d704b.

[hawksight]

Thanks all for the feedback.

I'd be happy to merge this given it has very low impact - I think my gut says that this would be a good fit for a page under https://cert-manager.io/docs/contributing/ so it's not explicitly in-repo, but I don't think that's a blocker.

@SgtCoDFish - I could follow up with a PR to the website under contributing as you suggested? In the context, these are provided as optional to use?

[cert-manager-bot]

Issues go stale after 6 months of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues remain open for an additional 3 months of inactivity and then close.
If this issue is safe to close now please do so with /close.
/lifecycle stale

[cert-manager-bot]

Stale issues rot after 3 months of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 3 months of inactivity.
If this issue is safe to close now please do so with /close.
/lifecycle rotten
/remove-lifecycle stale