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

resolver: update Resolver.Scheme() docstring to mention requirement of lowercase scheme names #6014

Merged
merged 6 commits into from Feb 15, 2023

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Feb 10, 2023

Also test the case-senstiveness of the checks.

Fixes #6011.

RELEASE NOTES:

  • resolver: update Resolver.Scheme() docstring to mention requirement of lowercase scheme names

Also recommend Scheme() to return no uppercase letters, as RFC3986 requires
parsing to convert the scheme to lowercase

Fixes grpc#6011.
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, other than the panic in the test.

@arvindbr8 arvindbr8 assigned dfawley and unassigned arvindbr8 Feb 10, 2023
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.

looks good!

resolver_test.go Outdated
@@ -88,10 +89,15 @@ func (s) TestResolverCaseSensitivity(t *testing.T) {
// globally-registered resolver. Since it is "dns" instead of passthrough,
// we can validate by checking for an address that has been resolved
// (i.e. is not "localhost:port").
cc, err = Dial(target, WithContextDialer(customDialer), WithResolvers(res), WithTransportCredentials(insecure.NewCredentials()))

// WithDisableServiceConfig disables TXT lookups, which can hang for
Copy link
Member

Choose a reason for hiding this comment

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

👍

@arvindbr8
Copy link
Member

oh wait, seems like the tests are still failing with a similar pattern

@arvindbr8
Copy link
Member

all tests are green now

@dfawley dfawley assigned arvindbr8 and unassigned dfawley Feb 14, 2023
@dfawley dfawley changed the title resolver: update registry to enforce case-insensitive lookups resolver: update Resolver.Scheme() docstring to mention requirement of lowercase scheme names Feb 14, 2023
@dfawley dfawley added Type: Documentation Documentation or examples and removed Type: Bug labels Feb 14, 2023
@dfawley
Copy link
Member Author

dfawley commented Feb 14, 2023

PTAL after the changes as discussed earlier today.

@dfawley dfawley merged commit 6d612a3 into grpc:master Feb 15, 2023
1 check passed
@dfawley dfawley deleted the rescase branch May 3, 2023 20:51
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 31, 2023
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.

dialing against registered schemes is not case-insensitive
2 participants