-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Not running govet or gofmt in Dockerfile builds #1283
Conversation
540ac13
to
acad632
Compare
The static check is failing because of the |
We have two patches proposed, hopefully the maintainer will choose one of them: dnephin/pre-commit-golang#63 and dnephin/pre-commit-golang#62 |
@arschles hi, any update on this? |
@zroubalik sorry for the delay. I've been busy with the HTTP scaling work but I haven't forgotten about this. There's a US holiday this week and I'm going on vacation but I'll finish this up early next week. |
No worries, enjoy your vacation! Thanks for the heads up :) |
Signed-off-by: Aaron Schlesinger <[email protected]>
@zroubalik this comment suggests to just use |
@arschles agree! |
Signed-off-by: Aaron Schlesinger <[email protected]>
@zroubalik I removed the go-vet precommit check and things seem better now. PTAL |
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.
LGTM, thanks!
Thanks @zroubalik 😄 |
* Not running govet or gofmt in Dockerfile builds Signed-off-by: Aaron Schlesinger <[email protected]> * not running go-vet in precommit Signed-off-by: Aaron Schlesinger <[email protected]>
Since we have code quality checks on the
v2
branch, they will run prior to any docker build operation. That means that thego fmt
andgo vet
calls inside the Dockerfile are redundant. This change removes them by creating two new makefile targets:adapter-dockerfile
andmanager-dockerfile
, and changing the adapter and manager dockerfiles to call those two targets, respectively.This patch also adds
go vet
to the pre-commit config file.Question: should we add go vet to the golangci config file? This way, we can avoid running
go fmt
andgo vet
explicitly in the pre commit file, as it'll be done automatically by the pre-commit golangci hookChecklist
Fixes #1070
Notes about TODOs:
Not sure about the changelog