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.

Sign up for GitHub

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

Jump to bottom

client: upgrade github.com/ugorji/go to v1.1.2 #10481

Merged
merged 2 commits into from
Feb 28, 2019
Merged

client: upgrade github.com/ugorji/go to v1.1.2 #10481

merged 2 commits into from
Feb 28, 2019

Conversation

sysulq
Copy link
Contributor

@sysulq sysulq commented Feb 16, 2019 •
edited by hexfusion
Loading

#10477

this PR fixes panic when updating github.com/ugorji/go to v1.1.2

maintainer note: also bumping github.com/urfave/cli to v1.20.0

@codecov-io
Copy link

codecov-io commented Feb 16, 2019 •
edited
Loading

Codecov Report

Merging #10481 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10481      +/-   ##
==========================================
+ Coverage    71.5%   71.53%   +0.03%     
==========================================
  Files         392      392              
  Lines       36518    36518              
==========================================
+ Hits        26111    26123      +12     
+ Misses       8576     8573       -3     
+ Partials     1831     1822       -9
Impacted Files Coverage Δ
pkg/adt/interval_tree.go 81.38% <0%> (-5.41%) ⬇️
proxy/grpcproxy/watcher.go 85.71% <0%> (-4.09%) ⬇️
proxy/grpcproxy/watch.go 88.55% <0%> (-3.02%) ⬇️
raft/progress.go 94.17% <0%> (-1.95%) ⬇️
etcdserver/api/rafthttp/msgappv2_codec.go 69.56% <0%> (-1.74%) ⬇️
mvcc/watchable_store.go 84.21% <0%> (-1.41%) ⬇️
etcdctl/ctlv3/command/printer_simple.go 72.48% <0%> (-1.35%) ⬇️
clientv3/watch.go 91.73% <0%> (-1.06%) ⬇️
clientv3/leasing/kv.go 89.03% <0%> (-0.67%) ⬇️
etcdserver/api/rafthttp/stream.go 79.39% <0%> (-0.43%) ⬇️
... and 14 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 5effa15...6543273. Read the comment docs.

hexfusion
hexfusion reviewed Feb 16, 2019
View reviewed changes
client/keys.generated.go Outdated
@@ -1819,20 +1915,25 @@ func (x *Node) CodecEncodeSelf(e *codec1978.Encoder) {
}
}
r.WriteMapStart(yynn2)
yynn2 = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line needs to be manually removed yynn2 = 0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that will resolve this https://travis-ci.com/etcd-io/etcd/jobs/178290492#L586

@hexfusion
Copy link
Contributor

Also please update you commit message per https://github.com/etcd-io/etcd/blob/master/CONTRIBUTING.md#format-of-the-commit-message

maybe vendor: upgrade github.com/ugorji/go to v1.1.2

@sysulq sysulq changed the title upgrade github.com/ugorji/go to v1.1.2 vendor: upgrade github.com/ugorji/go to v1.1.2 Feb 16, 2019
@sysulq sysulq changed the title vendor: upgrade github.com/ugorji/go to v1.1.2 client: upgrade github.com/ugorji/go to v1.1.2 Feb 18, 2019
@sysulq
Copy link
Contributor Author

sysulq commented Feb 18, 2019

@hexfusion done :-)

@hexfusion
Copy link
Contributor

@hnlq715 thanks for the changes. I will review this later today/tonight.

@hexfusion
Copy link
Contributor

Generally this looks good ugorji/go is now stating that codec is a module on its own this si why we are now depending on codec directly ugorji/go#279. But I seem to be getting different output when I regenerate client so I want to better understand that. Thanks for you patience we want to make sure this is done right!

hexfusion
hexfusion reviewed Feb 19, 2019
View reviewed changes
go.mod
@@ -42,7 +42,7 @@ require (
github.com/spf13/pflag v1.0.1
github.com/stretchr/testify v1.2.2 // indirect
github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8
github.com/ugorji/go v1.1.1
github.com/ugorji/go/codec v0.0.0-20190204201341-e444a5086c43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For anyone curious this is valid ugorji/go#279

@sysulq
Copy link
Contributor Author

sysulq commented Feb 20, 2019

Conflicts are resolved :-)

@sysulq sysulq mentioned this pull request Feb 20, 2019
Closed
@sysulq
Copy link
Contributor Author

sysulq commented Feb 21, 2019

@hexfusion Any updates? :-)

hexfusion
hexfusion reviewed Feb 23, 2019
View reviewed changes
Copy link
Contributor

@hexfusion hexfusion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hnlq715 thanks for your patience, this looks good rebase against master and we will merged.

@hexfusion
Copy link
Contributor

hexfusion commented Feb 23, 2019 •
edited
Loading

To clarify we want two commits here you currently have 5

vendor: update ugorji to v1.1.2
client: generate new keys manual remove "yynn2 = 0"

let me know if you need help on the rebase

@sysulq sysulq force-pushed the master branch 3 times, most recently from 66a1919 to 4239e08 Compare February 25, 2019 04:01
ihippik added a commit to ihippik/etcd that referenced this pull request Feb 25, 2019
@ihippik
Update keys.generated.go
40f45cf 
etcd-io#10481
@hexfusion
Copy link
Contributor

We are now bumping github.com/urfave/cli to v1.20.0. That wasn't part of the plan but we can use it I added the note above.

hexfusion
hexfusion approved these changes Feb 25, 2019
View reviewed changes
Copy link
Contributor

@hexfusion hexfusion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! @hnlq715 I really appreciate the effort here, nice work.

@sysulq
Copy link
Contributor Author

sysulq commented Feb 26, 2019

Thanks for your help @hexfusion

@hexfusion
Copy link
Contributor

/cc @xiang90

@xiang90
Copy link
Contributor

xiang90 commented Feb 28, 2019

lgtm

@xiang90 xiang90 merged commit 5694f3e into etcd-io:master Feb 28, 2019
@hexfusion hexfusion mentioned this pull request Mar 4, 2019
Closed
@zyt312074545 zyt312074545 mentioned this pull request Mar 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hexfusion hexfusion hexfusion approved these changes

@gyuho gyuho Awaiting requested review from gyuho

@jpbetz jpbetz Awaiting requested review from jpbetz

@xiang90 xiang90 Awaiting requested review from xiang90

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sysulq @codecov-io @hexfusion @xiang90