Use gometalinter; switch from x/net/context -> context #2750

kolyshkin · 2018-09-18T10:08:05Z

This PR contains a few related changes. The following is a copy-paste from commit messages. Hope it makes sense.

I. Makefile: use gometalinter

Instead of running many small source code checkers and linters one by
one, let's use gometalinter that runs them all in parallel.

While at it, remove the individual make targets (fmt, vet, lint,
ineffassign, and misspell) and replace with a single check target.

One thing it provides is faster source validation.

BEFORE:

real    0m24.025s
user    1m2.646s
sys     0m3.860s

(note these timings are without building binaries, which
for some reason was a dependency of the vet target)

AFTER:

real    0m6.330s
user    0m20.255s
sys     0m1.019s

In addition to this, it is now way easier to add/remove the checks,
as well as to filter out some errors from linters that we consider
false positives.

II. Switch from x/net/context -> context

Since Go 1.7, context is a standard package. Since Go 1.9, everything
that is provided by "x/net/context" is a couple of type aliases to
types in "context".
While at it, fix the order of imports (done by goimports and some shell
trickery, see below).
Also, when the standard context package is used, the errors of
not calling cancel() are detected/reported by go vet:

the cancel function returned by context.WithTimeout should be called,
not discarded, to avoid a context leak (vet)

This essentially asks to call cancel() as otherwise a stray timer
is leaked. Fix a few such issues, mostly in _test.go files.

Since in func (*session).start (see agent/session.go) we deliberately
do not want to cancel the context, govet gives us a couple of errors:

agent/session.go:140: the cancelSession function is not used
on all paths (possible context leak)
agent/session.go:163: this return statement may be reached
without using the cancelSession var defined on line 140

To silence these, use // nolint: vet mark in a couple of places
(this is a feature of gometalinter).

Oh, the conversion (items 1 and 2 above) was performed by this
shell script:

FILES=$*
test -z "$FILES" && FILES=$(git ls-files \*.go | grep -v ^vendor/ | grep -v .pb.go$)

for f in $FILES; do
        sed -i 's|"golang.org/x/net/context"|"context"|' $f
        goimports -w $f
        for i in 1 2; do
                awk '/^$/ {e=1; next;}
                        /\t"context"$/ {e=0;}
                        {if (e) {print ""; e=0}; print;}' < $f > $f.new && mv $f.new $f
                goimports -w $f
        done
        echo -n .
done
echo

Multiple goimports calls and some awk trickery is needed to iron out
incorrect formatting (excessive empty lines) from the import statements.

III. Add some more linters and fix warnings reported by those

Those are:

deadcode
goimports
unconvert
gosimple

For the warnings reported and fixed, please see individual commit messages.

kolyshkin · 2018-09-18T17:53:22Z

Please note patches 3 to 5 are optional -- please let me know if you want them to either be removed or separated out to another PR.

codecov · 2018-09-18T18:00:50Z

Codecov Report

Merging #2750 into master will increase coverage by 0.21%.
The diff coverage is 58.33%.

@@            Coverage Diff             @@
##           master    #2750      +/-   ##
==========================================
+ Coverage   61.66%   61.87%   +0.21%     
==========================================
  Files         134      134              
  Lines       21890    21857      -33     
==========================================
+ Hits        13499    13525      +26     
+ Misses       6932     6868      -64     
- Partials     1459     1464       +5

kolyshkin · 2018-09-18T18:01:56Z

@dperny PTAL

kolyshkin · 2018-09-18T18:48:00Z

Similar PRs:

kolyshkin · 2018-10-02T00:00:34Z

Rebased; merge conflicts (due to PR #2748) resolved.

@thaJeztah @yongtang PTAL

kolyshkin · 2018-10-02T00:15:04Z

I realize this looks messy like there are two independent features in one PR. Let me explain.

The first patch (introducing gometalinter) is needed to use gometalinter's functionality of silencing some selected warnings (ones we are sure are bogus).

The second patch (switch to standard context package) reveals some warnings about leaked contexts that the code fails to cancel(). Some of them are fixed in the patch, some are silenced as we are quite sure we do not want to cancel those contexts.

The rest of the series is more linters for gometalinter.

Now, if this is too much to digest at once, I can split this into two, three, or more PRs:

Use gometalinter.
Switch to standard context pkg.
Add more linters.

PRs 2 and 3 would depend on 1.

Please let me know how to proceed.

kolyshkin · 2018-10-02T00:42:47Z

CI failure:

protoc --decode google.protobuf.FileDescriptorSet /usr/local/include/google/protobuf/descriptor.proto
/usr/local/include/google/protobuf/descriptor.proto: File does not reside within any path specified using --proto_path (or -I). You must specify a --proto_path which encompasses this file. Note that the proto_path must be an exact prefix of the .proto file names -- protoc is too dumb to figure out when two paths (e.g. absolute and relative) are equivalent (it's harder than you think).

Mea culpa. It's because of containerd/protobuild#20; let me try to fix it

kolyshkin · 2018-10-04T16:56:46Z

Fix is submitted (containerd/protobuild#21) but not yet merged.

wk8

Thanks for the clean up :)

wk8 · 2018-10-04T17:01:56Z

agent/errors.go

-	errTaskStatusUpdateNoChange = errors.New("agent: no change in task status")
-	errTaskUnknown              = errors.New("agent: task unknown")
-
-	errTaskInvalid = errors.New("task: invalid")


All of these were unused?

Yes, as listed in the changelog message from the commit:

agent/errors.go:7:1 errTaskNoController is unused (deadcode)
agent/errors.go:7:1 errTaskStatusUpdateNoChange is unused (deadcode)
agent/errors.go:7:1 errTaskNotAssigned is unused (deadcode)
agent/errors.go:7:1 errTaskInvalid is unused (deadcode)

Note that these are private to the package so if it compiles those can safely be removed

Sorry, had missed that.

Instead of running many small source code checkers and linters one by one, let's use gometalinter that runs them all in parallel. While at it, remove the individual make targets (fmt, vet, lint, ineffassign, and misspell) and replace with a single check target. One thing it provides is faster source validation. BEFORE: real 0m24.025s user 1m2.646s sys 0m3.860s (note these timings are without building binaries, which for some reason was a dependency of the vet target) AFTER: real 0m6.330s user 0m20.255s sys 0m1.019s In addition to this, it is now way easier to add/remove the checks, as well as to filter out some errors from linters that we consider false positives. [v2: add 2m deadline] [v3: use gometalinter --install; move configuration to .json] Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

1. Since Go 1.7, context is a standard package. Since Go 1.9, everything that is provided by "x/net/context" is a couple of type aliases to types in "context". 2. While at it, fix the order of imports (done by goimports and some shell trickery, see below). 3. Also, when the standard context package is used, the errors of not calling cancel() are detected/reported by go vet: > the cancel function returned by context.WithTimeout should be called, > not discarded, to avoid a context leak (vet) This essentially asks to call cancel() as otherwise a stray timer is leaked. Fix a few such issues, mostly in _test.go files. 4. Since in func (*session).start (see agent/session.go) we deliberately do not want to cancel the context, govet gives us a couple of errors: > agent/session.go:140: the cancelSession function is not used > on all paths (possible context leak) > agent/session.go:163: this return statement may be reached > without using the cancelSession var defined on line 140 To silence these, use `// nolint: vet` mark in a couple of places (this is a feature of gometalinter). Oh, the conversion (items 1 and 2 above) was performed by this shell script: ```sh FILES=$* test -z "$FILES" && FILES=$(git ls-files \*.go | grep -v ^vendor/ | grep -v .pb.go$) for f in $FILES; do sed -i 's|"golang.org/x/net/context"|"context"|' $f goimports -w $f for i in 1 2; do awk '/^$/ {e=1; next;} /\t"context"$/ {e=0;} {if (e) {print ""; e=0}; print;}' < $f > $f.new && mv $f.new $f goimports -w $f done echo -n . done echo ``` Multiple `goimports` calls and some awk trickery is needed to iron out incorrect formatting (excessive empty lines) from the import statements. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

...and fix the following initial bunch of warnings: agent/session.go:18:1:warning: errSessionDisconnect is unused (deadcode) agent/errors.go:7:1:warning: errTaskNoController is unused (deadcode) agent/errors.go:7:1:warning: errTaskStatusUpdateNoChange is unused (deadcode) agent/errors.go:7:1:warning: errTaskNotAssigned is unused (deadcode) agent/errors.go:7:1:warning: errTaskInvalid is unused (deadcode) ca/transport.go:21:1:warning: timeoutError is unused (deadcode) ca/config.go:29:1:warning: nodeCSRFilename is unused (deadcode) cmd/swarmctl/node/common.go:58:1:warning: changeNodeMembership is unused (deadcode) integration/cluster.go:44:1:warning: newTestCluster is unused (deadcode) manager/orchestrator/global/global.go:590:1:warning: isTaskCompleted is unused (deadcode) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

... and fix some warnings from unconvert: ca/config.go:627:58:warning: unnecessary conversion (unconvert) manager/allocator/cnmallocator/portallocator.go:410:38:warning: unnecessary conversion (unconvert) manager/allocator/cnmallocator/portallocator.go:415:50:warning: unnecessary conversion (unconvert) manager/controlapi/service.go:200:47:warning: unnecessary conversion (unconvert) manager/controlapi/service.go:210:45:warning: unnecessary conversion (unconvert) manager/controlapi/service.go:220:35:warning: unnecessary conversion (unconvert) manager/dispatcher/nodes.go:159:33:warning: unnecessary conversion (unconvert) manager/state/store/memory.go:692:91:warning: unnecessary conversion (unconvert) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

... and fix warnings reported by it: agent/exec/dockerapi/adapter.go:147:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple) agent/testutils/fakes.go:143:2:warning: should use a simple channel send/receive instead of select with a single case (S1000) (gosimple) agent/testutils/fakes.go:151:2:warning: should use a simple channel send/receive instead of select with a single case (S1000) (gosimple) ca/certificates_test.go:708:2:warning: should use for range instead of for { select {} } (S1000) (gosimple) ca/config.go:630:12:warning: should use time.Until instead of t.Sub(time.Now()) (S1024) (gosimple) ca/config_test.go:790:3:warning: should use a simple channel send/receive instead of select with a single case (S1000) (gosimple) ca/external_test.go:116:3:warning: should use a simple channel send/receive instead of select with a single case (S1000) (gosimple) cmd/swarm-bench/collector.go:26:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple) cmd/swarmctl/node/common.go:51:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple) cmd/swarmctl/node/common.go:89:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple) cmd/swarmctl/node/common.go:172:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple) ioutils/ioutils_test.go:28:5:warning: should use !bytes.Equal(actual, expected) instead (S1004) (gosimple) manager/allocator/cnmallocator/networkallocator.go:818:3:warning: should merge variable declaration with assignment on next line (S1021) (gosimple) manager/constraint/constraint.go:59:7:warning: should omit comparison to bool constant, can be simplified to !matched (S1002) (gosimple) manager/constraint/constraint.go:67:7:warning: should omit comparison to bool constant, can be simplified to !matched (S1002) (gosimple) manager/dispatcher/dispatcher.go:1095:5:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple) manager/dispatcher/dispatcher.go:1095:5:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple) manager/dispatcher/dispatcher.go:1095:5:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple) manager/dispatcher/dispatcher_test.go:2090:2:warning: redundant return statement (S1023) (gosimple) manager/manager.go:1005:4:warning: should replace loop with m.config.NetworkConfig.DefaultAddrPool = append(m.config.NetworkConfig.DefaultAddrPool, cluster.DefaultAddressPool...) (S1011) (gosimple) manager/metrics/collector.go:191:2:warning: redundant return statement (S1023) (gosimple) manager/metrics/collector.go:222:2:warning: redundant return statement (S1023) (gosimple) manager/orchestrator/replicated/update_test.go:53:3:warning: should use for range instead of for { select {} } (S1000) (gosimple) manager/orchestrator/taskinit/init.go:83:32:warning: should use time.Until instead of t.Sub(time.Now()) (S1024) (gosimple) manager/state/raft/raft.go:1185:2:warning: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (S1008) (gosimple) manager/state/raft/raft.go:1594:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple) node/node.go:1209:2:warning: redundant return statement (S1023) (gosimple) node/node.go:1219:2:warning: redundant return statement (S1023) (gosimple) watch/sinks_test.go:42:2:warning: should merge variable declaration with assignment on next line (S1021) (gosimple) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

yongtang

LGTM

thaJeztah

LGTM

dperny · 2018-10-08T15:51:05Z

alright, ship it.

Use gometalinter; switch from x/net/context -> context Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

kolyshkin force-pushed the context branch 2 times, most recently from ebf6fdd to 75ee3d8 Compare September 18, 2018 17:49

kolyshkin force-pushed the context branch from 75ee3d8 to cc8aac1 Compare October 1, 2018 23:58

This was referenced Oct 4, 2018

Dockerfile, circleci: update protoc to 3.6.1, simplify install #2756

Merged

Fix running protoc containerd/protobuild#21

Closed

wk8 approved these changes Oct 4, 2018

View reviewed changes

kolyshkin added 5 commits October 4, 2018 16:53

kolyshkin force-pushed the context branch from cc8aac1 to 0e8bb70 Compare October 4, 2018 23:54

yongtang approved these changes Oct 8, 2018

View reviewed changes

thaJeztah approved these changes Oct 8, 2018

View reviewed changes

dperny merged commit 486ad69 into moby:master Oct 8, 2018

kolyshkin deleted the context branch October 8, 2018 16:55

wk8 mentioned this pull request Oct 9, 2018

Adding a new Deallocator component #2759

Merged

olljanat pushed a commit to olljanat/swarmkit that referenced this pull request Oct 10, 2018

Merge pull request moby#2750 from kolyshkin/context

dce6ce4

Use gometalinter; switch from x/net/context -> context Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

This was referenced Oct 11, 2018

Bump swarmkit to 7d5d33b97794440462a04acdd3f31914da8f66f3 moby/moby#38024

Merged

WIP [18.09 backport] Bump swarmkit to a84c01f docker-archive/engine#89

Closed

andrewhsu mentioned this pull request Oct 19, 2018

[bump_v18.09] Fix nil pointer dereference in node allocation #2768

Closed

anshulpundir mentioned this pull request Oct 19, 2018

[bump_v18.09] Fix nil pointer dereference in node allocation, linter fixes #2769

Merged

This was referenced Feb 11, 2019

[18.03 backport] Use gometalinter; switch from x/net/context -> context #2827

Closed

[18.03 backport] use a custom grpc dialer when managers are joining #2828

Merged

thaJeztah mentioned this pull request Aug 8, 2019

[18.03 backport] makefile and linting changes, bump go 1.11 #2877

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use gometalinter; switch from x/net/context -> context #2750

Use gometalinter; switch from x/net/context -> context #2750

kolyshkin commented Sep 18, 2018 •

edited

Loading

kolyshkin commented Sep 18, 2018

codecov bot commented Sep 18, 2018 •

edited

Loading

kolyshkin commented Sep 18, 2018

kolyshkin commented Sep 18, 2018

kolyshkin commented Oct 2, 2018

kolyshkin commented Oct 2, 2018

kolyshkin commented Oct 2, 2018

kolyshkin commented Oct 4, 2018

wk8 left a comment

wk8 Oct 4, 2018

kolyshkin Oct 4, 2018

kolyshkin Oct 4, 2018

wk8 Oct 4, 2018

yongtang left a comment

thaJeztah left a comment

dperny commented Oct 8, 2018

Use gometalinter; switch from x/net/context -> context #2750

Use gometalinter; switch from x/net/context -> context #2750

Conversation

kolyshkin commented Sep 18, 2018 • edited Loading

I. Makefile: use gometalinter

II. Switch from x/net/context -> context

III. Add some more linters and fix warnings reported by those

kolyshkin commented Sep 18, 2018

codecov bot commented Sep 18, 2018 • edited Loading

Codecov Report

kolyshkin commented Sep 18, 2018

kolyshkin commented Sep 18, 2018

kolyshkin commented Oct 2, 2018

kolyshkin commented Oct 2, 2018

kolyshkin commented Oct 2, 2018

kolyshkin commented Oct 4, 2018

wk8 left a comment

Choose a reason for hiding this comment

wk8 Oct 4, 2018

Choose a reason for hiding this comment

kolyshkin Oct 4, 2018

Choose a reason for hiding this comment

kolyshkin Oct 4, 2018

Choose a reason for hiding this comment

wk8 Oct 4, 2018

Choose a reason for hiding this comment

yongtang left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

dperny commented Oct 8, 2018

kolyshkin commented Sep 18, 2018 •

edited

Loading

codecov bot commented Sep 18, 2018 •

edited

Loading