[18.03 backport] makefile and linting changes, bump go 1.11 #2877
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Credit goes to dperny (moby#2687) **- What I did** Adds a Dockerfile for the swarmkit project, to easily get off the ground. Modifies the Makefile to make intelligent use of Docker. Also made small clean up changes to the Makefile. **- How I did it** Modifies the Makefile to have two paths: containerized.mk, which builds the docker image and forwards any make targets to a container, and direct.mk, which encompasses the old Makefile's workflow. By default, nothing will run inside a container. Set the environment variable `DOCKER_SWARMKIT_USE_CONTAINER` to use dockerized making. Also leverages docker-sync for synchronizing code to the container if the `DOCKER_SWARMKIT_USE_DOCKER_SYNC` env variable is set; comes in handy on Macs, for example. **- How to test it** Set `DOCKER_SWARMKIT_USE_CONTAINER` and verify that your favorite make targets all work! Signed-off-by: Jean Rouge <[email protected]> (cherry picked from commit f8c048c) Signed-off-by: Sebastiaan van Stijn <[email protected]>
Looks like it was used to get faster incremental builds. Nowdays (since Go 1.10) there is no need to use it, because go build cache is used [1]. This fixes `make binaries` on my system where golang is installed as read-only snap: > $ make binaries > 🐳 bin/swarmd > go build runtime/cgo: open /snap/go/2635/pkg/linux_amd64/runtime/cgo.a: read-only file system > direct.mk:100: recipe for target 'bin/swarmd' failed [1] https://groups.google.com/forum/#!msg/golang-dev/qfa3mHN4ZPA/X2UzjNV1BAAJ Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit cb50952) Signed-off-by: Sebastiaan van Stijn <[email protected]>
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 <[email protected]> (cherry picked from commit 318574d) Signed-off-by: Sebastiaan van Stijn <[email protected]>
...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 <[email protected]> (cherry picked from commit 04ae7e3) Signed-off-by: Sebastiaan van Stijn <[email protected]>
... 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 <[email protected]> (cherry picked from commit 278edc2) Signed-off-by: Sebastiaan van Stijn <[email protected]>
... 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 <[email protected]>
(cherry picked from commit 0e8bb70)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
That's basically just a light wrapper around delve, to make it nicer to use (stops it after the session is done, and doesn't ignore interrupts like the vanilla version does). Also amending the containerized Makefile to add the right run options when using delve. That's controlled by the `DOCKER_SWARMKIT_USE_DELVE` env variable; it's also possible to override the port delve uses via the `DOCKER_SWARMKIT_DELVE_PORT` env variable. Signed-off-by: Jean Rouge <[email protected]> (cherry picked from commit 0a88f50) Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
changed the title
thaJeztah
force-pushed
the
18.03_backport_golangci_lint
branch
2 times, most recently
from
e22e1c8 to
720f77a
Compare
Codecov Report
@@ Coverage Diff @@
## bump_v18.03 #2877 +/- ##
===============================================
- Coverage 61.57% 61.55% -0.02%
===============================================
Files 134 134
Lines 21834 21802 -32
===============================================
- Hits 13444 13420 -24
+ Misses 6947 6940 -7
+ Partials 1443 1442 -1 |
thaJeztah
changed the title
gometalinter dropped support for gosimple, which is deprecated anyway and has been subsumed by staticcheck. This commit removes gosimple from our list of enabled linters (as it's no longer valid). It does not enable staticcheck, because staticcheck throws too many errors. Signed-off-by: Drew Erny <[email protected]> (cherry picked from commit 3bfc201) Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
18.03_backport_golangci_lint
branch
from
720f77a to
17450c2
Compare
Signed-off-by: Sebastiaan van Stijn <[email protected]>
``` agent/exec/containerd/adapter.go:313:26: unnecessary conversion (unconvert) timeout = time.Duration(10 * time.Second) ``` Signed-off-by: Sebastiaan van Stijn <[email protected]>
gometalinter is deprecated, and golangci-lint is its recommended successor. This commit adds golangci-lint as the linter for swarmkit. In addition, golangci-lint found a few issues in the code that were not yet identified, and so those issues have been fixed. Signed-off-by: Drew Erny <[email protected]> (cherry picked from commit 27c2d27) Signed-off-by: Sebastiaan van Stijn <[email protected]>
Whitespace changes are caused by the fact that gofmt from go-1.11 uses a different heuristic as to how to format the file, making the source code that was OK for go-1.10 causing a warning with go-1.11. NOTE this whitespace change makes the gofmt from go-1.10 complain, so please upgrade your golang. [v2: regen pb files] Signed-off-by: Kir Kolyshkin <[email protected]> (cherry picked from commit fd2d7f2) Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
18.03_backport_golangci_lint
branch
from
17450c2 to
d0282d0
Compare
This was referenced Aug 9, 2019
Member
Author
|
@dperny @kolyshkin PTAL |
Contributor
|
Seems it's looking better now! |
Member
Author
|
I skipped the We can still do that change at some point if we have some time (definitely not critical, just can make backporting somewhat easier) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See if we can get CI to pass with some of the linting back ports and Makefile improvements
Backports of
contextchange (because some dependencies are too old)hack/debugscript #2766 Adding ahack/debugscript