Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add ability to pass proxyOptions #688

Merged
merged 3 commits into from
Oct 11, 2021
Merged

add ability to pass proxyOptions #688

merged 3 commits into from
Oct 11, 2021

Conversation

yannickglt
Copy link
Contributor

@yannickglt yannickglt commented Jun 9, 2021 •
edited by thornjad
Loading

Replaces #459

http-proxy library that is used in http-server support options.

Fixes #214, fixes #246, fixes #638, fixes #719

@yannickglt yannickglt force-pushed the http-proxy-options branch from 542ac74 to 0155b4b Compare June 9, 2021 15:10
@yannickglt
Copy link
Contributor Author

@thornjad already made the first review of #459.

I tried to bring the changes he expected, but I didn't get that two points:

@yannickglt yannickglt force-pushed the http-proxy-options branch from 0155b4b to 29678ea Compare June 9, 2021 15:15
@yannickglt
Copy link
Contributor Author

Is it possible to be reviewed or get an advice about this please?

@thornjad thornjad self-requested a review July 12, 2021 20:07
@thornjad thornjad added the minor version non-breaking, non-trivial change label Jul 12, 2021
@thornjad thornjad added this to the v0.13.0 milestone Jul 12, 2021
@thornjad thornjad mentioned this pull request Jul 12, 2021
Closed
thornjad
thornjad requested changes Aug 6, 2021
View reviewed changes
Copy link
Member

@thornjad thornjad left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good! Last couple things to do:

  • Add this option to doc/http-server.1
  • Tests! They don't need to be too complex since http-proxy already has its own tests, but we should make sure the options are getting through

@thornjad thornjad modified the milestones: v0.13.0, v14.0 Aug 6, 2021
@yannickglt
Copy link
Contributor Author

yannickglt commented Sep 15, 2021 •
edited
Loading

I added the expected doc and an example (inspired from http-proxy) which tests proxy options and ensures the issue #214 is covered.

@yannickglt yannickglt requested a review from thornjad September 15, 2021 14:13
thornjad
thornjad requested changes Oct 5, 2021
View reviewed changes
@thornjad
Copy link
Member

thornjad commented Oct 5, 2021 •
edited
Loading

Also, could you merge/rebase the current master in again? I don't expect conflicts, but it looks like this PR doesn't have the current tests setup, given that none are running on it

Nevermind, tests have run now

This was referenced Oct 5, 2021
Closed
Closed
Closed
@dwjohnston
Copy link

dwjohnston commented Oct 6, 2021 •
edited
Loading

This PR appears to solve #719

@thornjad thornjad modified the milestone: v14.0 Oct 11, 2021
@thornjad thornjad merged commit 35ff346 into http-party:master Oct 11, 2021
@yannickglt
Copy link
Contributor Author

Thanks a lot @thornjad

@yannickglt yannickglt deleted the http-proxy-options branch October 12, 2021 08:56
@yannickglt yannickglt mentioned this pull request Oct 20, 2021
Closed
@thom4parisot
Copy link

Thanks @yannickglt, very helpful proposal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thornjad thornjad thornjad approved these changes

Assignees
No one assigned
Labels
feature minor version non-breaking, non-trivial change
Projects
None yet
Milestone
v14.0
4 participants
@yannickglt @thornjad @dwjohnston @thom4parisot