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.

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

Pre-commit and linting #112

Merged
merged 6 commits into from
Oct 19, 2021
Merged

Pre-commit and linting #112

merged 6 commits into from
Oct 19, 2021

Conversation

RothAndrew
Copy link
Member

@RothAndrew RothAndrew commented Oct 18, 2021

What

  • Add pre-commit hooks including golang linting
  • Update Contributor documentation

Why

By using pre-commit hooks we can easily and automatically catch simple issues. They run automatically on every git commit and we can validate that they ran by running them in the CI pipeline and failing if they report any anomalies.

Pay particular attention to the changes in CONTRIBUTING.md so the new prerequisites are well understood.

This first PR adds the .pre-commit-config file including the following hooks:

  • check-added-large-files: Fails if any files over 500kb are added. Is great for keeping repos small. If a bigger file needs to be added you can add it by passing the filename to an exceptions list.
  • check-merge-conflict: Ensures that there are no orphaned Git merge conflict strings in the codebase. These can appear when a git merge is done incorrectly.
  • detect-aws-credentials: Detects accidentally added AWS credentials before they make it into the Git history. Huzzah!
  • detect-private-key: Detects private SSH keys and private certificates before they get accidentally added to the git history
  • end-of-file-fixer: Ensures that every file in the codebase ends with a new line, which is a POSIX standard.
  • fix-byte-order-marker: Strips BOMs which are not recommended when using UTF-8
  • trailing-whitespace: Strips trailing whitespace (except for markdown since it has special meaning)
  • fix-smartquotes: Automatically changes smartquotes to dumb quotes. Robots hate smartquotes.
  • go-fmt: Automatically runs go fmt on the codebase
  • golangci-lint: Automatically runs a linter on the golang codebase. This one is commented out for now but it works and can be turned on very easily once we fix the linting errors (there aren't that many of them)

In the documentation I have said that the CI pipeline requires these checks, but that won't be true at the time of merging this PR. That will come later.

Fixes #74
Related to #75

@RothAndrew RothAndrew self-assigned this Oct 18, 2021
@RothAndrew
Copy link
Member Author

golangci-lint results:

image

@RothAndrew
Copy link
Member Author

/test all

@RothAndrew RothAndrew marked this pull request as ready for review October 19, 2021 01:07
@YrrepNoj
Copy link
Member

This doesn't add any checks in the CI pipeline right? Someone could try to be sneaky (or accidentally) commit with the --no-verify flag and push non-conforming code.

@RothAndrew
Copy link
Member Author

This doesn't add any checks in the CI pipeline right? Someone could try to be sneaky (or accidentally) commit with the --no-verify flag and push non-conforming code.

Right, or they could just not run pre-commit install. If this addition is agreeable to everyone we'll get it merged and then we can go back in and add the CI check.

jeff-mccoy
jeff-mccoy previously approved these changes Oct 19, 2021
Copy link
Member

@jeff-mccoy jeff-mccoy left a comment

Choose a reason for hiding this comment

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

LGTM as long as we communicate that this is an optional step locally but the pipeline will require it to merge.

# Conflicts:
#	test/e2e/terraform_ssh_e2e_test.go
@RothAndrew
Copy link
Member Author

/test all

@RothAndrew RothAndrew merged commit 34f5e62 into master Oct 19, 2021
@RothAndrew RothAndrew deleted the feature/pre-commit-and-linting branch October 19, 2021 20:31
jeff-mccoy pushed a commit that referenced this pull request Feb 8, 2022
Signed-off-by: Jeff McCoy <code@jeffm.us>
Noxsios pushed a commit that referenced this pull request Mar 8, 2023
Signed-off-by: Jeff McCoy <code@jeffm.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pre-Commit hooks
3 participants