fix custom-header #369
Merged
fix custom-header #369
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
collector must return the result it's unclear why the test passed, when running the same code by hand did - allow colons in header values (split only on first) - log and exit when proxy cli fails to start in tests
minrk
force-pushed
the
return-custom-header
branch
from
16736b4 to
807dda7
Compare
Timost
reviewed
Feb 2, 2022
bin/configurable-http-proxy
Outdated
| value = value.trim(); | ||
| var colon = value.indexOf(":"); | ||
| if (colon <= 0) { | ||
| log.error("A colon was expected in custom header: " + value); |
There was a problem hiding this comment.
I think calling log here isn't valid. I got crashes when fidling with this part of the code to make it log using log.
There was a problem hiding this comment.
Good catch! Fixed and test added.
| @@ -229,7 +240,7 @@ describe("CLI Tests", function () { | |||
| "--custom-header", | |||
| "k1: v1", | |||
| "--custom-header", | |||
| " k2 : v2 v2 ", | |||
| " k2 : host:123 ", | |||
There was a problem hiding this comment.
Maybe adding a test with an invalid header (eg --custom-header badheader) would be worthwile ? I think the current error logging in collectHeadersIntoObject crashes.
and fix undefined `log.error` in favor of `console.error`
|
LGTM ! Thank you for doing all the work ! 🙏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
collector must return the result
The test was passing because the update-in-place way we implemented the collector apparently works if you have more than one header, but not if you have only one. I don't know if that's a bug upstream or not, but the right thing to do is to return after each collection.
Additional improvements:
closes #368