GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Revert "net: validate host name for server listen" by jakecastelli · Pull Request #54554 · nodejs/node

Conversation

jakecastelli commented Aug 25, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

This reverts commit 52322aa.

Both PRs #54264 and #54470 made changes to the ipv6 related code base (net) and the latter caused the Github CI to break (break the test that added in the earlier PR). ~~We should revert the change and properly fix the test before #54470 can land again to unblock the CI.~~

Update: Luigi has gone ahead and fixed the broken tests in #54556.
Update: More issues were identified, reopened again to revert it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss and puskin reacted with rocket emoji

All reactions

🚀 2 reactions


          Revert "net: validate host name for server listen"

c3653d2

This reverts commit 52322aa.

nodejs-github-bot commented Aug 25, 2024

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/net

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot added needs-ci

PRs that need a full CI run.

net Issues and PRs related to the net subsystem. labels

Aug 25, 2024

jakecastelli mentioned this pull request

Aug 25, 2024

test: refactor test_runner tests to change default reporter #54547

Merged

avivkeller added the revert PRs that revert previously landed PRs. label

Aug 25, 2024

avivkeller commented Aug 25, 2024

Copy link

Copy Markdown

Member

CC @jazelly

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ovflowd commented Aug 25, 2024

Copy link

Copy Markdown

Member

@jakecastelli are you looking into having this fast tracked?

jakecastelli reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ovflowd approved these changes

Aug 25, 2024

View reviewed changes

jakecastelli added the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label

Aug 25, 2024

github-actions Bot commented Aug 25, 2024

Copy link

Copy Markdown

Contributor

Fast-track has been requested by @jakecastelli. Please 👍 to approve.

jakecastelli, ovflowd, pimterry, bricss, and benjamingr reacted with thumbs up emoji

All reactions

👍 5 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ovflowd commented Aug 25, 2024

Copy link

Copy Markdown

Member

@jakecastelli your own 👍 doesn't count for the fast-track request. So another collaborator is needed to fast track.

jakecastelli reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jakecastelli commented Aug 25, 2024

Copy link

Copy Markdown

Member Author

@jakecastelli your own 👍 doesn't count for the fast-track request. So another collaborator is needed to fast track.

Thanks for the heads up 🙏

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 25, 2024

jasnell approved these changes

Aug 25, 2024

View reviewed changes

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 25, 2024

nodejs-github-bot commented Aug 25, 2024

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

codecov Bot commented Aug 25, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.35%. Comparing base (7fea010) to head (c3653d2).
Report is 326 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54554      +/-   ##
==========================================
+ Coverage   87.32%   87.35%   +0.02%     
==========================================
  Files         649      649              
  Lines      182618   182618              
  Branches    35033    35045      +12     
==========================================
+ Hits       159475   159519      +44     
+ Misses      16407    16382      -25     
+ Partials     6736     6717      -19

Files with missing lines	Coverage Δ
lib/net.js	`92.81% <ø> (ø)`

... and 27 files with indirect coverage changes

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca approved these changes

Aug 25, 2024

View reviewed changes

lpinca commented Aug 25, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Can't we simply fix the test instead of reverting this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca added a commit to lpinca/node that referenced this pull request

Aug 25, 2024


          test: use valid hostnames

2d7aee7

Use valid hostnames in
`test/sequential/test-net-server-listen-ipv6-link-local.js`.

Refs: nodejs#54554

lpinca mentioned this pull request

Aug 25, 2024

test: use valid hostnames #54556

Merged

UlisesGascon approved these changes

Aug 25, 2024

View reviewed changes

avivkeller commented Aug 25, 2024

Copy link

Copy Markdown

Member

See #54556 as well

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot pushed a commit that referenced this pull request

Aug 25, 2024


          test: use valid hostnames

1399d4e

Use valid hostnames in
`test/sequential/test-net-server-listen-ipv6-link-local.js`.

Refs: #54554
PR-URL: #54556
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

aduh95 removed the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label

Aug 25, 2024

aduh95 commented Aug 25, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Removing fast-track PRs proposed for a shorter-than-standard waiting period before landing. as the other PR was merged, which IIUC also fixes the CI. Do we still want to land this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jakecastelli commented Aug 25, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

Thanks guys for looking into this 🙏 it was nearly 1:30 A.M. when I found this issue and I wasn't sure if there is going to be more issues, the safest way comes to my mind was raising the issue, reverting (as I knew prior to this commit it was green), run CI, maybe land when I wake up 😄 but thanks @lpinca for looking into and fixing it and also everyone else who helped here!

Do we still want to land this?

~~Since the Github Actions / CI is green, we don't want to land this anymore 👍 if there are bugs, we can always send patches instead.~~

Update: Yes, we found bigger issue with that commit, and we need to revert the commit.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jakecastelli closed this

Aug 25, 2024

jakecastelli mentioned this pull request

Aug 26, 2024

doc: improve the process for landing a PR #54564

Closed

lpinca reopened this

Aug 26, 2024

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 26, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 26, 2024

nodejs-github-bot commented Aug 26, 2024

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Aug 26, 2024

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This comment was marked as outdated.

jakecastelli commented Aug 26, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

Looks like we started the rerun around the same time @lpinca, I will cancel mine (as mine appears to be late 😄)
Update: cancelled and marked as outdated.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jakecastelli commented Aug 26, 2024

Copy link

Copy Markdown

Member Author

test-http2-socket-close has been extremely flaky 😞

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Aug 26, 2024

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Aug 26, 2024 •

edited by jakecastelli

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca approved these changes

Aug 26, 2024

View reviewed changes

benjamingr approved these changes

Aug 26, 2024

View reviewed changes

benjamingr added the commit-queue PRs queued for automated landing through the Commit Queue. label

Aug 26, 2024

nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels

Aug 27, 2024

nodejs-github-bot commented Aug 27, 2024

Copy link

Copy Markdown

Collaborator

Commit Queue failed

- Loading data for nodejs/node/pull/54554
✔  Done loading data for nodejs/node/pull/54554
----------------------------------- PR info ------------------------------------
Title      Revert "net: validate host name for server listen" (#54554)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     jakecastelli:revert-52322aa -> nodejs:main
Labels     net, needs-ci, revert
Commits    1
 - Revert "net: validate host name for server listen"
Committers 1
 - jakecastelli <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54554
Reviewed-By: Claudio Wunder <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54554
Reviewed-By: Claudio Wunder <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 25 Aug 2024 15:48:40 GMT
   ✔  Approvals: 5
   ✔  - Claudio Wunder (@ovflowd): https://github.com/nodejs/node/pull/54554#pullrequestreview-2259334489
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54554#pullrequestreview-2259336303
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/54554#pullrequestreview-2261077656
   ✔  - Ulises Gascón (@UlisesGascon): https://github.com/nodejs/node/pull/54554#pullrequestreview-2259348797
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/54554#pullrequestreview-2261429112
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2024-08-26T14:06:43Z: https://ci.nodejs.org/job/node-test-pull-request/61491/
- Querying data for job/node-test-pull-request/61491/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/10581610536

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jakecastelli commented Aug 28, 2024

Copy link

Copy Markdown

Member Author

Last GitHub CI failed

This is not true 😞 landing manually now

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jakecastelli added a commit that referenced this pull request

Aug 28, 2024


          Revert "net: validate host name for server listen"

4f14eb1

This reverts commit 52322aa.

PR-URL: #54554
Reviewed-By: Claudio Wunder <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

jakecastelli commented Aug 28, 2024

Copy link

Copy Markdown

Member Author

Landed in 4f14eb1

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jakecastelli closed this

Aug 28, 2024

jakecastelli removed the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label

Aug 28, 2024

Gguyzaza commented Aug 28, 2024

Copy link

Copy Markdown

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

RafaelGSS pushed a commit that referenced this pull request

Aug 30, 2024


          test: use valid hostnames

66ae9f4

Use valid hostnames in
`test/sequential/test-net-server-listen-ipv6-link-local.js`.

Refs: #54554
PR-URL: #54556
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

RafaelGSS pushed a commit that referenced this pull request

Aug 30, 2024


          Revert "net: validate host name for server listen"

04f83b5

This reverts commit 52322aa.

PR-URL: #54554
Reviewed-By: Claudio Wunder <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

RafaelGSS mentioned this pull request

Aug 30, 2024

v22.8.0 proposal #54560

Merged

targos added the dont-land-on-v20.x label

Sep 22, 2024

Reviewers

jasnell

jasnell approved these changes

benjamingr

benjamingr approved these changes

lpinca

lpinca approved these changes

UlisesGascon

UlisesGascon approved these changes

ovflowd

ovflowd approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

needs-ci

PRs that need a full CI run.

net

Issues and PRs related to the net subsystem.

revert

PRs that revert previously landed PRs.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

12 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

jakecastelli commented Aug 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 25, 2024

Uh oh!

avivkeller commented Aug 25, 2024

Uh oh!

ovflowd commented Aug 25, 2024

Uh oh!

github-actions Bot commented Aug 25, 2024

Uh oh!

ovflowd commented Aug 25, 2024

Uh oh!

jakecastelli commented Aug 25, 2024

Uh oh!

nodejs-github-bot commented Aug 25, 2024

Uh oh!

codecov Bot commented Aug 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lpinca commented Aug 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avivkeller commented Aug 25, 2024

Uh oh!

aduh95 commented Aug 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakecastelli commented Aug 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 26, 2024

Uh oh!

nodejs-github-bot commented Aug 26, 2024

Uh oh!

This comment was marked as outdated.

jakecastelli commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakecastelli commented Aug 26, 2024

Uh oh!

nodejs-github-bot commented Aug 26, 2024

Uh oh!

nodejs-github-bot commented Aug 26, 2024 • edited by jakecastelli Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 27, 2024

Uh oh!

jakecastelli commented Aug 28, 2024

Uh oh!

jakecastelli commented Aug 28, 2024

Uh oh!

Gguyzaza commented Aug 28, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

jakecastelli commented Aug 25, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

codecov Bot commented Aug 25, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

lpinca commented Aug 25, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

aduh95 commented Aug 25, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

jakecastelli commented Aug 25, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

jakecastelli commented Aug 26, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Aug 26, 2024 •

edited by jakecastelli

Loading

Uh oh!

There was an error while loading. Please reload this page.