-
Notifications
You must be signed in to change notification settings - Fork 176
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
bump dependencies #578
bump dependencies #578
Conversation
adrianchiris
commented
Jul 24, 2024
•
edited
Loading
edited
- bump dependencies like in Bump the gomod-dependencies group across 1 directory with 6 updates #577
- fix lint issues by removing the use of grpc.WithBlock() dial option
Pull Request Test Coverage Report for Build 10092515578Details
💛 - Coveralls |
7dae6e4
to
a74e792
Compare
@Eoghan1232 PTAL if you got time :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
pkg/resources/server.go
Outdated
@@ -265,38 +264,14 @@ func (rs *resourceServer) Start() error { | |||
} | |||
pluginapi.RegisterDevicePluginServer(rs.grpcServer, rs) | |||
|
|||
// start serving from grpcServeer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx, some of my keyboard switches are double typing :\
go func() { | ||
err := rs.grpcServer.Serve(lis) | ||
if err != nil { | ||
glog.Errorf("serving incoming requests failed: %s", err.Error()) | ||
} | ||
}() | ||
|
||
conn, err := grpc.NewClient( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't want to wait for the server to start?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we dont. thats an anti-pattern, see commit msg with relevant link.
Signed-off-by: adrianc <adrianc@nvidia.com>
WithBlock() dial option is deprecated, recommendation is to let the caller handle failures with retry see[1] this change is needed to sort out lint issues in ci. [1] https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md#especially-bad-using-deprecated-dialoptions Signed-off-by: adrianc <adrianc@nvidia.com>
a74e792
to
958ed1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM