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 the change of network interfaces family from string to integer #43054

Closed
wants to merge 5 commits into from
Closed

Revert the change of network interfaces family from string to integer #43054

wants to merge 5 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 11, 2022

This reverts commits 68fb0bf, 4ad342a, and 3a26db9.

Fixes: #43014

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 11, 2022
@aduh95 aduh95 added semver-major PRs that contain breaking changes and should be released in the next major version. tsc-agenda Issues and PRs to discuss during the meetings of the TSC. labels May 11, 2022
@targos
Copy link
Member

targos commented May 11, 2022

IMO the revert doesn't make sense if it is semver-major, because everyone will have to adapt to the change for v18 (which is going to be LTS).

mcollina
mcollina approved these changes May 11, 2022
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

mcollina
mcollina approved these changes May 11, 2022
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

@mcollina
Copy link
Member

+1 to land it on Node v18.

@mcollina mcollina added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels May 11, 2022
@mcollina
Copy link
Member

mcollina commented May 11, 2022 •
edited
Loading

@nodejs/tsc at Today meeting we did not have quorum to LGTM as a non-semver-major change. Does anybody from the TSC objects to this?

We actually had quorum at the end of the meeting, we approve landing this as semver-minor.

@targos targos added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label May 11, 2022
@mcollina mcollina added semver-minor PRs that contain new features and should be released in the next minor version. dont-land-on-v12.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels May 11, 2022
@RaisinTen RaisinTen added the notable-change PRs with changes that should be highlighted in changelogs. label May 11, 2022
RaisinTen
RaisinTen approved these changes May 11, 2022
View reviewed changes
lib/dns.js Outdated Show resolved Hide resolved
@RaisinTen
Copy link
Contributor

Is it okay if we remove this from the tsc-agenda now, since everyone in the meeting was +1 with reverting?

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels May 11, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 11, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
Trott
Trott approved these changes May 11, 2022
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Rubber stamp

@aduh95
Copy link
Contributor Author

aduh95 commented Jun 2, 2022

  • consider this a bug within the SmartOS host and the AIX host, and mark the test as FLAKY on those platforms. 'IPv6' should be treated as 6, and 'IPv4' as 4.

I'm leaning towards this solution. @nodejs/build @nodejs/platform-smartos @nodejs/platform-aix any objections?

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 force-pushed the revert-net-family-integer branch from 070ff18 to 34bb24a Compare June 5, 2022 20:46
@bnoordhuis
Copy link
Member

@aduh95 I remember running into the same issue on smartos a few years ago. I'd be okay with marking the tests flaky.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Jun 13, 2022
Open
15 tasks
@mhdawson mhdawson mentioned this pull request Jun 15, 2022
Closed
@mhdawson
Copy link
Member

We discussed in the TSC meeting and there were no objections to this landing. Doing that now.

mhdawson pushed a commit that referenced this pull request Jun 15, 2022
@aduh95 @mhdawson
dns: accept 'IPv4' and 'IPv6' for family
70b516e 
Refs: #43014

PR-URL: #43054
Fixes: #43014
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@mhdawson
Copy link
Member

Landed as 70b516e

@mhdawson mhdawson closed this Jun 15, 2022
@aduh95 aduh95 mentioned this pull request Jun 15, 2022
Merged
danielleadams pushed a commit that referenced this pull request Jun 16, 2022
@aduh95 @danielleadams
dns: accept 'IPv4' and 'IPv6' for family
0bb84b0 
Refs: #43014

PR-URL: #43054
Fixes: #43014
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
danielleadams added a commit that referenced this pull request Jun 16, 2022
@danielleadams
2022-06-15, Version 18.4.0 (Current)
40d7acc 
Notable changes:

* crypto:
  * remove Node.js-specific webcrypto extensions (Filip Skokan) #43310
  * add CFRG curves to Web Crypto API (Filip Skokan) #42507
* dns:
  * accept `'IPv4'` and `'IPv6'` for `family` (Antoine du Hamel) #43054
* report:
  * add more heap infos in process report (theanarkh) #43116

PR-URL: #43385
danielleadams added a commit that referenced this pull request Jun 16, 2022
@danielleadams
2022-06-16, Version 18.4.0 (Current)
e339e9c 
Notable changes:

* crypto:
  * remove Node.js-specific webcrypto extensions (Filip Skokan) #43310
  * add CFRG curves to Web Crypto API (Filip Skokan) #42507
* dns:
  * accept `'IPv4'` and `'IPv6'` for `family` (Antoine du Hamel) #43054
* report:
  * add more heap infos in process report (theanarkh) #43116

PR-URL: #43385
danielleadams added a commit that referenced this pull request Jun 16, 2022
@danielleadams
2022-06-16, Version 18.4.0 (Current)
f50a80c 
Notable changes:

* crypto:
  * remove Node.js-specific webcrypto extensions (Filip Skokan) #43310
  * add CFRG curves to Web Crypto API (Filip Skokan) #42507
* dns:
  * accept `'IPv4'` and `'IPv6'` for `family` (Antoine du Hamel) #43054
* report:
  * add more heap infos in process report (theanarkh) #43116

PR-URL: #43385
@chitowncrispy chitowncrispy mentioned this pull request Jul 23, 2022
Closed
1 task
@timacdonald timacdonald mentioned this pull request Jul 25, 2022
Closed
@AdamMajer AdamMajer mentioned this pull request Jul 28, 2022
Closed
@aduh95 aduh95 deleted the revert-net-family-integer branch March 29, 2023 16:00
@aduh95 aduh95 mentioned this pull request Mar 29, 2023
Merged
@sapphi-red sapphi-red mentioned this pull request Aug 10, 2023
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@mcollina mcollina mcollina approved these changes

@Trott Trott Trott approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@mhdawson mhdawson mhdawson approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. tsc-agenda Issues and PRs to discuss during the meetings of the TSC.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Revert the change of network interfaces family from String to Integer