Skip to content

Revert "net: validate host name for server listen" - #54554

Closed
jakecastelli wants to merge 1 commit into
nodejs:mainfrom
jakecastelli:revert-52322aa
Closed

Revert "net: validate host name for server listen"#54554
jakecastelli wants to merge 1 commit into
nodejs:mainfrom
jakecastelli:revert-52322aa

Conversation

@jakecastelli

@jakecastellijakecastelli commented Aug 25, 2024

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.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-botnodejs-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
@avivkelleravivkeller added the revert PRs that revert previously landed PRs. label Aug 25, 2024
@avivkeller

Copy link
Copy Markdown
Member

CC @jazelly

@ovflowd

Copy link
Copy Markdown
Member

@jakecastelli are you looking into having this fast tracked?

@jakecastellijakecastelli added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 25, 2024
@github-actions

Copy link
Copy Markdown
Contributor

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

@ovflowd

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

Copy link
Copy Markdown
MemberAuthor

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

Thanks for the heads up 🙏

@jakecastellijakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 25, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 25, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecovBot commented Aug 25, 2024

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 linesCoverage Δ
lib/net.js92.81% <ø> (ø)

... and 27 files with indirect coverage changes

@lpinca

lpinca commented Aug 25, 2024

Copy link
Copy Markdown
Member

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

lpinca added a commit to lpinca/node that referenced this pull request Aug 25, 2024
Use valid hostnames in
`test/sequential/test-net-server-listen-ipv6-link-local.js`.
Refs: nodejs#54554
@lpincalpinca mentioned this pull request Aug 25, 2024
@avivkeller

Copy link
Copy Markdown
Member

See #54556 as well

nodejs-github-bot pushed a commit that referenced this pull request Aug 25, 2024
Use valid hostnames in
`test/sequential/test-net-server-listen-ipv6-link-local.js`.
Refs: #54554
PR-URL: #54556
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95aduh95 removed the fast-track PRs that do not need to wait for 48 hours to land. label Aug 25, 2024
@aduh95

aduh95 commented Aug 25, 2024

Copy link
Copy Markdown
Contributor

Removing fast-trackPRs that do not need to wait for 48 hours to land. as the other PR was merged, which IIUC also fixes the CI. Do we still want to land this?

@jakecastelli

jakecastelli commented Aug 25, 2024

Copy link
Copy Markdown
MemberAuthor

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.

@lpincalpinca reopened this Aug 26, 2024
@lpincalpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

This comment was marked as outdated.

@jakecastelli

jakecastelli commented Aug 26, 2024

Copy link
Copy Markdown
MemberAuthor

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.

@jakecastelli

Copy link
Copy Markdown
MemberAuthor

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

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

nodejs-github-bot commented Aug 26, 2024

Copy link
Copy Markdown
Collaborator

@benjamingrbenjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 26, 2024
@nodejs-github-botnodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 27, 2024
@nodejs-github-bot

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 <jake.yuesong@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/54554
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54554
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
--------------------------------------------------------------------------------
ℹ 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

@jakecastelli

Copy link
Copy Markdown
MemberAuthor

Last GitHub CI failed

This is not true 😞 landing manually now

jakecastelli added a commit that referenced this pull request Aug 28, 2024
This reverts commit 52322aa.
PR-URL: #54554
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@jakecastelli

Copy link
Copy Markdown
MemberAuthor

Landed in 4f14eb1

@jakecastellijakecastelli removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Aug 28, 2024
@Gguyzaza

Copy link
Copy Markdown

Ok

RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
Use valid hostnames in
`test/sequential/test-net-server-listen-ipv6-link-local.js`.
Refs: #54554
PR-URL: #54556
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
This reverts commit 52322aa.
PR-URL: #54554
Reviewed-By: Claudio Wunder <cwunder@gnome.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Aug 30, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.netIssues and PRs related to the net subsystem.revertPRs that revert previously landed PRs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

12 participants

@jakecastelli@nodejs-github-bot@avivkeller@ovflowd@lpinca@aduh95@jazelly@Gguyzaza@jasnell@benjamingr@UlisesGascon@targos