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

ci: add golang lint and fixed. #157 #320

Merged
merged 2 commits into from
Oct 26, 2023
Merged

ci: add golang lint and fixed. #157 #320

merged 2 commits into from
Oct 26, 2023

Conversation

u5surf
Copy link
Contributor

@u5surf u5surf commented Oct 23, 2023

Fixes #157
Add golangci-lint on github actions ci and fixed some conventional issues of linting.

Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @u5surf! For starters, it's probably fine to go with the default linting as here: we can always add additional rules later 👍
I have a few questions and small CRs - please check the comments.

controllers/testrun_controller.go Outdated Show resolved Hide resolved
.github/workflows/golangci-lint.yaml Outdated Show resolved Hide resolved
pkg/types/k6cli.go Show resolved Hide resolved
.github/workflows/golangci-lint.yaml Outdated Show resolved Hide resolved
@yorugac
Copy link
Collaborator

yorugac commented Oct 24, 2023

One more thing: can you please add a small instruction / command on how to run this linter locally? It could be a target in Makefile or part of internal docs (i.e. docs/ folder). It can be very frustrating to fix linting in Github Actions so running it quickly locally is preferable.

Makefile Outdated
Comment on lines 102 to 104
lint:
go install github.com/golangci/golangci-lint/cmd/[email protected]
golangci-lint --timeout 30m run ./...
Copy link
Collaborator

@yorugac yorugac Oct 25, 2023

Choose a reason for hiding this comment

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

Nice 🙌 Timeout is pretty large: our code base is rather small so shouldn't need that much in theory 🤔 Either way, this would work, thanks for adding Makefile change!

Copy link
Contributor Author

@u5surf u5surf Oct 25, 2023

Choose a reason for hiding this comment

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

@yorugac
I could reduce the timeout to 5min.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 thanks. It should be fine with 5m, and if not, we can always tinker with it.

* use the latest version of golangci-lint in github actions
* add the target of lint in Makefile
* fix nolint errcheck
Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @u5surf, for working on this 😄

@yorugac yorugac merged commit 12d691e into grafana:main Oct 26, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve linting
2 participants