-
Notifications
You must be signed in to change notification settings - Fork 60
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
all: use net.JoinHostPort instead of fmt.Sprintf #388
Conversation
I tried to run the tests on my Mac and got the following error, which could be related to my change but I can't tell.
|
@@ -107,7 +108,7 @@ func newIntegrationClientServer( | |||
}() | |||
|
|||
httpURL := fmt.Sprintf("https://localhost:%d/hello", httpPort) |
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 mean you could do e.g. := "https://"+net.JoinHostPort("localhost", strconv.Itoa(httpPort)) + "/hello"
but it obscures the URL a bit too much for me in terms of a readability vs. fit for purpose tradeoff.
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 is fine as-is.
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.
OK - I left as is so I think we are golden.
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.
LGTM modulo nit
@@ -107,7 +108,7 @@ func newIntegrationClientServer( | |||
}() | |||
|
|||
httpURL := fmt.Sprintf("https://localhost:%d/hello", httpPort) |
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 is fine as-is.
I don't get the same failure when running these changes on my machine (also a Mac) - does the test consistently fail for you in the same way? |
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.
Could you please add a changelog entry for this change?
Yes, I'll open a new ticket. |
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.
Fixing what I broke yesterday
apologies - I will update shortly |
ee70bd0
to
d3dae63
Compare
OK, I've added a changelog entry and double checked the rest of the PR comments - I think this is ready for another look. |
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.
Thank you!
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.
One last change and then I think this is good to go 🚀
CHANGELOG.md
Outdated
@@ -147,6 +147,7 @@ | |||
* [ENHANCEMENT] server: clarify documentation for `-server.grpc-max-concurrent-streams` CLI flag. #369 | |||
* [ENHANCEMENT] Ring: Add ID attribute to `InstanceDesc` for ring members. #387 | |||
* [ENHANCEMENT] httpgrpc, grpcutil: added constants and functions for adding request details into outgoing grpc metadata. #391 | |||
* [ENHANCEMENT] Use `net.JoinHostPort` instead of `fmt.Sprintf` to concatenate host and port throughout the codebase to ensure ipv6 compatibility for provided hosts. #388 |
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'd rephrase this to focus on the outcome, not the mechanism we use to achieve it, and I think this can be classified as a bug fix rather than an enhancement (in which case, it should be moved to the end of the list):
* [ENHANCEMENT] Use `net.JoinHostPort` instead of `fmt.Sprintf` to concatenate host and port throughout the codebase to ensure ipv6 compatibility for provided hosts. #388 | |
* [BUGFIX] Correctly format `host:port` addresses when using IPv6. #388 |
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.
Done
Head branch was pushed to by a user without write access
325bbda
to
c2130c6
Compare
CHANGELOG.md
Outdated
@@ -1,5 +1,6 @@ | |||
## Changelog | |||
|
|||
port throughout the codebase. |
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.
Looks like this line was left behind 😬
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.
Ugh, not sure how I missed that one.
c2130c6
to
ea40743
Compare
This will ensure ipv6 compatibility for provided hosts, and this method should also be faster than the general purpose fmt.Sprintf, which must first parse the input string. Fixes grafana#380.
ea40743
to
ccccec6
Compare
Thanks @kevinburkesegment! |
This will ensure ipv6 compatibility for provided hosts, and this method should also be faster than the general purpose fmt.Sprintf, which must first parse the input string. Fixes #380.
This will ensure ipv6 compatibility for provided hosts, and this method should also be faster than the general purpose fmt.Sprintf, which must first parse the input string.
Fixes #380.
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]