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.

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

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

Closed
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

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

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 ;)

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.

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

mcollina pushed a commit that referenced this pull request Jun 8, 2021
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

@mcollina mcollina closed this Jun 8, 2021
@targos targos deleted the fix-38922 branch June 8, 2021 16:45
targos added a commit that referenced this pull request Jun 11, 2021
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
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]>
@danielleadams danielleadams mentioned this pull request Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential breaking change on v14.17.0