golangci-lint workflow

[uturunku1]

Description

This PR is meant to address an Asana ticket that requests to add a lint library using Github Actions that can be trigger every time someone opens a PR against go-tfe.

After creating the github action workflow, golangci-lint displayed its errors here: https://github.com/hashicorp/go-tfe/pull/262/checks?check_run_id=3681121701

I ignored the recommendations related to "unused" code coming from helper_test.go because the unused library pulled by Golangci-lint does not see that those test functions are being used in other integrations tests within the same package.

This issue was reported last year but it sounds like that golangci-lint may not cover all edge cases around that external unused library: golangci/golangci-lint#791

It seemed like the simple solution would be to configure our golangci.yml to ignore the unused library for test files.

Documentation on Golangci-lint: https://golangci-lint.run/usage/configuration/

[hashicorp-cla]

All committers have signed the CLA.

[omarismail]

Are we sure this works as intended? I am running golint locally and caught a number of issues:

go-tfe % golint
admin_run.go:33:6: exported type AdminRun should have comment or be unexported
admin_setting_saml.go:83:1: receiver name a should be consistent with previous receiver name s for adminSAMLSettings
admin_setting_saml.go:100:1: receiver name a should be consistent with previous receiver name s for adminSAMLSettings
admin_workspace.go:32:6: exported type AdminVCSRepo should have comment or be unexported
admin_workspace.go:36:1: comment on exported type AdminWorkspace should be of the form "AdminWorkspace ..." (with optional leading article)
helper_test.go:564:9: if block ends with a return statement, so drop this else and outdent its block
ip_ranges.go:14:1: comment on exported type IPRanges should be of the form "IPRanges ..." (with optional leading article)
ip_ranges.go:29:6: exported type IPRange should have comment or be unexported
policy_set_version.go:68:6: exported type PolicySetVersion should have comment or be unexported
policy_set_version.go:88:32: error strings should not be capitalized or end with punctuation or a newline
policy_set_version.go:92:32: error strings should not be capitalized or end with punctuation or a newline
registry_module.go:129:6: exported type RegistryModulePermissions should have comment or be unexported
registry_module.go:135:6: exported type RegistryModuleVersionStatuses should have comment or be unexported
registry_module.go:273:6: exported type RegistryModuleVCSRepoOptions should have comment or be unexported
state_version_output.go:12:1: comment on exported type StateVersionOutputs should be of the form "StateVersionOutputs ..." (with optional leading article)
state_version_output.go:25:6: exported type StateVersionOutput should have comment or be unexported
tag.go:3:6: exported type TagList should have comment or be unexported
tfe.go:534:2: var jsonApiFields should be jsonAPIFields
tfe.go:555:9: if block ends with a return statement, so drop this else and outdent its block
workspace.go:155:6: exported type WorkspaceOutputs should have comment or be unexported
workspace.go:344:1: comment on exported type VCSRepoOptions should be of the form "VCSRepoOptions ..." (with optional leading article)
workspace.go:881:6: exported type RemoteStateConsumersListOptions should have comment or be unexported
workspace.go:1012:6: exported type WorkspaceTagListOptions should have comment or be unexported
workspace.go:1042:6: exported type WorkspaceAddTagsOptions should have comment or be unexported
workspace.go:1077:6: exported type WorkspaceRemoveTagsOptions should have comment or be unexported

[uturunku1]

@omarismail Hey Omar! About this #262 (comment) , are you running a different lint library locally or did you alias golangci-lint run to be golint? I have implemented Golangci-lint for the GH action workflow and I am also running it locally without any new errors after adding that yml configuration: https://golangci-lint.run/usage/install/#local-installation

[uturunku1]

I was able to surface most of the errors that you'd see with golint and staticcheck with this additional configuration:

go-tfe/.golangci.yml

Lines 4 to 8 in cabf350

	enable:
	- gosimple
	- stylecheck
	- revive #golint is deprecated and golangci-lint recommends to use revive instead
	#other deprecated lint libraries: maligned, scopelint, interfacer

I fixed the issues reported by those libraries locally, so in the latest golangci-lint github workflow you will not see any errors in the output. But you can take a look at the previous workflow run to check for the errors I fixed: https://github.com/hashicorp/go-tfe/actions/runs/1284596686
Let me know your thoughts! @omarismail

[omarismail]

Hmm, the docs say that this gosimple plus others are enabled by default. Part of that list includes errcheck. But when I run errcheck -verbose ./... locally, i get

go-tfe % errcheck -verbose ./...
checking github.com/hashicorp/go-tfe
checking github.com/hashicorp/go-tfe/examples/workspaces
checking github.com/hashicorp/go-tfe
checking github.com/hashicorp/go-tfe.test
checking github.com/hashicorp/go-tfe/examples/organizations
ip_ranges.go:83:23:     (io.Closer).Close       defer resp.Body.Close()
logreader.go:76:23:     (io.Closer).Close       defer resp.Body.Close()
tfe.go:410:17:  (io.Closer).Close       resp.Body.Close()
tfe.go:594:23:  (io.Closer).Close       defer resp.Body.Close()

It doesn't seem like these are enabled by default then? I think we should add all those that are supposed to be enabled by default explicitly here. So

linters:
  enable:
   - deadcode
   - errcheck
   - gosimple
   - govet
   - ineffassign
   - staticcheck
   - structcheck
   - typecheck
   - unused
   - varcheck

[uturunku1]

@omarismail Ok, I think I know what is going on with errcheck and other "enabled by default" lint libraries. They are enabled, yes, except some of their features are disabled by default. So in the case of errcheck we need to do additional configuration on the .golangci.yml to get all those errors that you see when you errcheck independently. The additional configuration would look something like this:

linters-settings:
  errcheck:
    # report about not checking of errors in type assertions: `a := b.(MyStruct)`;
    # default is false: such cases aren't reported by default.
    check-type-assertions: true

And now when we run golangci-lint run, we get:

tfe.go:361:14: Error return value of `strconv.ParseFloat` is not checked (errcheck)
			if reset, _ := strconv.ParseFloat(v, 64); reset > 0 {
			          ^
tfe.go:426:17: Error return value of `strconv.ParseFloat` is not checked (errcheck)
		if rateLimit, _ := strconv.ParseFloat(v, 64); rateLimit > 0 {

Errors, yay!

[omarismail]

lgtm once tests pass