Skip to content

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

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
Closed
targos wants to merge 1 commit into from
Closed

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

targos wants to merge 1 commit into from

Conversation

@targos
Copy link
Member

@targostargos 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-actionsgithub-actionsbot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Jun 6, 2021
@targostargos mentioned this pull request Jun 6, 2021
Closed
mcollina
mcollina approved these changes Jun 6, 2021
View reviewed changes
Copy link
Member

@mcollinamcollina 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
MemberAuthor

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

@mhdawsonmhdawson 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
MemberAuthor

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

richardlau commented Jun 7, 2021

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

@mmarchinimmarchini 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

nodejs-github-bot commented Jun 8, 2021

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 8, 2021

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 8, 2021

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Jiawen Geng <[email protected]>
@mcollina
Copy link
Member

mcollina commented Jun 8, 2021

Landed in f504c9c

@mcollinamcollina closed this Jun 8, 2021
@targostargos deleted the fix-38922 branch June 8, 2021 16:45
@mmarchinimmarchini 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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Jiawen Geng <[email protected]>
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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Jiawen Geng <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Jun 14, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@jasnelljasnelljasnell approved these changes

@cjihrigcjihrigcjihrig approved these changes

@ronagronagronag approved these changes

@gengjiawengengjiawengengjiawen approved these changes

@mhdawsonmhdawsonmhdawson approved these changes

@VoltrexKeyvaVoltrexKeyvaVoltrexKeyva approved these changes

+3 more reviewers

@mmarchinimmarchinimmarchini approved these changes

@dnlupdnlupdnlup approved these changes

@bl-uebl-uebl-ue approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

httpIssues or PRs related to the http subsystem.needs-ciPRs 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

14 participants

@targos@bl-ue@Trott@richardlau@mcollina@nodejs-github-bot@jasnell@cjihrig@ronag@gengjiawen@mmarchini@mhdawson@dnlup@VoltrexKeyva