Skip to content

Replace deprecated grpc functions#2645

Merged
dperny merged 2 commits into
moby:masterfrom
thaJeztah:replace-deprecated-grpc
Aug 1, 2018
Merged

Replace deprecated grpc functions#2645
dperny merged 2 commits into
moby:masterfrom
thaJeztah:replace-deprecated-grpc

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

grpc.ErrorDesc(err) and grpc.Code(err) were deprecated in favor of the "status" package in grpc/grpc-go@b507112

Replacing these with their equivalent, in case the deprecated methods are removed in future

@thaJeztah thaJeztah force-pushed the replace-deprecated-grpc branch from b5430b6 to 78191a8 Compare May 23, 2018 19:23
@thaJeztah thaJeztah force-pushed the replace-deprecated-grpc branch from 78191a8 to 2776dca Compare June 1, 2018 15:36
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 1, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@39d5dc4). Click here to learn what that means.
The diff coverage is 57.89%.

@@            Coverage Diff            @@
##             master    #2645   +/-   ##
=========================================
  Coverage          ?   61.83%           
=========================================
  Files             ?      134           
  Lines             ?    21772           
  Branches          ?        0           
=========================================
  Hits              ?    13463           
  Misses            ?     6863           
  Partials          ?     1446

@thaJeztah
Copy link
Copy Markdown
Member Author

@cyli @anshulpundir I fixed the linting issue; this is green now PTAL

Copy link
Copy Markdown
Contributor

@cyli cyli left a comment

Choose a reason for hiding this comment

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

Generally looks good, aside from where we are importing testutils from non-test code. Thanks for keeping us up to date with GRPC.

Comment thread cmd/swarmctl/main.go Outdated
func main() {
if c, err := mainCmd.ExecuteC(); err != nil {
c.Println("Error:", grpc.ErrorDesc(err))
msg := err.Error()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non-blocking: This works fine, but from the GRPC docstring for status.FromError:

FromError returns a Status representing err if it was produced from this package or has a method GRPCStatus() *Status. Otherwise, ok is false and a Status is returned with codes.Unknown and the original error message.

So it sounds like you can probably just do:

s, _ := status.FromError(err)
c.Println(s.Message())

Comment thread manager/state/raft/transport/peer.go Outdated
// Handle errors.
if grpc.Code(err) == codes.NotFound && grpc.ErrorDesc(err) == membership.ErrMemberRemoved.Error() {
s, _ = status.FromError(err)
if s.Code() == codes.NotFound && testutils.ErrorDesc(err) == membership.ErrMemberRemoved.Error() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should probably not be importing testutils from non-test code. Can we just call s.Message()?

thaJeztah added 2 commits July 9, 2018 16:13
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the replace-deprecated-grpc branch from 2776dca to cb6dcf2 Compare July 9, 2018 14:16
@thaJeztah
Copy link
Copy Markdown
Member Author

@cyli thanks for reviewing! I rebased, and addressed your comments; PTAL

Copy link
Copy Markdown
Contributor

@cyli cyli left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for staying on top of the GRPC changes @thaJeztah!

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.

3 participants