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

Migrate to golangci-lint #1443

Merged
merged 7 commits into from
Jan 9, 2020
Merged

Conversation

sayboras
Copy link
Contributor

@sayboras sayboras commented Oct 12, 2019

Description

Migrate from gometalinter to golangci-lint as mentioned in #973

Currently, I keep the same configuration as gometalinter (govet, golint, errcheck, deadcode, missspel). gofmt, unused, goimports linters are added. The goal is to migrate to golangci-lint with minimal code changes.

Any improvement can be tackled based on discussion.

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, and examples directory)
  • Manually tested

@sayboras sayboras force-pushed the feature/golangci-lint branch 3 times, most recently from a8a63f3 to 33a79b6 Compare October 12, 2019 11:42
@errordeveloper
Copy link
Contributor

Thanks @sayboras! I'd like to merge #1200 first.

@sayboras
Copy link
Contributor Author

@errordeveloper sure, I will rebase later

@sayboras sayboras force-pushed the feature/golangci-lint branch 6 times, most recently from ee7f8a7 to bba8b38 Compare October 18, 2019 09:48
@sayboras sayboras force-pushed the feature/golangci-lint branch 2 times, most recently from 682bde0 to 6baf011 Compare November 9, 2019 09:13
.golangci.yml Show resolved Hide resolved
Copy link
Contributor

@martina-if martina-if left a comment

Choose a reason for hiding this comment

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

Hi @sayboras , thank you so much for this PR! This is great! I added some feedback :)

.gitignore Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
pkg/eks/mocks/CloudTrailAPI.go Show resolved Hide resolved
pkg/eks/mocks/IAMAPI.go Show resolved Hide resolved
@sayboras sayboras force-pushed the feature/golangci-lint branch from 5db8b9b to 1c982ef Compare November 12, 2019 21:30
@sayboras sayboras requested a review from martina-if November 12, 2019 22:17
@sayboras sayboras force-pushed the feature/golangci-lint branch 2 times, most recently from 38adfb5 to 9c9c1e4 Compare November 12, 2019 23:27
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
install-build-deps.sh Outdated Show resolved Hide resolved
@martina-if
Copy link
Contributor

Hi @sayboras , thanks again for this PR. Since it introduces some risks in the build pipeline I would like to hold it until the end of kubecon next week. By the way if you happen to go come and say hi at the Weaveworks booth :)

@sayboras
Copy link
Contributor Author

Hi @sayboras , thanks again for this PR. Since it introduces some risks in the build pipeline I would like to hold it until the end of kubecon next week. By the way if you happen to go come and say hi at the Weaveworks booth :)

No hurry, I can leave it as it is now :).

I wish I could have come for kube con 😢 , hopefully next year

@sayboras sayboras force-pushed the feature/golangci-lint branch from fdbe851 to 38425f4 Compare November 21, 2019 04:55
@sayboras
Copy link
Contributor Author

Let me know if you have any other comments, happy to address.

@martina-if
Copy link
Contributor

Thank you for being so patient and keeping this up to date! :) We will try to merge it next week!

@sayboras sayboras force-pushed the feature/golangci-lint branch 3 times, most recently from 317c921 to daaaa2c Compare December 4, 2019 09:47
@sayboras sayboras force-pushed the feature/golangci-lint branch from a6e56de to f752740 Compare December 20, 2019 12:59
@martina-if
Copy link
Contributor

martina-if commented Jan 8, 2020

@sayboras it seems I can't push to your branch. Can you run this command locally and push the changes? make -f Makefile.docker commit-new-image-tag. This is what we need to do when we change the tooling so that we use a new version of the docker image. I will push the docker image to the docker hub as well. That should make the circleci pipeline happy.

@sayboras
Copy link
Contributor Author

sayboras commented Jan 8, 2020

@sayboras it seems I can't push to your branch. Can you run this command locally and push the changes? make -f Makefile.docker commit-new-image-tag. This is what we need to do when we change the tooling so that we use a new version of the docker image. I will push the docker image to the docker hub as well. That should make the circleci pipeline happy.

I tried to push the new image before, but got the below error

denied: requested access to the resource is denied
make: *** [push-build-image] Error 1

So it might need your help on this last step, I updated the manifest as well.

Copy link
Contributor

@martina-if martina-if left a comment

Choose a reason for hiding this comment

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

I have now pushed the docker image with the tool installed so the build should not fail because of that.

If you check, there are some failures from the generated aws sdk mocks. We should probably make the linter ignore those.

@@ -62,7 +71,7 @@ linters-settings:
check-blank: false
govet:
# report about shadowed variables
check-shadowing: true
check-shadowing: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this one changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this lint is to check shadow declaration of variable. Lot of times we re-declare err by using := operator.

Let me know if you want to fix it, I can make the changes.

time "/Users/tammach/go/bin/golangci-lint" run
pkg/gitops/flux/installer.go:166:5         govet  shadow: declaration of "err" shadows declaration at line 133
pkg/gitops/flux/installer.go:172:6         govet  shadow: declaration of "err" shadows declaration at line 133
pkg/gitops/flux/installer.go:182:5         govet  shadow: declaration of "err" shadows declaration at line 133
pkg/gitops/profile.go:52:5                 govet  shadow: declaration of "err" shadows declaration at line 46
pkg/addons/vpc_controller.go:98:7          govet  shadow: declaration of "err" shadows declaration at line 87
pkg/nodebootstrap/userdata.go:104:5        govet  shadow: declaration of "err" shadows declaration at line 96
pkg/kubernetes/client.go:313:6             govet  shadow: declaration of "err" shadows declaration at line 308
pkg/kubernetes/client.go:357:6             govet  shadow: declaration of "err" shadows declaration at line 351
pkg/kubernetes/client.go:395:6             govet  shadow: declaration of "err" shadows declaration at line 351
pkg/eks/api.go:206:5                       govet  shadow: declaration of "err" shadows declaration at line 196
pkg/ctl/create/cluster.go:112:5            govet  shadow: declaration of "err" shadows declaration at line 94
pkg/ctl/create/cluster.go:116:5            govet  shadow: declaration of "err" shadows declaration at line 94
pkg/ctl/create/cluster.go:134:7            govet  shadow: declaration of "err" shadows declaration at line 94
pkg/ctl/create/cluster.go:375:7            govet  shadow: declaration of "err" shadows declaration at line 346
pkg/ctl/create/cluster.go:381:7            govet  shadow: declaration of "err" shadows declaration at line 346
pkg/ctl/create/cluster.go:384:7            govet  shadow: declaration of "err" shadows declaration at line 346
pkg/ctl/create/fargate.go:65:5             govet  shadow: declaration of "err" shadows declaration at line 61
pkg/ctl/create/fargate.go:69:9             govet  shadow: declaration of "err" shadows declaration at line 61
pkg/ctl/create/fargate.go:81:5             govet  shadow: declaration of "err" shadows declaration at line 61
pkg/ctl/create/iamidentitymapping.go:56:5  govet  shadow: declaration of "err" shadows declaration at line 51
pkg/ctl/create/iamidentitymapping.go:68:5  govet  shadow: declaration of "err" shadows declaration at line 51
pkg/ctl/create/iamidentitymapping.go:76:9  govet  shadow: declaration of "err" shadows declaration at line 51
pkg/ctl/create/iamserviceaccount.go:71:5   govet  shadow: declaration of "err" shadows declaration at line 65
pkg/ctl/create/iamserviceaccount.go:75:9   govet  shadow: declaration of "err" shadows declaration at line 65
pkg/ctl/create/nodegroup.go:86:5           govet  shadow: declaration of "err" shadows declaration at line 80
pkg/ctl/create/nodegroup.go:90:9           govet  shadow: declaration of "err" shadows declaration at line 80
pkg/ctl/create/nodegroup.go:94:5           govet  shadow: declaration of "err" shadows declaration at line 80
pkg/ctl/delete/cluster.go:92:5             govet  shadow: declaration of "err" shadows declaration at line 86
pkg/ctl/delete/cluster.go:97:5             govet  shadow: declaration of "err" shadows declaration at line 86
pkg/ctl/delete/cluster.go:101:9            govet  shadow: declaration of "err" shadows declaration at line 86
pkg/ctl/delete/fargate.go:58:5             govet  shadow: declaration of "err" shadows declaration at line 54
pkg/ctl/delete/iamidentitymapping.go:53:5  govet  shadow: declaration of "err" shadows declaration at line 47
pkg/ctl/delete/iamidentitymapping.go:64:9  govet  shadow: declaration of "err" shadows declaration at line 47
pkg/ctl/delete/iamidentitymapping.go:76:5  govet  shadow: declaration of "err" shadows declaration at line 47
pkg/ctl/delete/iamserviceaccount.go:71:5   govet  shadow: declaration of "err" shadows declaration at line 65
pkg/ctl/delete/nodegroup.go:62:5           govet  shadow: declaration of "err" shadows declaration at line 56
pkg/ctl/delete/nodegroup.go:66:9           govet  shadow: declaration of "err" shadows declaration at line 56
pkg/ctl/drain/nodegroup.go:58:5            govet  shadow: declaration of "err" shadows declaration at line 52
pkg/ctl/drain/nodegroup.go:62:9            govet  shadow: declaration of "err" shadows declaration at line 52
pkg/ctl/enable/profile.go:87:5             govet  shadow: declaration of "err" shadows declaration at line 83
pkg/ctl/enable/repo.go:54:5                govet  shadow: declaration of "err" shadows declaration at line 50
pkg/ctl/enable/utils.go:18:5               govet  shadow: declaration of "err" shadows declaration at line 14
pkg/ctl/get/fargate.go:64:5                govet  shadow: declaration of "err" shadows declaration at line 60
pkg/ctl/get/iamidentitymapping.go:57:5     govet  shadow: declaration of "err" shadows declaration at line 52
pkg/ctl/get/iamserviceaccount.go:63:5      govet  shadow: declaration of "err" shadows declaration at line 58
pkg/ctl/get/iamserviceaccount.go:67:9      govet  shadow: declaration of "err" shadows declaration at line 58
pkg/ctl/get/nodegroup.go:69:5              govet  shadow: declaration of "err" shadows declaration at line 64
pkg/ctl/scale/nodegroup.go:68:5            govet  shadow: declaration of "err" shadows declaration at line 63
pkg/ctl/update/cluster.go:63:5             govet  shadow: declaration of "err" shadows declaration at line 57
pkg/ctl/update/cluster.go:67:9             govet  shadow: declaration of "err" shadows declaration at line 57

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I wonder how this was working before 🤔 If there is a way to only allow shadowing err and ctx that would be nice. Otherwise let's leave it like that for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no way that I am aware of. I think i just leave it for now :(

.golangci.yml Outdated Show resolved Hide resolved
@sayboras sayboras force-pushed the feature/golangci-lint branch from 5b25726 to d0a2357 Compare January 9, 2020 08:52
@sayboras sayboras force-pushed the feature/golangci-lint branch from d0a2357 to 7996a57 Compare January 9, 2020 08:59
@martina-if martina-if merged commit cfc50ff into eksctl-io:master Jan 9, 2020
@sayboras sayboras deleted the feature/golangci-lint branch January 9, 2020 09:35
@martina-if martina-if self-assigned this Jan 9, 2020
@marccarre marccarre added this to the 0.13.0 milestone Jan 20, 2020
@martina-if martina-if mentioned this pull request Jan 20, 2020
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.

4 participants