-
Notifications
You must be signed in to change notification settings - Fork 168
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
Replaces Travis with Github Action #465
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice - some small comments, but else looks good.
name: Go lint | ||
|
||
on: | ||
push: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use the push
entry here, but rather protect the repository against direct pushes to master
(which should be renamed to main
:) and only allow merges when the tests pass.
This avoids running an unnecessary lint when you merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe redundant in some cases, but it doesn't hurt. I feel safer having a second check :)
.github/workflows/go_lint.yml
Outdated
run: make lint | ||
|
||
- name: Vet | ||
run: make vet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add newline
name: Go test | ||
|
||
on: | ||
push: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above - you can remove push
if the repo is configured accordingly.
.github/workflows/go_tests.yml
Outdated
steps: | ||
- uses: shogo82148/actions-goveralls@v1 | ||
with: | ||
parallel-finished: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing newline
# Coding style static check. | ||
@go install honnef.co/go/tools/cmd/staticcheck@latest | ||
@go mod tidy | ||
#staticcheck `go list ./...` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to put the staticcheck in its own target? That would make more sense, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer having one target that gathers all vetting scripts. Except if there is a case where we need to launch staticcheck but not the other linting tools.
Makefile
Outdated
# target to run all the possible checks; it's a good habit to run it before | ||
# pushing code | ||
check: lint vet | ||
go test ./... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing newline
Kudos, SonarCloud Quality Gate passed! |
gofmt
andgo mod tidy
staticcheck
is commented out because there are a lot to fix