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.

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: clean up doc strings and some code around Dial vs NewClient #7029

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Mar 7, 2024

Also added a test and removed an unused parameter from a function and call sites. (The reason it was needed in the past is to auto-detect the gRPCLB LB policy, which is functionality we no longer support.)

RELEASE NOTES: none

@dfawley dfawley added the Type: Documentation Documentation or examples label Mar 7, 2024
@dfawley dfawley added this to the 1.63 Release milestone Mar 7, 2024
@dfawley dfawley requested a review from arvindbr8 March 7, 2024 23:50
Copy link

codecov bot commented Mar 7, 2024

Codecov Report

Merging #7029 (f60efc1) into master (c808322) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7029      +/-   ##
==========================================
- Coverage   82.34%   82.33%   -0.02%     
==========================================
  Files         296      296              
  Lines       31471    31475       +4     
==========================================
  Hits        25914    25914              
- Misses       4491     4493       +2     
- Partials     1066     1068       +2     
Files Coverage Δ
clientconn.go 90.82% <100.00%> (-0.68%) ⬇️
dialoptions.go 89.07% <100.00%> (+0.18%) ⬆️

... and 14 files with indirect coverage changes

Copy link
Member

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor nit

@@ -632,7 +632,7 @@ func withHealthCheckFunc(f internal.HealthChecker) DialOption {
})
}

func defaultDialOptions(defScheme string) dialOptions {
func defaultDialOptions() dialOptions {
Copy link
Member

Choose a reason for hiding this comment

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

nit: There is only one usage of defaultDialOptions() which is in NewClient. How do you feel about putting this inline for more readability?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't make this, but I'd prefer to keep it like this. Having a separate function next to the struct like this makes it easy to find the default values. 🤷

@arvindbr8 arvindbr8 assigned dfawley and unassigned arvindbr8 Mar 8, 2024
@dfawley dfawley merged commit 7c37770 into grpc:master Mar 8, 2024
14 checks passed
@dfawley dfawley deleted the newclientdoc branch March 8, 2024 00:26
winkingturtle-vmw added a commit to cloudfoundry/locket that referenced this pull request Apr 10, 2024
- [X] Read the [Contributing document](../blob/-/.github/CONTRIBUTING.md).

Summary
---------------
Context: Context: grpc/grpc-go#7029

Wait for status changed and return when connection is connected. Since
DialContext has been removed, it's a bit unclear what needed to happen
for connection to be ready for use. This code will bring back
partial implementation of DialContext so that connection is ready.

Backward Compatibility
---------------
Breaking Change? **No**
geofffranks pushed a commit to cloudfoundry/locket that referenced this pull request Apr 10, 2024
- [X] Read the [Contributing document](../blob/-/.github/CONTRIBUTING.md).

Summary
---------------
Context: Context: grpc/grpc-go#7029

Wait for status changed and return when connection is connected. Since
DialContext has been removed, it's a bit unclear what needed to happen
for connection to be ready for use. This code will bring back
partial implementation of DialContext so that connection is ready.

Backward Compatibility
---------------
Breaking Change? **No**
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Documentation Documentation or examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants