Influxdb: 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:

• [x] [root] #4340 thanks @skanjo
• [x] client #4439 thanks @skanjo
• [x] cluster #4266 thanks @lenko-d
• [ ] cluster/internal
• [X] cmd/influx #4703 thanks @pablolmiranda
• [x] cmd/influx_stress #5822 thanks @Gouthamve
• [x] cmd/influxd #5822 thanks @Gouthamve
• [x] cmd/influxd/backup #5822 thanks @Gouthamve
• [x] cmd/influxd/help #5822 thanks @Gouthamve
• [x] cmd/influxd/restore #5822 thanks @Gouthamve
• [x] cmd/influxd/run #5822 thanks @Gouthamve
• [ ] cmd/inspect
• [ ] importer/v8
• [ ] influxql
• [ ] meta
• [x] 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
• [x] tsdb/engine
• [ ] tsdb/engine/b1
• [ ] tsdb/engine/bz1
• [ ] tsdb/engine/wal
• [ ] tsdb/internal
• [x] uuid #4173 thanks @lenko-d

This PR is modeled after Dockers initiative to do the same with their project: https://github.com/docker/docker/issues/14756

Updated List October, 2016

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

[orthogonous]

@corylanou
PR #4835 makes golint pass on these dirs

services/collectd
services/precreator
services/subscriber

[pires]

cmd/influxd is done.

[gabelev]

@corylanou: PR #5366 enables golint for influxql.

[gabelev]

Starting tsdb/internal, but top level comment says not to edit? But am going to tackle it anyway.

[gabelev]

@corylanou PR #5418 for enabling golint in tsdb/internal (sorry for the duplicate pull request, pushed the wrong branch).

[gabelev]

Took care of tsdb/engine/wal.go and wal_test.go in PR#5550.

[nussjustin]

The list of packages should be updated (as some packages were added and some removed). Current status:

• [ ] [root]
• [x] client
• [x] client/v2
• [ ] cluster
• [ ] cluster/internal
• [x] cmd/influx
• [x] cmd/influx/cli
• [x] cmd/influxd
• [x] cmd/influxd/backup
• [x] cmd/influxd/help
• [ ] cmd/influxd/restore
• [ ] cmd/influxd/run
• [x] cmd/influx_inspect
• [x] cmd/influx_stress
• [ ] cmd/influx_tsm
• [ ] cmd/influx_tsm/b1
• [ ] cmd/influx_tsm/bz1
• [ ] cmd/influx_tsm/tsdb
• [ ] cmd/influx_tsm/tsdb/internal
• [x] importer/v8
• [ ] influxql
• [ ] influxql/internal
• [ ] models
• [ ] monitor
• [ ] monitor/diagnostics
• [x] pkg/deep
• [ ] pkg/escape
• [x] pkg/slices
• [x] services/admin
• [x] services/admin/statik
• [ ] services/collectd
• [x] services/collectd/test_client
• [ ] services/continuous_querier
• [x] services/copier
• [ ] services/copier/internal
• [ ] services/graphite
• [ ] services/hh
• [x] services/httpd
• [ ] services/meta
• [ ] services/meta/internal
• [x] services/opentsdb
• [x] services/precreator
• [x] services/retention
• [ ] services/snapshotter
• [x] services/subscriber
• [x] services/udp
• [ ] stress
• [x] stress/stress_test_server
• [x] tcp
• [ ] tests/urlgen
• [x] toml
• [ ] tsdb
• [x] tsdb/engine
• [ ] tsdb/engine/tsm1
• [ ] tsdb/internal
• [x] uuid

Generated with:

#!/bin/bash                                                                                                                                                                                                        

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

for pkg in ${PACKAGES[@]}; do
    if [ "$pkg" == "." ]; then
        name="[root]"
    else
        name=${pkg:2}
    fi  

    if [ $(golint $pkg | wc -l) -eq 0 ]; then
        echo "- [x] $name"
    else
        echo "- [ ] $name"
    fi  
done

[gabelev]

I'm tackling the rest of tsdb/engine. It looks like tsdb/internal should be taken off the list as the meta.pb.go generated protobuf file should not be edited:
https://github.com/influxdata/influxdb/pull/5418

[gouthamve]

Hi @nuss-justin, I was looking at the possible things to take up and I think you can check these off:

1. client/v2 - golint passes
2. cluster/internal - generated file, not to be edited
3. cmd/influx_tsm/tsdb/internal - generated file, not to be edited
4. influxql/internal - generated file, not to be edited
5. services/copier/internal - generated file, not to be edited
6. services/meta/internal - generated file, not to be edited
7. tsdb/internal - generated file, not to be edited

I am currently tackling cmd/ packages. Might be able to get a PR in 2 days.

[gouthamve]

@corylanou running lint on services/continuous_querier is returning: don't use an underscore in package name

Now, while I can change this and any package importing it, I think it will break someone else's code if they are importing it. How should I go forward?

[kkirsche]

@corylanou it seems as though package importer/v8 is fine per golint and can be marked as complete

[sdtsui]

Updated list (including influx_tsm changes:

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

[morganzellers]

Going to work on influxdb/stress, should be able to make a PR today

[clebs]

Hi,
I ran the linter on influxdb/influxql and there is some work there to be done which I would like to tackle if it is fine.

[corylanou]

@clebs Sounds good. Let me know if you have any questions. Happy to help. Thanks!

[clebs]

Thanks for the quick response @corylanou!
In fact I do have one question :)
iterator_mapper.go has types and an interface that are exported but not documented, so the linter complains.
Nevertheless, they are only used inside the same package influxql. So my idea was to unexport them.
I can't do that because iterator_mapper_test.go has its own package influxql_test.

Is there a reason for that? Shouldn't it also have package influxql?

Thanks!

[corylanou]

@jsternberg do you know the answer? Is it a matter of updating the generator (I think those files are generated, but didn't look). Thanks.

[e-dard]

@clebs @corylanou influxql_test exists so that the exported API of the influxql is tested. It may or may not be possible to unexport types in influxql as we (InfluxData) may have other projects/tools depending on them.

@clebs one other piece of advice — if the file ends in .gen.go then you will need to lint the .gen.go.tmpl file and re-run go generate. 👍

[clebs]

Hi @e-dard and thanks for the tips!
I see, so the reason is to test the public API only... I am then wondering if those types really need to be tested from an external perspective. Right now they look to me like internal works on how the QL gets and processes queried data.

I will give it a thought and either document the exported types or come back to seek consensus on unexporting them and adapting the tests.

[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?

[jrbury]

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

[jrbury]

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

[jrbury]

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[bot]]

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[bot]]

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.