Conversation
kobalicek
left a comment
kobalicek
left a comment
There was a problem hiding this comment.
LGTM - needs one more review and possibly a future test case so this doesn't happen again.
harshavardhana
left a comment
harshavardhana
left a comment
There was a problem hiding this comment.
Please also add a test
flokaiser
force-pushed
the
fix_port_parsing
branch
3 times, most recently
from
6bbe61b to
8cded87
Compare
| return; | ||
| } | ||
|
|
||
| this->https = url.https; |
There was a problem hiding this comment.
This is not correct. this->https is set in the constructor definition already.
There was a problem hiding this comment.
I have seen that, but the Url class parses the schema to set the https flag in the url class that…
Correct me if I am wrong, but if I am constructing a u=BasicUrl("https://foo.bar", false)
Then u.https is false
There was a problem hiding this comment.
Firstly BaseUrl accepts host. If URL is used, it takes only host and port. Secondly passed argument should override values. The current behavior is correct and this change is not correct.
There was a problem hiding this comment.
I don't think this is a normal operation, in fact, I happened to experience this problem and have been troubled for a while for using of an HTTP address but the constructed baseURL being the default HTTPS. I cannot understand choosing to pass parameters instead of using URL parsed values. After all, people are sensitive to their own URLs and are not aware of the construction of functions in third-party libraries
There was a problem hiding this comment.
The right fix is to fail if URL is passed like minio-go and minio-py. We should not support URL style as base URL
without this change, urls like 5.my.s3.cluster were misinterpreted and the '5' parsed as the port of the url. Bug: * the getline() function puts the entire input string into the destination string, if the delimiter was not found. This leads to the entire host string being fed to the stoi() function to parse the port. Fix: * applied suggestion by @balamurugana
this flag assignement was removed in: minio#111 I don't know if it was removed intentional, but without it the BaseUrl.https flag only reflects the state of how BaseUrl was constructed with.
flokaiser
force-pushed
the
fix_port_parsing
branch
from
8cded87 to
8a6591b
Compare
5 participants
without this change, urls like 5.my.s3.cluster were misinterpreted and
the '5' parsed as the port of the url.
Bug:
Fix: