Skip to content

Replace deprecated grpc.Errorf() with status.Errorf()#2440

Merged
stevvooe merged 1 commit intomoby:masterfrom
thaJeztah:replace-deprecated-errorf
Nov 23, 2017
Merged

Replace deprecated grpc.Errorf() with status.Errorf()#2440
stevvooe merged 1 commit intomoby:masterfrom
thaJeztah:replace-deprecated-errorf

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Nov 11, 2017

from the function's description; https://github.com/docker/swarmkit/blob/c23aa65fc646081600eb206c2f2dcff5a9e25143/vendor/google.golang.org/grpc/rpc_util.go#L393-L399

// Errorf returns an error containing an error code and a description;
// Errorf returns nil if c is OK.
//
// Deprecated; use status.Errorf instead.
func Errorf(c codes.Code, format string, a ...interface{}) error {
	return status.Errorf(c, format, a...)
}

thaJeztah
thaJeztah commented Nov 11, 2017
View reviewed changes
api/api.pb.txt
type: TYPE_STRING
json_name: "swiftPrefix"
}
field {
Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Nov 11, 2017

Choose a reason for hiding this comment

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

interesting, I see these were removed in a previous commit; 8f9f7dc#diff-832df52f65be6ede52a44115a7cfe9e5L714

different version used to generate?

Copy link
Copy Markdown
Contributor

@stevvooe stevvooe Nov 16, 2017

Choose a reason for hiding this comment

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

Rebase? I think this fix was merged.

Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Nov 17, 2017

Choose a reason for hiding this comment

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

Yes that fix was, but.. #2451 😅

@thaJeztah thaJeztah force-pushed the replace-deprecated-errorf branch from c0c17c4 to b0861c0 Compare November 15, 2017 16:13
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 15, 2017 •
edited
Loading

Codecov Report

Merging #2440 into master will increase coverage by 2.87%.
The diff coverage is 53.73%.

@@            Coverage Diff            @@
##           master   #2440      +/-   ##
=========================================
+ Coverage   57.73%   60.6%   +2.87%     
=========================================
  Files          10     128     +118     
  Lines        1552   26412   +24860     
=========================================
+ Hits          896   16007   +15111     
- Misses        588    9000    +8412     
- Partials       68    1405    +1337

@thaJeztah thaJeztah force-pushed the replace-deprecated-errorf branch from b0861c0 to 5faebed Compare November 15, 2017 16:49
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Nov 15, 2017

Is this a flaky test?

time="2017-11-15T16:59:00Z" level=warning msg="no certificate expiration specified, using default" cluster.id=tetpm9gd2z13j0h6gso5zfbjw method="(*Server).UpdateRootCA" module=ca testHasExternalCA=false 
--- FAIL: TestCanRemoveMember (23.41s)
	Error Trace:	cluster_test.go:353
	Error:		Received unexpected error context deadline exceeded
		

	Error Trace:	cluster_test.go:355
	Error:		Expected nil, but got: &api.RaftMember{RaftID:0x43c623d82926a70d, NodeID:"3a5piy1yjkrq1x46ec2ewxak9", Addr:"127.0.0.1:47892", Status:api.RaftMemberStatus{Leader:false, Reachability:1, Message:""}}
		

	Error Trace:	cluster_test.go:356
	Error:		Not equal: 3 (expected)
			        != 2 (actual)
		

FAIL

@anshulpundir
Copy link
Copy Markdown
Contributor

anshulpundir commented Nov 16, 2017

Is this a flaky test?

Seems like it. See #2445

@stevvooe
Copy link
Copy Markdown
Contributor

stevvooe commented Nov 16, 2017

LGTM

@thaJeztah
Replace deprecated grpc.Errorf() with status.Errorf()
553f145 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the replace-deprecated-errorf branch from 5faebed to 553f145 Compare November 21, 2017 20:40
@stevvooe stevvooe merged commit 3b24e5a into moby:master Nov 23, 2017
@thaJeztah thaJeztah deleted the replace-deprecated-errorf branch November 23, 2017 00:30
@andrewhsu andrewhsu mentioned this pull request Dec 5, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@stevvooe stevvooe stevvooe approved these changes

@anshulpundir anshulpundir anshulpundir approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@thaJeztah @anshulpundir @stevvooe