-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 favicon returning 500 inside container #3569
Conversation
net/http depends on mime-type to set the Content-Type for a http response, mime-type is calculated using https://pkg.go.dev/mime#TypeByExtension (https://github.com/golang/go/blob/master/src/net/http/fs.go#L235) In alpine, there is no default installation of mime-type library hence net/http was setting application/text for favicon.ico which was leading to 500. Adding mailcap pacakge will install /etc/mime.types and fix the issue Signed-off-by: Ashmita152 <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #3569 +/- ##
==========================================
+ Coverage 96.46% 96.48% +0.01%
==========================================
Files 264 264
Lines 15429 15429
==========================================
+ Hits 14884 14886 +2
+ Misses 460 458 -2
Partials 85 85
Continue to review full report at Codecov.
|
maybe we can add a test to after UI image is built to ensure that curl for favicon returns ok? |
btw, nice find! |
Signed-off-by: Ashmita152 <[email protected]>
@@ -138,6 +142,18 @@ func healthCheck() error { | |||
return fmt.Errorf("query service is not ready") | |||
} | |||
|
|||
func faviconCheck() error { |
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 don't think this one is testing the docker image
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.
Hi Yuri,
My understanding may be wrong but looking at this code https://github.com/jaegertracing/jaeger/blob/main/scripts/build-all-in-one-image.sh#L23-L32 it seems we first bring up the newly built all-in-one image and then run the integration test from this file against the container.
Signed-off-by: Albert Teoh <[email protected]>
Signed-off-by: Ashmita152 [email protected]
Which problem is this PR solving?
Short description of the changes
cc @yurishkuro