Enable golint on the code base

[corylanou]

Description

We want to enable golint on our codebase.

First, get golint:

go get -u github.com/golang/lint

Then run it on any package you want to lint:

# will lint entire project
golint ./...

# will lint single package in project
golint ./meta

Also, you can use this script to get an output of how many lint errors are in each package, and which packages are currently passing (thanks @wind0r):

#!/bin/bash

#All Packages
PACKAGES=$(find . -name '*.go' -print0 | xargs -0 -n1 dirname | sort --unique)

echo 'packages needing linting'
for pkg in ${PACKAGES[@]}; do
    echo $pkg $(golint $pkg | wc -l) | grep -v '\s0$'
done

echo ''
echo 'packages passing lint'
for pkg in ${PACKAGES[@]}; do
    echo $pkg $(golint $pkg | wc -l) | grep '\s0$'
done

We want to do this for several reasons:

• We want to improve code quality
• We need objective filters on quality to help us discriminate bad pull requests

How to

Following is the list of all github.com/influxdb/influxdb subpackages. For each we need to:

• Submit a PR cleaning all existing golint warnings. One thing to keep in mind is that golint will require that every exported symbol has a comment: favor making symbols private where possible before documenting.
• Enable golint on this package as part of the Circle CI jobs to make sure we don't diverge (something that individual contributors can do, but @otoolep who owns the testing infrastructure will help with that).

We also need to make it easy for contributors to easily give golint a local run before submitting the PR. We should probably have that script only cover a hardcoded list of packages that we can augment over time.

Packages:

• [root] Complete lint of project root. #4340 thanks @skanjo
• client ref #4098 lint client package #4439 thanks @skanjo
• cluster Changes to make the cluster sub package golint-able #4266 thanks @lenko-d
• cluster/internal
• cmd/influx Complete lint for cmd/influx #4703 thanks @pablolmiranda
• cmd/influx_stress Linted cmd/ packages. Refs #4098 #5822 thanks @gouthamve
• cmd/influxd Linted cmd/ packages. Refs #4098 #5822 thanks @gouthamve
• cmd/influxd/backup Linted cmd/ packages. Refs #4098 #5822 thanks @gouthamve
• cmd/influxd/help Linted cmd/ packages. Refs #4098 #5822 thanks @gouthamve
• cmd/influxd/restore Linted cmd/ packages. Refs #4098 #5822 thanks @gouthamve
• cmd/influxd/run Linted cmd/ packages. Refs #4098 #5822 thanks @gouthamve
• cmd/inspect
• importer/v8
• influxql
• meta
• meta/internal
• monitor
• pkg/slices
• services/admin
• services/collectd
• services/collectd/test_client
• services/continuous_querier
• services/copier
• services/copier/internal
• services/graphite
• services/hh
• services/httpd
• services/opentsdb
• services/precreator
• services/retention
• services/snapshotter
• services/udp
• snapshot
• statik
• tcp
• tests/urlgen
• toml
• tsdb
• tsdb/engine
• tsdb/engine/b1
• tsdb/engine/bz1
• tsdb/engine/wal
• tsdb/internal
• uuid Changes to make the uuid subpakage golint-able. #4173 thanks @lenko-d

This PR is modeled after Dockers initiative to do the same with their project: moby/moby#14756

Updated List October, 2016

• influxdb
• influxdb/client
• influxdb/client/v2
• influxdb/cmd/influx
• influxdb/cmd/influx/cli
• influxdb/cmd/influx_inspect
• influxdb/cmd/influx_stress
• influxdb/cmd/influx_tsm
• influxdb/cmd/influx_tsm/b1
• influxdb/cmd/influx_tsm/bz1
• influxdb/cmd/influx_tsm/stats
• influxdb/cmd/influx_tsm/tsdb
• influxdb/cmd/influx_tsm/tsdb/internal
• influxdb/cmd/influxd
• influxdb/cmd/influxd/backup
• influxdb/cmd/influxd/help
• influxdb/cmd/influxd/restore
• influxdb/cmd/influxd/run
• influxdb/coordinator
• influxdb/importer/v8
• influxdb/influxql
• influxdb/influxql/internal
• influxdb/influxql/neldermead
• influxdb/models
• influxdb/monitor
• influxdb/monitor/diagnostics
• influxdb/pkg/deep
• influxdb/pkg/escape
• influxdb/pkg/limiter
• influxdb/pkg/slices
• influxdb/services/admin
• influxdb/services/admin/statik
• influxdb/services/collectd
• influxdb/services/collectd/test_client
• influxdb/services/continuous_querier
• influxdb/services/graphite
• influxdb/services/httpd
• influxdb/services/meta
• influxdb/services/meta/internal
• influxdb/services/opentsdb
• influxdb/services/precreator
• influxdb/services/retention
• influxdb/services/snapshotter
• influxdb/services/subscriber
• influxdb/services/udp
• influxdb/stress
• influxdb/stress/stress_test_server
• influxdb/stress/v2
• influxdb/stress/v2/statement
• influxdb/stress/v2/stress_client
• influxdb/stress/v2/stressql
• influxdb/stress/v2/stressql/statement
• influxdb/tcp
• influxdb/tests/urlgen
• influxdb/toml
• influxdb/tsdb
• influxdb/tsdb/engine
• influxdb/tsdb/engine/tsm1
• influxdb/tsdb/internal
• influxdb/uuid

[sparrc]

one problem is that golint explicitly states that the tool shouldn’t be used in automated build processes: https://github.com/golang/lint#purpose

"do not expect or require code to be completely "lint-free". In short, this tool is not, and will never be,
trustworthy enough for its suggestions to be enforced automatically, for example as part of a build
process."

So there is no ability to specify ANY ignores for exceptions to the rules, like there is with other tools like pylint. And they reject PRs by people trying to change that...

you could always just enforce that the number of lint errors is less than the number of exceptions with a wc -l, I was doing something like that with go fmt in telegraf

[skanjo]

Just to get things going I will start with root and client.

[skanjo]

So far it looks like root, uuid, and cluster are merged. @corylanou could you update the todo list?

[corylanou]

#4173 is still open, but I updated the other ones. Thanks!

[lenko-d]

Please let me know if I should make more changes to #4173 or close it? Thanks.

[lenko-d]

I will start working on influxql and influxd

[skanjo]

@corylanou client is merged.

[skanjo]

Starting work on cluster/internal.

[wind0r]

I run a little script on the codebase to count the warnings.
(Not all folders. Only that one that are ask above)

cluster/internal 28
cmd/influx 13
cmd/influx_stress 0
cmd/influxd 2
cmd/influxd/backup 0
cmd/influxd/help 0
cmd/influxd/restore 1
cmd/influxd/run 4
cmd/influx_inspect 5
importer/v8 0
influxql 28
meta 15
meta/internal 368
monitor 9
pkg/slices 3
services/admin 2
services/collectd 1
services/collectd/test_client 0
services/continuous_querier 7
services/copier 0
services/copier/internal 10
services/graphite 12
services/hh 11
services/httpd 7
services/opentsdb 3
services/precreator 1
services/retention 2
services/snapshotter 0
services/udp 7
snapshot 0
statik 0
tcp 0
tests/urlgen 3
toml 0
tsdb 100
tsdb/engine 1
tsdb/engine/b1 5
tsdb/engine/bz1 6
tsdb/engine/wal 19
tsdb/internal 24

So following packages can be marked as done and enabled in ci

cmd/influx_stress 0
cmd/influxd/backup 0
cmd/influxd/help 0
importer/v8 0
services/collectd/test_client 0
services/copier 0
services/snapshotter 0
snapshot 0
statik 0
tcp 0
toml 0

[corylanou]

@wind0r can you share a gist of the script then we can use it here as well and all stay on the same page. Thanks!

[wind0r]

Sure. Need to be executed in the influxdb/influxdb folder.
Not the best but i works ;)
Combine with " | grep ' 0' " to get the finished ones.

[corylanou]

Awesome, thanks!

[orthogonous]

@corylanou services/udp is done and passes. ref commit is 434d060

[shaybix]

I wanted to spend this weekend a couple of hours and make a few of these packages lintable.

Regarding influxdb/services/graphite package. In the parser.go file the exported function NewTemplate returns a pointer to an unexported type *graphite.template. To make it also testable it is idiomatic to return an exported interface if not an exported struct, then the returned satisfying struct of that interface can be unexported. That would make the linter pass for that specific line. What you think?

[xjerod]

@corylanou I was looking at tackling influxdb/services/httpd it looks like all of the currently exported Structs/functions without comments are only used within the httpd package. Would you prefer them to be changed to private? It should be pretty straight forward to go either way I just wanted to see if there was a preference.

[corylanou]

@jrbury I'm no longer with Influx but hopefully someone else can give you direction on the issue from the team.

[xjerod]

@corylanou Thanks! Maybe @e-dard would know and/or point me towards who would

[e-dard]

@jrbury can you give me some example struct/methods you're talking about?

[xjerod]

For example in httpd/requests.go there is

type RequestInfo struct {
	IPAddr   string
	Username string
}

type RequestStats struct {
	Writes  int64 `json:"writes"`
	Queries int64 `json:"queries"`
}

Currently, neither of these structs are accessed outside of the httpd package, so I could make them private, but if they were designed with the intent to be able to access them, then I'll just comment them.

[jsternberg]

I think before we continue working on this, we need a way to make sure that packages that have been made golintable stay golinted. That usually means the build has to fail.

So I would say if you want to start somewhere, updating the build system so that it runs golint on a subset of directories that have been golinted already is the first thing that has to be done before we bother accepting more PR's for this issue. Without that, this issue will have no end since we will just continue writing code and forgetting to run golint to check it.

[liukun4515]

Can we get the latest state? I see that the updating is about 10 months ago.
@corylanou

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically closed because it has not had recent activity. Please reopen if this issue is still important to you. Thank you for your contributions.