-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
grpc: Deprecate WithBlock, WithReturnConnectionError, FailOnNonTempDialError #7097
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #7097 +/- ##
==========================================
+ Coverage 81.28% 81.32% +0.04%
==========================================
Files 345 345
Lines 33925 33925
==========================================
+ Hits 27577 27591 +14
+ Misses 5184 5175 -9
+ Partials 1164 1159 -5
|
I guess Type: Documentation label can be added. |
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.
Thank you for the PR. Two small requests, otherwise LGTM!
dialoptions.go
Outdated
// | ||
// Notice: This API is EXPERIMENTAL and may be changed or removed in a | ||
// later release. |
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.
Could you please remove this experimental warning and we'll just call it supported now ("throughout 1.x")?
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.
PTAL cce3000
dialoptions.go
Outdated
// | ||
// Notice: This API is EXPERIMENTAL and may be changed or removed in a | ||
// later release. |
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.
For this one, I'd say we should delete "experimental", but we probably want to reserve the right to remove it. IIRC this makes its way through multiple layers of our library and might be more painful to maintain long-term.
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.
PTAL cbdf351
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.
Looks great, thank you!
Someone can fix vet failure for me: https://github.com/grpc/grpc-go/actions/runs/8573454634/job/23498177256?pr=7097 Otherwise I will fix it on Monday (I made changes on mobile) |
This is an optional checker, and the current breakage is caused by an upstream change. No need to do anything for this PR. |
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.
Looks great! Thanks for the PR @pellared
Fixes #7095
Notice in
NewClient
GoDoc comment thatFailOnNonTempDialError
dial option is ignored.Deprecate options that are ignored by
NewClient
function.These options are only supported by deprecated
Dial
andDialContext
functions.RELEASE NOTES: