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

add golangci-lint checks #77

Closed
wants to merge 6 commits into from
Closed

add golangci-lint checks #77

wants to merge 6 commits into from

Conversation

displague
Copy link
Member

@displague displague commented Dec 15, 2021

Description

Adds golangci-lint checks.

Because this project contains many sub-packages with their own go.mod file, a work-around was needed (copied from a comment on the relevant issue: golangci/golangci-lint#828 (comment)).

Why is this needed

This prevents the need for humans leaving common go review comments, many of which may be nitpics that would otherwise be dismissed resulting in poor code quality over time.

Fixes: #

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

Signed-off-by: Marques Johansson <[email protected]>
Signed-off-by: Marques Johansson <[email protected]>
Errors were detected with:

find . -name go.mod  | sed s/go.mod// | xargs -I{} sh -c 'cd {}; GOOS=linux golangci-lint run --path-prefix {} --fix ./...'

Initial problems captured at https://gist.github.com/displague/94aa6e20a2aba10adc6582222acd6ee5

Signed-off-by: Marques Johansson <[email protected]>
@mmlb
Copy link
Contributor

mmlb commented Dec 15, 2021

Can you give it a try with https://github.com/tinkerbell/lint-install instead? It should already have support for multiple modules and has golangci-lint config that is applied to other go repos in this org which will help to keep the code bases consistent with each other. It also supports other files types that are in this repo and would be good to have linted also.

@mmlb
Copy link
Contributor

mmlb commented Dec 15, 2021

Actually I messed around with doing this and have a bunch of other quick/misc things to fix needed. Give me a minute and I'll get my changes up for PR too.

@mmlb mmlb mentioned this pull request Dec 15, 2021
3 tasks
@displague
Copy link
Member Author

Closing for #78

See #78 (comment) for more details

@displague displague closed this Jan 11, 2022
mergify bot added a commit that referenced this pull request May 3, 2022
## Description

This PR mostly just makes use of https://github.com/tinkerbell/lint-install.
Some nixpkgs stuff came along for the ride as we were using a quite old pin.
I've cleaned up some of the code in hack/ci-check.sh so its clearer and produces better output if `go mod tidy` makes changes.
I also fixed a bug in .gitignore that would have caused new files in cmd/hub to be ignored.

## Why is this needed

I was prodded into this due to #77, which is using a different golangci-lint config than other projects and also doesn't lint other files.

## How Has This Been Tested?

`make lint` passes and `docker buildx build` worked for me on all the images.

## How are existing users impacted? What migration steps/scripts do we need?

Cleaner and/or more consistent code under tinkerbell/ org.

## Checklist:

I have:

- [ ] updated the documentation and/or roadmap (if required)
- [ ] added unit or e2e tests
- [ ] provided instructions on how to upgrade
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.

2 participants