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

http: use localAddress instead of path #5190

Closed
wants to merge 1 commit into from
Closed

http: use localAddress instead of path #5190

wants to merge 1 commit into from

Conversation

dirceu
Copy link
Contributor

@dirceu dirceu commented Feb 11, 2016

Fix options usage on lib/_http_agent.js for the Legacy API.

Fixes: #5051

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Feb 11, 2016
@evanlucas
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/1633/

@jasnell
Copy link
Member

jasnell commented Feb 11, 2016

/cc @nodejs/http

@mscdex
Copy link
Contributor

mscdex commented Feb 11, 2016

LGTM

@evanlucas
Copy link
Contributor

LGTM. Maybe this is something that we should run citgm on first? /cc @thealphanerd

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

New CI: https://ci.nodejs.org/job/node-test-pull-request/2002/

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

@nodejs/ctc ... anyone have any concerns with this one?

@MylesBorins
Copy link
Contributor

citgm: https://ci.nodejs.org/job/thealphanerd-smoker/127/

@MylesBorins
Copy link
Contributor

MylesBorins commented May 9, 2016 •
edited
Loading

Running test suites one more time. Is anyone opposed to this landing if everything is green?

ci: https://ci.nodejs.org/job/node-test-pull-request/2551/
citgm: https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker/256/

edit: we are having issues with citgm in CI right now... figuring out what is going on, ignore results in the mean time

@MylesBorins MylesBorins self-assigned this May 9, 2016
@bnoordhuis
Copy link
Member

LGTM but there are a few style violations in the test that need to be fixed. make jslint will point them out.

@dirceu
http: use localAddress instead of path
cd4c6b5 
Fix `options` usage on `lib/_http_agent.js` for the Legacy API.

Fixes: nodejs#5051
@dirceu
Copy link
Contributor Author

dirceu commented May 10, 2016

@bnoordhuis thanks! I've updated the test file, and there are no more jslint errors.

@bnoordhuis
Copy link
Member

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/2560/

@MylesBorins
Copy link
Contributor

MylesBorins commented May 10, 2016 •
edited
Loading

citgm: https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker/261/

Hopefully the CI problems we were seeing yesterday should be resolved

@bnoordhuis
Copy link
Member

CI is green, citgm is taking too long to load here.

@jasnell
Copy link
Member

jasnell commented May 11, 2016

LGTM

@MylesBorins
Copy link
Contributor

citgm: https://ci.nodejs.org/job/thealphanerd-smoker/270/

if it is green I will land this

bnoordhuis pushed a commit that referenced this pull request May 19, 2016
@dirceu @bnoordhuis
http: use localAddress instead of path
fe77de1 
Fix `options` usage on `lib/_http_agent.js` for the Legacy API.

Fixes: #5051
PR-URL: #5190
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis
Copy link
Member

CITGM is green. Landed in fe77de1, thanks Dirceu.

@bnoordhuis bnoordhuis closed this May 19, 2016
Fishrock123 pushed a commit that referenced this pull request May 23, 2016
@dirceu @Fishrock123
http: use localAddress instead of path
a3b3a47 
Fix `options` usage on `lib/_http_agent.js` for the Legacy API.

Fixes: #5051
PR-URL: #5190
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

how long should this stay on v6 before a backport?

rvagg pushed a commit that referenced this pull request Jun 2, 2016
@dirceu @rvagg
http: use localAddress instead of path
2ab3609 
Fix `options` usage on `lib/_http_agent.js` for the Legacy API.

Fixes: #5051
PR-URL: #5190
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

ping @nodejs/http re: backport

@MylesBorins
Copy link
Contributor

@nodejs/lts is this something we want to see land on v4.x?

@MylesBorins MylesBorins removed their assignment Dec 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dirceu @evanlucas @jasnell @mscdex @MylesBorins @bnoordhuis