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

Revert "http: make HEAD method to work with keep-alive" #38949

Closed
wants to merge 1 commit into from
Closed

Revert "http: make HEAD method to work with keep-alive" #38949

wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Jun 6, 2021

This reverts commit 7afa533.

The change breaks clients like cURL.

Fixes: #38922

@targos
Revert "http: make HEAD method to work with keep-alive"
dce7bb2 
This reverts commit 7afa533.

The change breaks clients like cURL.

Fixes: nodejs#38922
@github-actions github-actions bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jun 6, 2021
@targos targos mentioned this pull request Jun 6, 2021
Closed
mcollina
mcollina approved these changes Jun 6, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@targos
Copy link
Member Author

targos commented Jun 6, 2021

Hi, @bl-ue and @VoltrexMaster! I appreciate all your comments in the issue tracker, but it would be helpful if you could refrain from "approving" pull requests. Overall, it causes a lot of notification noise. A thumbs-up reaction would convey the same information without piling up notifications for collaborators.

Given how much you both watch and interact with the repository, I want to make sure you are aware of our triager role, in case that is of interest to you.

@bl-ue
Copy link
Contributor

bl-ue commented Jun 6, 2021

Hi @targos! Sure, I'll avoid doing that - I didn't realize it was bothering you :)
I'll take a look at that repo and get started helping. I'd like to be a triager, but I don't want my name or email anywhere obvious like on the readme, and I'd also like to avoid any meetings - is this possible? If it's not, I'll just live without the extra power, but it would be cool! :)
I'll definitely help out though.

@Trott
Copy link
Member

Trott commented Jun 7, 2021

I'd like to be a triager, but I don't want my name or email anywhere obvious like on the readme, and I'd also like to avoid any meetings - is this possible?

As far as I know, there are no meetings for triagers, so I think that's not a problem.

Name is also probably not a problem. We allow people to put whatever name they want (although we'd probably step in and block anything that was intentionally misleading, like using the name of an existing collaborator, or something intentionally and blatantly offensive).

Email, I'm less sure about. We need a way to get in touch with people outside of GitHub sometimes, and that is usually through the email listed in the README.

@bl-ue
Copy link
Contributor

bl-ue commented Jun 7, 2021 •
edited
Loading

Email, I'm less sure about. We need a way to get in touch with people outside of GitHub sometimes, and that is usually through the email listed in the README.

Understood, so maybe I'll just have to decline — but I'm available all day every day at https://gitter.im/bl-ue ;)

mhdawson
mhdawson approved these changes Jun 7, 2021
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@targos
Copy link
Member Author

targos commented Jun 7, 2021

@nodejs/tsc @nodejs/lts Does anyone object to pulling this into v14.17.1, to avoid having the change in LTS for too long?

@richardlau
Copy link
Member

No objections to including in 14.17.1 from me.

@mcollina
Copy link
Member

mcollina commented Jun 7, 2021

no objection, actually it's the course of action that I would recommend.

mmarchini
mmarchini approved these changes Jun 7, 2021
View reviewed changes
Copy link
Contributor

@mmarchini mmarchini left a comment

Choose a reason for hiding this comment

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

and +1 on pulling it on 14.17.1

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

mcollina pushed a commit that referenced this pull request Jun 8, 2021
@targos @mcollina
Revert "http: make HEAD method to work with keep-alive"
f504c9c 
This reverts commit 7afa533.

The change breaks clients like cURL.

Fixes: #38922

PR-URL: #38949
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@mcollina
Copy link
Member

mcollina commented Jun 8, 2021

Landed in f504c9c

@mcollina mcollina closed this Jun 8, 2021
@targos targos deleted the fix-38922 branch June 8, 2021 16:45
@mmarchini mmarchini mentioned this pull request Jun 9, 2021
Merged
targos added a commit that referenced this pull request Jun 11, 2021
@targos
Revert "http: make HEAD method to work with keep-alive"
336571f 
This reverts commit 7afa533.

The change breaks clients like cURL.

Fixes: #38922

PR-URL: #38949
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
targos added a commit that referenced this pull request Jun 11, 2021
@targos
Revert "http: make HEAD method to work with keep-alive"
07c55d2 
This reverts commit 7afa533.

The change breaks clients like cURL.

Fixes: #38922

PR-URL: #38949
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mary Marchini <oss@mmarchini.me>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@danielleadams danielleadams mentioned this pull request Jun 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

@bl-ue bl-ue bl-ue approved these changes

@dnlup dnlup dnlup approved these changes

@mhdawson mhdawson mhdawson approved these changes

@mmarchini mmarchini mmarchini approved these changes

@gengjiawen gengjiawen gengjiawen approved these changes

@ronag ronag ronag approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Potential breaking change on v14.17.0