-
-
Notifications
You must be signed in to change notification settings - Fork 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
fileserver: Fix compression breaks using httpInclude (#4352) #4358
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, I like where this is going. I'm not 100% sure on the right function to use here. Is there a specific reason you went with Add instead of Set?
d031748
to
f3eaa62
Compare
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 the change doesn't compile yet, that will need to be fixed (the context
collision) - see the CI logs for details.
Otherwise, this LGTM.
Does your test fail without the change?
* Add `Accept-Encoding: identity` in virtReq * Add Unit tests for funcHTTPInclude * Change variable name `context` to `tplContext` to use `context` package
It seemed to doubt whether test failed without my changes. But just now, I came up with how to check overwriting Accept-Encoding header with this test. |
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 to me! Thanks!
Fixes: #4352
Add
Accept-Encoding: identity
in virtReqAdd Unit tests for funcHTTPInclude
Change variable name
context
totplContext
to usecontext
packageBut I don't know my test implementation is really enough to this fix.
We should make a generic tests or e2e tests about
templates
to check how does it compression or not.I'm not sure how and what to add these tests.