-
Notifications
You must be signed in to change notification settings - Fork 851
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
os/user.Lookup #4217
base: dev
Are you sure you want to change the base?
os/user.Lookup #4217
Conversation
4826227
to
3c925ed
Compare
@deadprogram any thoughts on this? |
@deadprogram do you have any recommendations on what to improve or does this look good to you? |
@aykevl maybe you can have a short look at this? |
ping for review |
Just waiting for #4027 to land before any more changes to this part of the code. |
6c8cf91
to
d9ef0e1
Compare
for some reason smoke test |
That's #4206 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this was mostly copied from upstream Go, correct?
The code looks reasonable, though I would prefer if the test was actually being run (you can add it to GNUmakefile, see TEST_PACKAGES_FAST
and related variables). Also, what about MacOS/Windows/baremetal support? Does it just fail to compile? I'm worried we might be breaking some existing programs that currently work.
Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
Signed-off-by: leongross <leon.gross@9elements.com>
Not quite sure why the windows build fails here: |
Add support for User lookup via
os/user.Lookup()