-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix for parsing port at log middleware #4348
Conversation
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.
Thanks for the fix.
This pr worth a unit test. Would you mind adding 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.
Looks good SriKrishna :) i just have small style suggestions
Nice! Can we add more clear PR title? It does not tell much what exactly was changed to fix it. |
Added a unit test to test with an URL that does not explicitly contain a port number. |
@spaparaju please fix the DCO:
https://github.com/thanos-io/thanos/pull/4348/checks?check_run_id=2861409152 |
Signed-off-by: Srikrishna Paparaju <[email protected]> Signed-off-by: Srikrishna Paparaju <[email protected]>
Signed-off-by: Srikrishna Paparaju <[email protected]>
/retest |
Signed-off-by: spaparaju <[email protected]>
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.
Still seeing these errors in the logs after upgrading Thanos from 0.21.1 to 0.24.0 |
This PR fixes issue#: 4320
Changes
Added check for
:
prior to splitting the port from the hostURL.Verification
Tested on standalone, Kube. environments and did not observe parsing errors in the log files.