feat: add https proxy server implementation#626
Conversation
jirimoravcik
left a comment
There was a problem hiding this comment.
Thanks for the PR, looks great, had a few comments
tobice
left a comment
There was a problem hiding this comment.
Honestly I have no idea how proxy-chain works and my general knowledge of proxies is still limited 😄 So I'm leaving the actual functionality review to Jirka and Ludvík.
From my PoV the application code change is small, well documented and easy to read. No concerns there.
The tests, especially the new ones, would benefit from splitting into files. Also one scenario caught my attention due to its nested structure. I'm not sure why that's needed.
Finally, I'd expand on the PR description. You can e.g. add there the nice diagram from the original PR.
|
I will remove myself as a reviewer. I left some initial feedback and you have now Dan :)) |
danpoletaev
left a comment
There was a problem hiding this comment.
We went through this in person! Looks good for me 👍
This PR represents simplification of #602. It contains changes related only to HTTPS proxy server support.
Note: I might miss some edge cases for HTTPS server proxy, however I think that current coverage is good enough and covers core logic fully (80/20) and is backward compatible (HTTP proxy server tests pass).
Closes: #603