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.

Sign up for GitHub

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

Jump to bottom

feat: add pre-commit hooks #798

Merged
merged 12 commits into from
Sep 4, 2024
Merged

feat: add pre-commit hooks #798

merged 12 commits into from
Sep 4, 2024

Conversation

jackhodgkiss
Copy link
Contributor

Add a simple playbook that can install pre-commits and register a few simple hooks. This process is implemented as a hook that will run post control host bootstrap.

  • check-yaml: perform basic yaml syntax linting
  • end-of-file-fixer: identify and automatically fix missing newline
  • trailing-whitespace: identify and automatically fix excessive white space
  • ripsecrets: identify and prevent secrets from being committed to the branch

Note: this is currently configured as an opt-in feature and will only install and register hooks if enable_pre_commit_hooks is set to true in install-pre-commit-hooks.yml

@jackhodgkiss jackhodgkiss requested a review from a team as a code owner November 23, 2023 16:17
Alex-Welsh
Alex-Welsh previously approved these changes Feb 12, 2024
View reviewed changes
markgoddard
markgoddard reviewed Feb 20, 2024
View reviewed changes
Copy link
Contributor

@markgoddard markgoddard left a comment

Choose a reason for hiding this comment

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

How are we expecting this to be used? Is it something that would be enabled for everyone in a given Kayobe config, or opted into per-person?

markgoddard
markgoddard suggested changes Feb 20, 2024
View reviewed changes
Copy link
Contributor

@markgoddard markgoddard left a comment

Choose a reason for hiding this comment

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

It would be nice to provide some simple docs on this in the contributor guide.

etc/kayobe/ansible/install-pre-commit-hooks.yml Outdated Show resolved Hide resolved
etc/kayobe/ansible/install-pre-commit-hooks.yml Outdated Show resolved Hide resolved
@jackhodgkiss
Copy link
Contributor Author

How are we expecting this to be used? Is it something that would be enabled for everyone in a given Kayobe config, or opted into per-person?

I think having it is as an opt-in at least within SKC it the sensible move. If individual engineers want to enable it within their own checkout of the site config they can do and in situations where their is a single checkout shared enabling it would a group decision.

Alex-Welsh
Alex-Welsh previously approved these changes Apr 12, 2024
View reviewed changes
Copy link
Contributor

@Alex-Welsh Alex-Welsh left a comment

Choose a reason for hiding this comment

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

My one lingering concern is that false positives could break things but they can be skipped with --no-verify so I think the benefits outweigh the risks

markgoddard
markgoddard suggested changes Apr 15, 2024
View reviewed changes
etc/kayobe/ansible/install-pre-commit-hooks.yml Outdated Show resolved Hide resolved
etc/kayobe/ansible/install-pre-commit-hooks.yml Outdated Show resolved Hide resolved
doc/source/contributor/pre-commit.rst Show resolved Hide resolved
@markgoddard markgoddard dismissed priteau’s stale review April 15, 2024 15:37

Requested change has been applied

markgoddard
markgoddard suggested changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@markgoddard markgoddard left a comment

Choose a reason for hiding this comment

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

Actually perhaps this should go to antelope only?

@Alex-Welsh
Copy link
Contributor

@jackhodgkiss should we get this merged? I'm happy to rebase it on antelope for you if you're busy

@markgoddard markgoddard changed the base branch from stackhpc/yoga to stackhpc/2023.1 August 21, 2024 11:25
jackhodgkiss and others added 7 commits August 22, 2024 13:10
@jackhodgkiss
feat!: change when pre-commit hooks are installed
ef2a2b3 
Initally `pre-commit` hooks were installed due the presense of a hook
for `control host bootstrap` and when `enable_pre_commit_hooks` was set
to `true`. However now `pre-commit` hooks are only installed when the
hook is present or the playbook is called, no requirement for an
`Ansible` conditional to be true.
@jackhodgkiss
fix: update release note for pre-commit hooks
05ccc8e
@jackhodgkiss
feat: add pre-commit to contributor docs
ac3abce
@jackhodgkiss
fix: remove unused variable
94e52f4
@jackhodgkiss
feat: run pre-commit install if package is installed or updated
7c4da9a
@jackhodgkiss
feat: use variable to control pre_commit version
cf4aaa0
@jackhodgkiss
feat: provide commands within docs for pre-commit hooks setup
54766db
@markgoddard markgoddard merged commit 65b4b98 into stackhpc/2023.1 Sep 4, 2024
12 checks passed
@markgoddard markgoddard deleted the add-pre-commit-hooks branch September 4, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markgoddard markgoddard markgoddard approved these changes

@priteau priteau Awaiting requested review from priteau

@Alex-Welsh Alex-Welsh Awaiting requested review from Alex-Welsh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jackhodgkiss @Alex-Welsh @priteau @markgoddard