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.

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

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
@sam-github

This comment has been minimized.

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.

Copy link
Contributor Author

commented Jun 14, 2019

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

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

@nodejs-github-bot

This comment has been minimized.

@sam-github

This comment has been minimized.

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.

Copy link
Contributor Author

commented Jun 14, 2019

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

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

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

@nodejs-github-bot

This comment has been minimized.

@sam-github

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@nodejs/collaborators first time contribution, needs another review, please.

@lpinca
lpinca approved these changes Jun 18, 2019
@ZYSzys
ZYSzys approved these changes Jun 18, 2019
@Trott
Trott approved these changes Jun 18, 2019
@Trott

This comment has been minimized.

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
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>
@sam-github

This comment was marked as outdated.

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.

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
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>
@targos targos referenced this pull request Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.