Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
rpc: fix RPC client doesn't handle url's without ports #6507
rpc: fix RPC client doesn't handle url's without ports #6507
Changes from 3 commits
bd8ba9e
dc26fe6
fa4366e
30608d0
ce351d9
d34845d
9036c5e
a6a777b
c896d70
554f472
44c00ae
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do you have an example of input here and resulting output?
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.
https://foo-bar.com
->foo-bar.com:https
http://foo-bar.com
->foo-bar.com:http
unix:///tmp/test
-> 'unix:///tmp/test' (because it's UnixSocket)ref: https://cs.opensource.google/go/go/+/refs/tags/go1.16.4:src/net/ipsock.go;drc=193d5141318d65cea310d995258288bd000d734c;l=248
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.
And why do we need to do this?
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.
These fixes just try to tackle the issue @marbar3778 posted. We can just close it if we think the current error message is good enough. But the user needs to aware they need to input the URL address with port info.
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.
in
newParsedURL
function.if we dourl.Parse
withhttps://foo-bar.com
.We will get
url.Scheme
= "https",url.Host
= "foo-bar.com"So under this input case, It's not able to connect to the remote due to a missing port.
Therefore I am padding the protocol at the end of the Host, and then the socket will look up the default port for the protocol.
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.
I will let @marbar3778 approve this one 👍
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.
I think this makes things a bit easier for when a user is connecting to a URL. What is the norm here? Should the application handle this instead?
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.
do we want to add other kinds of protocols and/or test error cases?
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.
@tychoish what other kinds of protocols we will use in the HTTP client?
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.
I can add some error cases.
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.
Added 2 error test cases.