resolver: delete Target.Scheme and Target.Authority#6363
resolver: delete Target.Scheme and Target.Authority#6363ginayeh merged 2 commits intogrpc:masterfrom
Conversation
|
FYI the vet checks are failing because you didn't run gofmt. You'll most likely want to set up your IDE to do that automatically on-save. LMK if you need help with that. |
|
Thanks for the hint. I've fixed that in my IDE. Seems like it's passing vet now. |
| // parseTarget uses RFC 3986 semantics to parse the given target into a | ||
| // resolver.Target struct containing scheme, authority and url. Query | ||
| // params are stripped from the endpoint. | ||
| // resolver.Target struct containing url. Query params are stripped from the endpoint. |
There was a problem hiding this comment.
Please wrap all block comments @ 80 columns.
| return resolver.Target{ | ||
| Scheme: u.Scheme, | ||
| Authority: u.Host, | ||
| URL: *u, | ||
| URL: *u, | ||
| }, nil |
There was a problem hiding this comment.
Nit: make the code a little more compact to improve readability:
return resolver.Target{URL: *u}, nil| _, err := url.Parse(target) | ||
| if err != nil { | ||
| t.Fatalf("Unexpected error parsing URL: %v", err) | ||
| } |
There was a problem hiding this comment.
Delete these lines entirely, please, as they don't do anything except validate our test data, which should already be valid.
| _, err := url.Parse(target) | ||
| if err != nil { | ||
| t.Fatalf("Unexpected error parsing URL: %v", err) | ||
| } |
|
|
||
| var customAuthorityDialler = func(authority string) func(ctx context.Context, network, address string) (net.Conn, error) { | ||
| return func(ctx context.Context, network, address string) (net.Conn, error) { | ||
| var addressDialer = func(authority string) func(ctx context.Context, network, address string) (net.Conn, error) { |
There was a problem hiding this comment.
Nit: s/authority/address/ since this is an "address dialer" not an "authority dialer" now. And just return type func(context.Context, string, string) (net.Conn, error) { as there's no need to name the arguments to the function returned.
32bf00f to
ceb83c4
Compare
|
@dfawley @easwars I would like to help review this PR, but could you please clarify this : is the goal of #5795 to remove the deprecated resolver.Target.Authority and resolver.Target.Scheme (that is what the PR is doing, and it's a breaking change) or just clean and remove their usages across the internal codebase? Thanks |
|
Could you please take care of the merge conflict, and if you think you have addressed Doug's comments, feel free to set the assignee back to him. |
dfawley
left a comment
There was a problem hiding this comment.
LGTM, thanks!
-
Please do a search (again?) internally and make sure nobody is referencing these deprecated fields, and update them before merging this PR if so.
-
Please don't resolve comments in github code reviews unless you are a reviewer of the PR. Resolving comments makes it hard to see what was asked for by the reviewer, unfortunately. Github is weird that way.
Fixes #5795
RELEASE NOTES: