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

Switch to using golangci-lint instead of go-metalinter #3049

Merged
merged 4 commits into from
Feb 15, 2019

Conversation

paddycarver
Copy link
Contributor

Following in the footsteps of our friends working on the AWS provider in hashicorp/terraform-provider-aws#7457 and prompted by go-metalinter's mainainer's stance, we're switching from go-metalinter to golangci-lint. This PR updates the Travis config, makefile, and lint configs to make this happen.

@ghost ghost added the size/s label Feb 14, 2019
paddycarver added a commit to GoogleCloudPlatform/magic-modules that referenced this pull request Feb 14, 2019
A whole slew of errors reported by golangci-lint are fixed in this PR.
To whit:

* Unchecked errors are now checked.
* When the DELETE URL is overriden, we have a debug log line to get
  around the ineffassign linter's complaints about it. We shoudl
  eventually just only generate the URL we actually need, but this fixes
  the problem for the moment.
* Removed a TODO sanity check override, because the bog that broke it
  was, I believe, fixed years ago.
* Fixed a subtle error shadowing bug in the delete for instance group
  managers.
* Stopped populating the unused "locations" variable in
  container_cluster's Delete method.
* Ignored unused return values instead of populating them.

The effect of these changes is that our linter should pass (once the
linter is updated in tpg and tpgb, see
hashicorp/terraform-provider-google#3049 and
hashicorp/terraform-provider-google-beta#437) and we, as a
bonus, are handling more error cases and fixed a subtle bug.
@paddycarver
Copy link
Contributor Author

Travis failed due to lint issues that it previously didn't catch. I filed GoogleCloudPlatform/magic-modules#1387 to resolve all those issues. When that PR is merged, these lint issues will go away.

@paddycarver
Copy link
Contributor Author

Fixes #3014.

@@ -6,7 +6,7 @@ language: go
go:
- "1.11.x"
env:
- GO111MODULE=off
- GO111MODULE=on
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you see if we're able to safely set GOFLAGS=-mod=vendor as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it's unnecessary, as the linting tool does it automatically?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh not for the linting tool, just because Go will realise we're using vendored modules in that case.

@paddycarver paddycarver merged commit 2149900 into master Feb 15, 2019
paddycarver added a commit to GoogleCloudPlatform/magic-modules that referenced this pull request Feb 15, 2019
A whole slew of errors reported by golangci-lint are fixed in this PR.
To whit:

* Unchecked errors are now checked.
* When the DELETE URL is overriden, we have a debug log line to get
  around the ineffassign linter's complaints about it. We shoudl
  eventually just only generate the URL we actually need, but this fixes
  the problem for the moment.
* Removed a TODO sanity check override, because the bog that broke it
  was, I believe, fixed years ago.
* Fixed a subtle error shadowing bug in the delete for instance group
  managers.
* Stopped populating the unused "locations" variable in
  container_cluster's Delete method.
* Ignored unused return values instead of populating them.

The effect of these changes is that our linter should pass (once the
linter is updated in tpg and tpgb, see
hashicorp/terraform-provider-google#3049 and
hashicorp/terraform-provider-google-beta#437) and we, as a
bonus, are handling more error cases and fixed a subtle bug.
@ghost
Copy link

ghost commented Mar 17, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants