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

only prepend http:// to endpoint if no protocol was provided #191

Merged
merged 2 commits into from
Oct 13, 2017

Conversation

Crunch09
Copy link
Contributor

@Crunch09 Crunch09 commented Oct 10, 2017

Hey,
this addresses the fix mentioned in #174: Although the Toxiproxy HTTP API does not support HTTPS, it should not prepend http:// to the endpoint.

client.NewClient("https://example.com")
// Before: http://https//example.com
// After: https://example.com

I would love to write a test for this but endpoint is not an exported identifier so i'm not sure how to test this as this is basically the first Go code that i have ever written 🙂

fixes #174

Copy link
Contributor

@jpittis jpittis left a comment

Choose a reason for hiding this comment

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

This is an improvement over the current code. Thanks for opening a PR!

Because Toxiproxy does not support HTTPS, it might be nice to fail fast with a clear error message when an https: protocol is provided.

IMO it would be appropriate to fail with a log.Fatal("the toxiproxy client does not support https") if an HTTPS protocol is provided.

client/client.go Outdated
@@ -47,7 +47,8 @@ type Proxy struct {
// with Toxiproxy. Endpoint is the address to the proxy (e.g. localhost:8474 if
// not overriden)
func NewClient(endpoint string) *Client {
if !strings.HasPrefix(endpoint, "http://") {
protocolProvided, _ := regexp.MatchString(`^https?://`, endpoint)
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally it's better to create a regex via regexp.MustCompile() and then calling the MatchString method. This ensures we don't ignore a possible error in the regex.

@Crunch09
Copy link
Contributor Author

@jpittis Thank you for the review! 👍 I updated the code to now fail when https:// is provided and removed the regexp again because i felt it didn't add much value then but thank you for letting me know about MustCompile! 💯

Before:
bildschirmfoto 2017-10-12 um 23 17 07

After:
bildschirmfoto 2017-10-12 um 23 17 14

@jpittis jpittis merged commit 87f5ccc into Shopify:master Oct 13, 2017
@jpittis
Copy link
Contributor

jpittis commented Oct 13, 2017

Thanks for the contribution! :)

@Crunch09 Crunch09 deleted the host_protocol branch October 13, 2017 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cli fails with https hosts
2 participants