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

Remove CGO dependency for user lookup in allocdir #421

Merged
merged 2 commits into from
Dec 1, 2015

Conversation

carlosdp
Copy link
Contributor

os/user's user.Lookup requires that the artifact be compiled with CGO
support enabled. This change instead reads /etc/passwd directly.

The code was acquired from moby/moby#1096

I'm not 100% sure this is necessary. But I was having trouble compiling Nomad to linux/amd64 because I was getting Lookup is not implmented for linux/amd64, so I'm gunna throw it out here.

This change was adopted by Docker, so it seems like a nice way to not have to mess with CGO when building Nomad.

os/user's user.Lookup requires that the artifact be compiled with CGO
support enabled. This change instead reads /etc/passwd directly.

The code was acquired from moby/moby#1096
@mitchellh
Copy link
Contributor

Funny enough we actually have a much more comprehensive library to do this in a cross-platform way: https://github.com/mitchellh/go-homedir (this is used in almost every one of our other projects)

Let's use that instead. Thanks for pointing it out!

@dadgar
Copy link
Contributor

dadgar commented Nov 16, 2015

@mitchellh: The user lookup is done to determine the uid/gid of the "nobody" user. I think the library you linked is not intended for that.

@mitchellh
Copy link
Contributor

Ah, it isn't. Darn. I didn't look closely enough.

@carlosdp
Copy link
Contributor Author

I suppose this could be spun out as a separate package too =P Let me know what direction you want to take!

@carlosdp
Copy link
Contributor Author

Any updates on this one?

@dadgar
Copy link
Contributor

dadgar commented Nov 24, 2015

Sorry been trying to get 0.2 and 0.2.1 out. I will test this PR soon.

@dadgar
Copy link
Contributor

dadgar commented Dec 1, 2015

There is one other place we use it:

u, err := user.Lookup(userid)

So lets refactor it so it can be shared by both.

@carlosdp
Copy link
Contributor Author

carlosdp commented Dec 1, 2015

Alright, I'll get on that.

Also replaces user.Lookup in exec driver
@carlosdp
Copy link
Contributor Author

carlosdp commented Dec 1, 2015

@dadgar done, there's a small conflict in the exec driver's imports, probably cleaner git-wise to resolve on PR merge.

@cbednarski cbednarski mentioned this pull request Dec 1, 2015
@cbednarski cbednarski merged commit eeaa84d into hashicorp:master Dec 1, 2015
cbednarski added a commit that referenced this pull request Dec 1, 2015
@dadgar
Copy link
Contributor

dadgar commented Dec 1, 2015

@carlosdp: Thanks!

c4milo added a commit to hooklift/nomad that referenced this pull request Dec 4, 2015
hashicorp#421 removed the last piece
of CGO dependant code. We we can stop building binaries with CGO
enabled now.
@c4milo c4milo mentioned this pull request Dec 4, 2015
@github-actions
Copy link

github-actions bot commented May 1, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants