Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 7 commits into from

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Feb 11, 2019

Backport for the 18.03 branch of;

git checkout -b 18.03_backport_context upstream/bump_v18.03

# https://github.com/docker/swarmkit/pull/2745 Adding a Dockerfile and making it easy to use it for dev
git cherry-pick -s -S -x f8c048cd8c00d3530bbdd87e160c6d0a6e30ac42


# https://github.com/docker/swarmkit/pull/2750 Use gometalinter; switch from x/net/context -> context
git cherry-pick -s -S -x 318574db9d8b5953f966b7f7e0e8803fb4de03b4

git cherry-pick -s -S -x b25f50fdda5b4999e8c37d5e54725cd6734911eb
# some conflicts; see below

git cherry-pick -s -S -x 04ae7e3c35c50f0d28c27b79ccbb45db2c28f780
git cherry-pick -s -S -x 278edc28b8c4330496c01f39b72c32a054af766d
git cherry-pick -s -S -x 0e8bb705bab9c06e98dc1f23e8a5be123b002a8b
# some conflicts; see below

# https://github.com/docker/swarmkit/pull/2811 Fix make check
git cherry-pick -s -S -x 3bfc201ae805c67d604b45c2c4f48350f5873bae

conflicts in b25f50f (all trivial, in import statements);

Unmerged paths:
  (use "git add <file>..." to mark resolution)

	both modified:   agent/agent.go
	both modified:   agent/session.go
	both modified:   agent/worker.go
	both modified:   agent/worker_test.go
	both modified:   ca/certificates_test.go
	both modified:   ca/server_test.go
	both modified:   log/grpc.go
	both modified:   manager/controlapi/cluster_test.go
	both modified:   manager/controlapi/config_test.go
	both modified:   manager/controlapi/network_test.go
	both modified:   manager/controlapi/node_test.go
	both modified:   manager/controlapi/secret_test.go
	both modified:   manager/controlapi/service_test.go
	both modified:   manager/controlapi/task_test.go
	both modified:   node/node.go

conflict in (due to defaultAddrPools not being in 18.03);

diff --cc manager/manager.go
index 040f7f9f,08b158db..00000000
--- a/manager/manager.go
+++ b/manager/manager.go
@@@ -908,7 -994,20 +908,24 @@@ func (m *Manager) becomeLeader(ctx cont
        // shutdown underlying manager processes when leadership is
        // lost.
  
++<<<<<<< HEAD
 +      m.allocator, err = allocator.New(s, m.config.PluginGetter)
++=======
+       // If DefaultAddrPool is null, Read from store and check if
+       // DefaultAddrPool info is stored in cluster object
+       if m.config.NetworkConfig == nil || m.config.NetworkConfig.DefaultAddrPool == nil {
+               var cluster *api.Cluster
+               s.View(func(tx store.ReadTx) {
+                       cluster = store.GetCluster(tx, clusterID)
+               })
+               if cluster.DefaultAddressPool != nil {
+                       m.config.NetworkConfig.DefaultAddrPool = append(m.config.NetworkConfig.DefaultAddrPool, cluster.DefaultAddressPool...)
+                       m.config.NetworkConfig.SubnetSize = cluster.SubnetSize
+               }
+       }
+ 
+       m.allocator, err = allocator.New(s, m.config.PluginGetter, m.config.NetworkConfig)
++>>>>>>> 0e8bb705... gometalinter: add gosimple
        if err != nil {
                log.G(ctx).WithError(err).Error("failed to create allocator")
                // TODO(stevvooe): It doesn't seem correct here to fail

@dperny
Copy link
Collaborator

dperny commented Feb 12, 2019

looks to be big failing on CI

wk8 and others added 7 commits February 12, 2019 21:23
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 <jer329@cornell.edu>
(cherry picked from commit f8c048c)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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>
(cherry picked from commit 318574d)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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>
(cherry picked from commit b25f50f)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
...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>
(cherry picked from commit 04ae7e3)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
... 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>
(cherry picked from commit 278edc2)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
... 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>
(cherry picked from commit 0e8bb70)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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 <drew.erny@docker.com>
(cherry picked from commit 3bfc201)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

hm.. looks like it's failing because 18.03 uses an older version of gRPC, which is still using x/net/context; https://github.com/docker/swarmkit/blob/c23aa65fc646081600eb206c2f2dcff5a9e25143/vendor/google.golang.org/grpc/credentials/credentials.go#L49

@thaJeztah
Copy link
Member Author

So, that would mean we'd have to backport #2649 🤔

@thaJeztah
Copy link
Member Author

closing for now, in favour of #2877, which includes all the CI / linting fixes, but does not make the switch to context

@thaJeztah thaJeztah closed this Aug 9, 2019
@thaJeztah thaJeztah deleted the 18.03_backport_context branch August 9, 2019 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants