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

Not running govet or gofmt in Dockerfile builds #1283

Merged
merged 2 commits into from
Dec 7, 2020

Conversation

arschles
Copy link
Contributor

@arschles arschles commented Oct 22, 2020

Since we have code quality checks on the v2 branch, they will run prior to any docker build operation. That means that the go fmt and go vet calls inside the Dockerfile are redundant. This change removes them by creating two new makefile targets: adapter-dockerfile and manager-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 and go vet explicitly in the pre commit file, as it'll be done automatically by the pre-commit golangci hook

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)
  • Tests have been added
  • A PR is opened to update the documentation on https://github.com/kedacore/keda-docs
  • Changelog has been updated

Fixes #1070

Notes about TODOs:

  • Tests don't need to be added here
  • No docs updates needed as well

Not sure about the changelog

@arschles
Copy link
Contributor Author

The static check is failing because of the go-vet pre-commit hook. That code is here. We are working on a patch for that code, at which point we'll update this PR 😄

@arschles
Copy link
Contributor Author

We have two patches proposed, hopefully the maintainer will choose one of them: dnephin/pre-commit-golang#63 and dnephin/pre-commit-golang#62

@zroubalik zroubalik changed the base branch from v2 to main October 30, 2020 13:23
@zroubalik
Copy link
Member

@arschles hi, any update on this?

@arschles
Copy link
Contributor Author

@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.

@zroubalik
Copy link
Member

@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 :)

@arschles
Copy link
Contributor Author

arschles commented Dec 1, 2020

@zroubalik this comment suggests to just use golangci-lint in our CI (which already does a go vet). I think that would solve the problem nicely because right now, we're doing two go vet calls. What do you think?

@zroubalik
Copy link
Member

@zroubalik this comment suggests to just use golangci-lint in our CI (which already does a go vet). I think that would solve the problem nicely because right now, we're doing two go vet calls. What do you think?

@arschles agree!

Signed-off-by: Aaron Schlesinger <[email protected]>
@arschles
Copy link
Contributor Author

arschles commented Dec 3, 2020

@zroubalik I removed the go-vet precommit check and things seem better now. PTAL

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@zroubalik zroubalik merged commit 0626a20 into kedacore:main Dec 7, 2020
@arschles
Copy link
Contributor Author

arschles commented Dec 9, 2020

Thanks @zroubalik 😄

@arschles arschles deleted the gofmt branch December 9, 2020 19:22
ycabrer pushed a commit to ycabrer/keda that referenced this pull request Mar 1, 2021
* 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]>
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.

Decouple go fmt and vet from building docker image
2 participants