★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/28219Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

test: added Ubuntu error code support to test-process-uid-gid.js #28219

Closed
wants to merge 1 commit into from
+1 −1

Conversation

@ssample812
Copy link
Contributor

commented Jun 14, 2019

This change adds exception handling for the error code received when process.setgid('nobody') is called on Ubuntu, where the standard 'nobody' group from UNIX systems is named 'nogroup'. Coverage had previously been added for the error message, but with Ubuntu 18 this message has changed. Using the error code, which is intended to be unchanging, will hopefully prevent the need for updates in the future.

Refs: #19594

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test label Jun 14, 2019

@sam-github

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 14, 2019

The change looks reasonable, other than failing linting (please run make test locally on your branch).

I'm a bit puzzled why we don't see this, I have developed on Ubuntu for years, and the test passes for me. I have a nobody in /etc/passwd and nogroup in /etc/group. 18 isn't an ubuntu version number, what version of Ubuntu are you running?

@ssample812 ssample812 force-pushed the ssample812:ssample branch from f9b07d6 to 8532404 Jun 14, 2019

@ssample812

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jun 14, 2019 •
edited

I built Node v12.4.0 from the source in a Dockerfile using a base image of ubuntu:bionic, which is Ubuntu 18.04.2. It very well could be an issue on my end because I'm pretty inexperienced with Docker and Node in general, but this was my only failing test

@ssample812 ssample812 force-pushed the ssample812:ssample branch from 8532404 to f3c1baa Jun 14, 2019

@sam-github
sam-github reviewed Jun 14, 2019
View changes
test/parallel/test-process-uid-gid.js Show resolved Hide resolved

@ssample812 ssample812 force-pushed the ssample812:ssample branch from 0442e60 to 2e2e93c Jun 14, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Jun 14, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/23891/
@sam-github

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 14, 2019

I pushed a commit on top of yours to just use .code. I don't think the original .message string is possible, its not a current string. We never noticed it was wrong, I assume, because none of our ci machines or developers have this setup. Thanks.

Lets see how it goes in ci. If you feel like rebasing this and squashing all three commits together with a new message, please do so, Otherwise, if you aren't comfortable enough with git to do that, I can do it when I land this.
@ssample812

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jun 14, 2019 •
edited

Thank you for the input. The original .message string confused me as well, but I assumed it was because the other contributor was running Ubuntu 16.04.1 and the message had changed in version 18.04.2. I agree the change to a .code check is the best move. This is my first time contributing to OSS on github and I have learned a lot. If you could squash the commits together, that would be great! Thanks
= @sam-github
test: use .code for error in setgid 
When the 'nobody' user is missing use .code to detect this, its more
robust than than the .message string.

Refs: #19594
Loading status checks…
fbbc862

@sam-github sam-github force-pushed the ssample812:ssample branch from 78e43be to fbbc862 Jun 18, 2019

@sam-github
sam-github approved these changes Jun 18, 2019
View changes
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Jun 18, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/23962/
@sam-github

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 18, 2019

@nodejs/collaborators first time contribution, needs another review, please.
@lpinca
lpinca approved these changes Jun 18, 2019
View changes
@ZYSzys
ZYSzys approved these changes Jun 18, 2019
View changes
@Trott
Trott approved these changes Jun 18, 2019
View changes
@trivikr
trivikr approved these changes Jun 18, 2019
View changes
@cjihrig
cjihrig approved these changes Jun 19, 2019
View changes
@hiroppy
hiroppy approved these changes Jun 19, 2019
View changes
@antsmartian
antsmartian approved these changes Jun 20, 2019
View changes
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 20, 2019

Landed in e57bf47.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

@Trott Trott closed this Jun 20, 2019

Trott added a commit to Trott/io.js that referenced this pull request Jun 20, 2019
= @Trott
test: use .code for error in setgid 
When the 'nobody' user is missing use .code to detect this, its more
robust than than the .message string.

Refs: nodejs#19594

PR-URL: nodejs#28219
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
e57bf47
@sam-github

This comment was marked as outdated.

Sign in to view
Copy link
Member

commented Jun 20, 2019

@ssample812 Your commit lacks any authorship information, its Author: = <=>, our landing tool says:

⚠ GitHub cannot link the author of 'test: use .code for error in setgid' to their GitHub account.
⚠ Please suggest them to take a look at https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork

I would add it for you, but you have no email address on your github page, can you give it to me, or follow the directions above, or do git commit --amend --author="Author Name <email@address.com>" and repush?
@sam-github

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 20, 2019

Ignore that last comment, it landed with author = <=>, thanks for the contribution. Please take a look at setting up your local git configuration to include your name and email, it will be useful for all git commits, not just Node.js.
targos added a commit that referenced this pull request Jul 2, 2019
= @targos
test: use .code for error in setgid 
When the 'nobody' user is missing use .code to detect this, its more
robust than than the .message string.

Refs: #19594

PR-URL: #28219
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
c14e4d5
@targos targos referenced this pull request Jul 2, 2019
Merged
@beevelop beevelop referenced this pull request Jul 3, 2019
Closed
This was referenced Jul 3, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sam-github sam-github

@Trott Trott

@antsmartian antsmartian

@lpinca lpinca

@cjihrig cjihrig

@hiroppy hiroppy

@trivikr trivikr

@ZYSzys ZYSzys

Assignees
No one assigned
Labels
test
Projects
None yet
Milestone
No milestone
10 participants
@ssample812 @sam-github @nodejs-github-bot @Trott @antsmartian @lpinca @cjihrig @hiroppy @trivikr @ZYSzys
You can’t perform that action at this time.