resolver: delete Target.Scheme and Target.Authority #6363
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. |
ginayeh
changed the title
clientconn.go
Outdated
| @@ -1807,18 +1807,15 @@ func (cc *ClientConn) parseTargetAndFindResolver() error { | |||
| } | |||
|
|
|||
| // 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.
clientconn.go
Outdated
| 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
clientconn_parsed_target_test.go
Outdated
| _, 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.
clientconn_parsed_target_test.go
Outdated
| _, err := url.Parse(target) | ||
| if err != nil { | ||
| t.Fatalf("Unexpected error parsing URL: %v", err) | ||
| } |
| @@ -86,14 +86,14 @@ var ( | |||
| minDNSResRate = 30 * time.Second | |||
| ) | |||
|
|
|||
| 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.
ginayeh
force-pushed
the
5795
branch
2 times, most recently
from
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. |
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: