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

*: update to Go 1.12.2 and make related changes #10631

Open
wants to merge 11 commits into
base: master
from

Conversation

6 participants
@spzala
Copy link
Member

commented Apr 11, 2019

Update to Go 1.12.2 testing. Remove deprecated unused and gosimple
pacakges, and mask staticcheck 1006.

Related #10528 #10438

Co-Authored-By: Gyuho Lee leegyuho@amazon.com

@codecov-io

This comment has been minimized.

Copy link

commented Apr 11, 2019

Codecov Report

Merging #10631 into master will decrease coverage by 1.13%.
The diff coverage is 30.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10631      +/-   ##
==========================================
- Coverage   71.51%   70.37%   -1.14%     
==========================================
  Files         394      392       -2     
  Lines       36658    36598      -60     
==========================================
- Hits        26216    25756     -460     
- Misses       8604     8980     +376     
- Partials     1838     1862      +24
Impacted Files Coverage Δ
clientv3/retry.go 87.27% <ø> (ø) ⬆️
clientv3/balancer/balancer.go 84.12% <ø> (-0.13%) ⬇️
client/keys.go 56.78% <ø> (-34.68%) ⬇️
proxy/grpcproxy/lease.go 68.77% <0%> (-10.41%) ⬇️
clientv3/retry_interceptor.go 44.5% <0%> (-23.64%) ⬇️
proxy/grpcproxy/watch.go 66.26% <0%> (-26.51%) ⬇️
clientv3/leasing/cache.go 87.77% <0%> (ø) ⬆️
clientv3/client.go 79.6% <0%> (-0.52%) ⬇️
clientv3/balancer/grpc1.7-health.go 59.01% <0%> (+1.16%) ⬆️
proxy/grpcproxy/adapter/chan_stream.go 45% <0%> (-10.13%) ⬇️
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cca0d5c...44a5c80. Read the comment docs.

@spzala spzala added the WIP label Apr 12, 2019

@spzala

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

@gyuho hi, there are several fmt errors related to staticchecks (existing (e.g. SA2002) and newly added (e.g. S1002)). Should we try fixing them under this PR? Thanks!

@gyuho

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

@spzala Yes, please. We want to fix the CIs. Thanks.

@spzala

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

@gyuho OK, sounds great, thanks! Working on it!

@spzala spzala force-pushed the spzala:goruntimeupdate112 branch 4 times, most recently from 857c2d2 to 5548bc4 Apr 15, 2019

@spzala

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

@gyuho @hexfusion staticchecks errors seems under control but now as you can see there is a unconvert failure in a generated file keys.generated.go. I have tried taking care of unconvert failure in few other places but any thoughts on how should I handle the generated code? Thanks!

@spzala

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

@gyuho @hexfusion staticchecks errors seems under control but now as you can see there is a unconvert failure in a generated file keys.generated.go. I have tried taking care of unconvert failure in few other places but any thoughts on how should I handle the generated code? Thanks!

We will get rid of client/keys.generated.go if we go with #10667

@spzala spzala force-pushed the spzala:goruntimeupdate112 branch 2 times, most recently from 45cf683 to 44a5c80 Apr 25, 2019

@spzala spzala removed the WIP label Apr 25, 2019

@spzala

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

@gyuho @hexfusion the changes are now ready for your initial review. Thanks!

@xiang90

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

@gyuho can you take a look?

@@ -507,7 +507,7 @@ func (b *GRPC17Health) Get(ctx context.Context, opts grpc.BalancerGetOptions) (g
addr = b.pinAddr
b.mu.RUnlock()
if closed {
return grpc.Address{Addr: ""}, nil, grpc.ErrClientConnClosing
return grpc.Address{Addr: ""}, nil, context.Canceled

This comment has been minimized.

Copy link
@xiang90

xiang90 May 1, 2019

Contributor

why do we need to change this error type?

This comment has been minimized.

Copy link
@gyuho

gyuho May 1, 2019

Member

@spzala

use he status code of Canceled instead of grpc.ErrClientConnClosing which is deprecated

Is it deprecated in gRPC? We haven't bumped up the gRPC yet.

This comment has been minimized.

Copy link
@spzala

spzala May 2, 2019

Author Member

@gyuho thanks and yes, the deprecated one. I also found a related issue #9304 so seems like we are good for now and I will make updates here. Thanks both!

@@ -47,7 +47,7 @@ var (
// client-side streaming retry limit, only applied to requests where server responds with
// a error code clearly indicating it was unable to process the request such as codes.Unavailable.
// If set to 0, retry is disabled.
defaultStreamMaxRetries = uint(^uint(0)) // max uint
defaultStreamMaxRetries = uint(0) // max uint

This comment has been minimized.

Copy link
@xiang90

xiang90 May 1, 2019

Contributor

this does not seem to be right. we still want all 1's in the binary representation.

This comment has been minimized.

Copy link
@spzala

spzala May 2, 2019

Author Member

My bad, I will fix it. Thanks @xiang90 for catching it.

gyuho added some commits Feb 1, 2019

Makefile: bump up default Go version for tests
Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
travis.yml: bump up default Go versions for tests
Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
test: remove "gosimple"
Deprecated, in favor of "staticcheck"

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
clientv3: remove unused
Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
test: remove unused
Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
test: skip more static checks
Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
test: change staticcheck code prefix
Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

gyuho and others added some commits Feb 1, 2019

*: update to Go 1.12.2 and make related changes
Update to Go 1.12.2 testing. Remove deprecated unused and gosimple
pacakges, and mask staticcheck 1006.

Related #10528 #10438

Co-Authored-By: Gyuho Lee <leegyuho@amazon.com>
*: fix staticcheck errors
Fix following errors from staticcheck:
- remove redundant return statements
- use for range instead of for select
- use time.Since instead of time.Now().Sub
- omit comparison to bool constant
- replace T.Fatal and T.Fatalf in tests with T.Error and T.Fatalf respectively because the goroutine calls T.Fatal must be called in the same goroutine as the test
- fix error strings that should not be capitalized
- use sort.Strings(...) instead of sort.Sort(sort.StringSlice(...))
- use he status code of Canceled instead of grpc.ErrClientConnClosing which is deprecated
- use use status.Errorf instead of grpc.Errorf which is deprecated
test: fix unconvert errors
Fix unconvert errors related to unnecessary type conversions.

@spzala spzala force-pushed the spzala:goruntimeupdate112 branch from 44a5c80 to 52f9dcc May 2, 2019

@spzala spzala added the WIP label May 2, 2019

@spzala

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Due to the merge of #10640 few things are changed, rebased but running into few conflicts and working on it.

@spzala spzala force-pushed the spzala:goruntimeupdate112 branch from 6697357 to 8b576e8 May 2, 2019

test: changes per review comments
Fix and undo certain changes based on review comments etc.

@spzala spzala force-pushed the spzala:goruntimeupdate112 branch from 8b576e8 to 5a6a16d May 3, 2019

@spzala spzala removed the WIP label May 3, 2019

@spzala

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

@xiang90 @gyuho since 'staticcheck' and 'unconvert' are removed from test under #10640 I believe many of the changes made in this PR to take care related fmt errors are not needed to pass CI but if the changes are harmless I guess it's good to still keep them as a good practice? I have addressed your review comments and also opened #10703 for the future work. Thanks!

@xiang90

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

travis failed with "go: sigs.k8s.io/yaml@v1.1.0: unrecognized import path "sigs.k8s.io/yaml" (https fetch: Get https://sigs.k8s.io/yaml?go-get=1: net/http: TLS handshake timeout)".

it seems to have something to do with 42a7ea6. /cc @rohitsardesai83

@spzala

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

travis failed with "go: sigs.k8s.io/yaml@v1.1.0: unrecognized import path "sigs.k8s.io/yaml" (https fetch: Get https://sigs.k8s.io/yaml?go-get=1: net/http: TLS handshake timeout)".

it seems to have something to do with 42a7ea6. /cc @rohitsardesai83

Thanks @xiang90 and yes, the same failure I noticed with another PR that came in today #10704

@rohitsardesai83

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

travis failed with "go: sigs.k8s.io/yaml@v1.1.0: unrecognized import path "sigs.k8s.io/yaml" (https fetch: Get https://sigs.k8s.io/yaml?go-get=1: net/http: TLS handshake timeout)".

it seems to have something to do with 42a7ea6. /cc @rohitsardesai83

Hi @xiang90 , I cloned a fresh copy of etcd repo and tested with go 1.12.4

root@ubuntu-20:~/go/src/go.etcd.io/etcd# ./test
Running with TEST_CPUS: 1,2,4
Starting 'fmt' pass at Sat May  4 11:21:46 IST 2019
'shellcheck' started at Sat May  4 11:21:46 IST 2019
'shellcheck' completed at Sat May  4 11:21:46 IST 2019
'markdown_you' started at Sat May  4 11:21:46 IST 2019
'markdown_you' completed at Sat May  4 11:21:46 IST 2019
'markdown_marker' started at Sat May  4 11:21:46 IST 2019
Skipping marker...
'markdown_marker' completed at Sat May  4 11:21:46 IST 2019
'goword' started at Sat May  4 11:21:46 IST 2019
Skipping goword...
'goword' completed at Sat May  4 11:21:46 IST 2019
'gofmt' started at Sat May  4 11:21:46 IST 2019
'gofmt' completed at Sat May  4 11:21:48 IST 2019
'govet' started at Sat May  4 11:21:48 IST 2019
go: finding github.com/coreos/go-systemd v0.0.0-20180511133405-39ca1b05acc7
go: finding github.com/modern-go/reflect2 v1.0.1
go: finding github.com/mattn/go-colorable v0.0.9
go: finding github.com/olekukonko/tablewriter v0.0.0-20170122224234-a0225b3f23b5
go: finding github.com/dustin/go-humanize v0.0.0-20171111073723-bb3d318650d4
go: finding github.com/stretchr/testify v1.2.2
go: finding github.com/golang/protobuf v1.2.0
go: finding github.com/soheilhy/cmux v0.1.4
go: finding golang.org/x/crypto v0.0.0-20180608092829-8ac0e0d97ce4
go: finding go.uber.org/zap v1.9.1
go: finding github.com/pmezard/go-difflib v1.0.0
go: finding github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd
go: finding github.com/inconshreveable/mousetrap v1.0.0
go: finding github.com/prometheus/procfs v0.0.0-20180612222113-7d6f385de8be
go: finding sigs.k8s.io/yaml v1.1.0     // this went through fine.
go: finding github.com/bgentry/speakeasy v0.1.0
go: finding github.com/google/uuid v1.0.0
go: finding gopkg.in/airbrake/gobrake.v2 v2.0.9
go: finding github.com/onsi/gomega v1.4.2
go: finding gopkg.in/cheggaaa/pb.v1 v1.0.25
go: finding github.com/grpc-ecosystem/grpc-gateway v1.4.1
go: finding github.com/gogo/protobuf v1.0.0
go: finding go.uber.org/multierr v1.1.0
go: finding github.com/golang/groupcache v0.0.0-20160516000752-02826c3e7903
go: finding github.com/json-iterator/go v1.1.5
go: finding gopkg.in/gemnasium/logrus-airbrake-hook.v2 v2.1.2
go: finding github.com/prometheus/client_model v0.0.0-20170216185247-6f3806018612
go: finding github.com/prometheus/client_golang v0.8.0
go: finding github.com/pkg/errors v0.8.0
go: finding github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2
go: finding github.com/coreos/pkg v0.0.0-20160727233714-3ac0863d7acf
go: finding github.com/jonboulle/clockwork v0.1.0
go: finding github.com/kr/pty v1.0.0
go: finding github.com/onsi/ginkgo v1.6.0
go: finding github.com/google/btree v0.0.0-20180124185431-e89373fe6b4a
go: finding gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7
go: finding go.uber.org/atomic v1.3.2
go: finding gopkg.in/yaml.v2 v2.2.2

I did not face the timeout issues which Travis reported. Not sure what is the cause here .

@spzala

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

travis failed with "go: sigs.k8s.io/yaml@v1.1.0: unrecognized import path "sigs.k8s.io/yaml" (https fetch: Get https://sigs.k8s.io/yaml?go-get=1: net/http: TLS handshake timeout)".
it seems to have something to do with 42a7ea6. /cc @rohitsardesai83

Hi @xiang90 , I cloned a fresh copy of etcd repo and tested with go 1.12.4

root@ubuntu-20:~/go/src/go.etcd.io/etcd# ./test
Running with TEST_CPUS: 1,2,4
Starting 'fmt' pass at Sat May  4 11:21:46 IST 2019
'shellcheck' started at Sat May  4 11:21:46 IST 2019
'shellcheck' completed at Sat May  4 11:21:46 IST 2019
'markdown_you' started at Sat May  4 11:21:46 IST 2019
'markdown_you' completed at Sat May  4 11:21:46 IST 2019
'markdown_marker' started at Sat May  4 11:21:46 IST 2019
Skipping marker...
'markdown_marker' completed at Sat May  4 11:21:46 IST 2019
'goword' started at Sat May  4 11:21:46 IST 2019
Skipping goword...
'goword' completed at Sat May  4 11:21:46 IST 2019
'gofmt' started at Sat May  4 11:21:46 IST 2019
'gofmt' completed at Sat May  4 11:21:48 IST 2019
'govet' started at Sat May  4 11:21:48 IST 2019
go: finding github.com/coreos/go-systemd v0.0.0-20180511133405-39ca1b05acc7
go: finding github.com/modern-go/reflect2 v1.0.1
go: finding github.com/mattn/go-colorable v0.0.9
go: finding github.com/olekukonko/tablewriter v0.0.0-20170122224234-a0225b3f23b5
go: finding github.com/dustin/go-humanize v0.0.0-20171111073723-bb3d318650d4
go: finding github.com/stretchr/testify v1.2.2
go: finding github.com/golang/protobuf v1.2.0
go: finding github.com/soheilhy/cmux v0.1.4
go: finding golang.org/x/crypto v0.0.0-20180608092829-8ac0e0d97ce4
go: finding go.uber.org/zap v1.9.1
go: finding github.com/pmezard/go-difflib v1.0.0
go: finding github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd
go: finding github.com/inconshreveable/mousetrap v1.0.0
go: finding github.com/prometheus/procfs v0.0.0-20180612222113-7d6f385de8be
go: finding sigs.k8s.io/yaml v1.1.0     // this went through fine.
go: finding github.com/bgentry/speakeasy v0.1.0
go: finding github.com/google/uuid v1.0.0
go: finding gopkg.in/airbrake/gobrake.v2 v2.0.9
go: finding github.com/onsi/gomega v1.4.2
go: finding gopkg.in/cheggaaa/pb.v1 v1.0.25
go: finding github.com/grpc-ecosystem/grpc-gateway v1.4.1
go: finding github.com/gogo/protobuf v1.0.0
go: finding go.uber.org/multierr v1.1.0
go: finding github.com/golang/groupcache v0.0.0-20160516000752-02826c3e7903
go: finding github.com/json-iterator/go v1.1.5
go: finding gopkg.in/gemnasium/logrus-airbrake-hook.v2 v2.1.2
go: finding github.com/prometheus/client_model v0.0.0-20170216185247-6f3806018612
go: finding github.com/prometheus/client_golang v0.8.0
go: finding github.com/pkg/errors v0.8.0
go: finding github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2
go: finding github.com/coreos/pkg v0.0.0-20160727233714-3ac0863d7acf
go: finding github.com/jonboulle/clockwork v0.1.0
go: finding github.com/kr/pty v1.0.0
go: finding github.com/onsi/ginkgo v1.6.0
go: finding github.com/google/btree v0.0.0-20180124185431-e89373fe6b4a
go: finding gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7
go: finding go.uber.org/atomic v1.3.2
go: finding gopkg.in/yaml.v2 v2.2.2

I did not face the timeout issues which Travis reported. Not sure what is the cause here .

@rohitsardesai83 thanks for testing! And, probably not but I was wondering if it's related to go version as CI is not yet on 1.12.4 and the error seems across the board with new PRs (e.g. https://travis-ci.com/etcd-io/etcd/jobs/197642542#L630 is another PR)

@rohitsardesai83

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

@spzala , in another instance , for this PR #10693 , the CI passed : https://travis-ci.com/etcd-io/etcd/jobs/197674921#L634. the dependency got downloaded correctly.

@jingyih

This comment has been minimized.

Copy link
Member

commented May 6, 2019

I am new to go mod. Why do we need to download dependencies when we already have vendor folder?

@spzala

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

@spzala , in another instance , for this PR #10693 , the CI passed : https://travis-ci.com/etcd-io/etcd/jobs/197674921#L634. the dependency got downloaded correctly.

@rohitsardesai83 thanks, and thanks @jingyih for debugging it further!

@xiang90

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

@spzala Can you try to retrigger the travis CI?

@jingyih

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Feels like maybe some of the go module flags are not configured properly. When building, dependencies code should be found in vendor folder, instead of downloading from a remote repo.

@spzala

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

@spzala Can you try to retrigger the travis CI?

@xiang90 sure, thanks, just restarted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.