test: add fix so that test exits if port 42 is unprivileged by 7suyash7 · Pull Request #45904 · nodejs/node · GitHub
Skip to content

test: add fix so that test exits if port 42 is unprivileged#45904

Merged
nodejs-github-bot merged 6 commits into
nodejs:mainfrom
7suyash7:test
Jan 18, 2023
Merged

test: add fix so that test exits if port 42 is unprivileged#45904
nodejs-github-bot merged 6 commits into
nodejs:mainfrom
7suyash7:test

Conversation

@7suyash7

@7suyash7 7suyash7 commented Dec 18, 2022

Copy link
Copy Markdown
Contributor

fixes: #45838

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Dec 18, 2022
@bnoordhuis

bnoordhuis commented Dec 18, 2022

Copy link
Copy Markdown
Member

@7suyash7

7suyash7 commented Dec 19, 2022

Copy link
Copy Markdown
Contributor Author

Okay, I'll make these changes

@7suyash7

7suyash7 commented Dec 19, 2022

Copy link
Copy Markdown
Contributor Author

@bnoordhuis is this fine?

bnoordhuis
bnoordhuis reviewed Dec 19, 2022
View reviewed changes
Comment thread test/parallel/test-cluster-bind-privileged-port.js Outdated
Comment thread test/parallel/test-cluster-bind-privileged-port.js Outdated
Comment thread test/parallel/test-cluster-bind-privileged-port.js
@7suyash7

7suyash7 commented Dec 19, 2022

Copy link
Copy Markdown
Contributor Author

added these changes @bnoordhuis

@7suyash7

7suyash7 commented Dec 21, 2022

Copy link
Copy Markdown
Contributor Author

Can you try merging again? Had some lint errors, hopefully, they are fixed now. @bnoordhuis

bnoordhuis
bnoordhuis reviewed Dec 21, 2022
View reviewed changes
Comment thread test/parallel/test-cluster-bind-privileged-port.js Outdated
Comment thread test/parallel/test-cluster-bind-privileged-port.js Outdated
Comment thread test/parallel/test-cluster-bind-privileged-port.js Outdated
Comment thread test/parallel/test-cluster-bind-privileged-port.js
@7suyash7

7suyash7 commented Dec 21, 2022

Copy link
Copy Markdown
Contributor Author

Can you please take a look?

bnoordhuis
bnoordhuis reviewed Dec 22, 2022
View reviewed changes
Comment thread test/parallel/test-cluster-bind-privileged-port.js Outdated
benjamingr
benjamingr reviewed Dec 22, 2022
View reviewed changes
Comment thread test/parallel/test-cluster-bind-privileged-port.js
const { execSync } = require('child_process');

if (common.isLinux) {
const sysctlOutput = execSync('sysctl net.ipv4.ip_unprivileged_port_start').toString();

@benjamingr benjamingr Dec 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add a comment?

@7suyash7 7suyash7 Dec 22, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, should I add one now? (PR is closing to merging I think...)

@benjamingr benjamingr Dec 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

PR is closing to merging I think...

I am not sure what you mean here, you can still push changes and I'll re-run CI after :]

If you prefer to put it in another change to practice making changes in Node.js as a new contributor - that's also fine with me.

@7suyash7 7suyash7 Dec 24, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would like it if I could put it in another change so I can practice making changes, that would be really helpful!

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 22, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 22, 2022
@nodejs-github-bot

nodejs-github-bot commented Dec 22, 2022

Copy link
Copy Markdown
Collaborator

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

@lpinca lpinca added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jan 10, 2023
@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 18, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 18, 2023
@nodejs-github-bot nodejs-github-bot merged commit 60cc1ba into nodejs:main Jan 18, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 18, 2023

Copy link
Copy Markdown
Collaborator

RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
@7suyash7 @RafaelGSS
test: add fix so that test exits if port 42 is unprivileged
4f491d3 
PR-URL: #45904
Fixes: #45838
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
@7suyash7 @juanarbol
test: add fix so that test exits if port 42 is unprivileged
c1f2c8d 
PR-URL: #45904
Fixes: #45838
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
@7suyash7 @juanarbol
test: add fix so that test exits if port 42 is unprivileged
896027c 
PR-URL: #45904
Fixes: #45838
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes
@bnoordhuis bnoordhuis bnoordhuis approved these changes
@lpinca lpinca lpinca approved these changes

Assignees

No one assigned

Labels

commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

test: check net.ipv4.ip_unprivileged_port_start in parallel/test-cluster-bind-privileged-port

5 participants

@7suyash7 @bnoordhuis @nodejs-github-bot @benjamingr @lpinca