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

chore(dev): fix uid/gid mapping for non-vscode devs #3981

Merged
merged 11 commits into from
May 20, 2022

Conversation

NGPixel
Copy link
Member

@NGPixel NGPixel commented May 17, 2022

No description provided.

@larseggert
Copy link
Collaborator

larseggert commented May 17, 2022

I still see these errors when I build a new container with run -r:

 => ERROR [ 4/28] RUN groupmod --gid 20 vscode     && usermod --uid 501 --gid 20 vscode     && chown -R 501:20 /home/vscode                          0.3s
------
 > [ 4/28] RUN groupmod --gid 20 vscode     && usermod --uid 501 --gid 20 vscode     && chown -R 501:20 /home/vscode:
#0 0.224 groupmod: GID '20' already exists
------

and later on

Compiling native node packages...
➤ YN0000: ┌ Resolution step
➤ YN0060: │ @parcel/optimizer-image@npm:2.5.0 provides @parcel/core (p53bfc) with version 2.4.1, which doesn't satisfy what @parcel/workers requests
➤ YN0060: │ @parcel/types@npm:2.5.0 provides @parcel/core (p8f883) with version 2.4.1, which doesn't satisfy what @parcel/fs requests
➤ YN0060: │ @parcel/types@npm:2.5.0 provides @parcel/core (p9e6d4) with version 2.4.1, which doesn't satisfy what @parcel/workers requests
➤ YN0060: │ @parcel/types@npm:2.5.0 provides @parcel/core (p218a2) with version 2.4.1, which doesn't satisfy what @parcel/cache requests
➤ YN0060: │ @parcel/types@npm:2.5.0 provides @parcel/core (p17919) with version 2.4.1, which doesn't satisfy what @parcel/package-manager requests
➤ YN0000: │ Some peer dependencies are incorrectly met; run yarn explain peer-requirements <hash> for details, where <hash> is the six-letter p-prefixed code
➤ YN0000: └ Completed
➤ YN0000: ┌ Fetch step
➤ YN0000: └ Completed in 1s 64ms
➤ YN0000: ┌ Link step
➤ YN0000: │ ESM support for PnP uses the experimental loader API and is therefore experimental
➤ YN0007: │ lmdb@npm:2.2.4 must be built because it never has been before or the last one failed
➤ YN0007: │ msgpackr-extract@npm:1.1.4 must be built because it never has been before or the last one failed
➤ YN0007: │ @parcel/watcher@npm:2.0.5 must be built because it never has been before or the last one failed
➤ YN0007: │ cypress@npm:9.6.1 must be built because it never has been before or the last one failed
➤ YN0000: └ Completed in 23s 94ms
➤ YN0000: Done with warnings in 24s 394ms

and then finally

rm: cannot remove '/usr/local/share/nvm/current': Permission denied

But despite the above, the container works again!

@NGPixel NGPixel requested a review from rjsparks May 17, 2022 21:26
@NGPixel
Copy link
Member Author

NGPixel commented May 17, 2022

@larseggert I'm looking into the first one (groupmod error).

For the yarn warnings, they can be ignored. Some parcel packages are simply using different versions of the same dependency and yarn is warning us about it. It doesn't prevent the packages from installing and being used.

As for the last one, that error is coming from bash / zsh directly (running zsh directly outputs the same thing). It seems to come from some nvm plugin being loaded with the shell... Looking into it.

@NGPixel NGPixel changed the title chore: fix uid/gid mapping for non-vscode devs chore(dev): fix uid/gid mapping for non-vscode devs May 18, 2022
@NGPixel
Copy link
Member Author

NGPixel commented May 18, 2022

The groupmod error should be silenced now (group already exists and mapped) and allow the build to continue.

I wasn't able to figure out where exactly is the nvm command is triggered (happens when calling zsh, so there's a hook somewhere, but it's not in .zshrc...). That error doesn't affect anything though, it's a superfluous command to set the current node version (which is already set).

@larseggert
Copy link
Collaborator

I think this change causes python modules to be installed in /home/vscode/.local, which is not in the PATH. That causes tools like pyang to not be found, causing test suite errors.

@larseggert
Copy link
Collaborator

I think we should enable CI for changes like this to catch these issues.

@NGPixel
Copy link
Member Author

NGPixel commented May 19, 2022

@larseggert It wouldn't catch anything since the dev and test images are completely different. The test image runs as root (while the dev image does not) and is based off the leaner python image (rather than the vscode dev python image).

@larseggert
Copy link
Collaborator

Ah. We'll, then you should run the tests manually :-)

@NGPixel
Copy link
Member Author

NGPixel commented May 19, 2022

I just ran the tests and didn't get any pyang errors. I can also see /home/vscode/.local/bin in PATH and run pyang -v just fine.

Have you rebuilt the container?

@larseggert
Copy link
Collaborator

I think I did. Maybe merge this and see if issues remain?

@rjsparks
Copy link
Member

I think it would be good if changes to the dev container configs triggered a separate kind of CI test if possible - one that would be slow, of course, because it should build the affected container(s) from scratch and exercise them to the point that at least manage.py check succeeds.

@rjsparks
Copy link
Member

The dev container is now leaking permission changes out onto the host, which is really not ok for many reasons, but just to point at one:

root@crawl:~/workspace/datatracker# ls -ld .
drwxr-xr-x 21 1000 1000 4096 May 19 13:40 .
root@crawl:~/workspace/datatracker# git status
fatal: unsafe repository ('/root/workspace/datatracker' is owned by someone else)
To add an exception for this directory, call:

	git config --global --add safe.directory /root/workspace/datatracker

@NGPixel
Copy link
Member Author

NGPixel commented May 19, 2022

The permissions change leak happens when running as root on the host, which no dev should be in theory?

It happens because of this line:
https://github.com/ietf-tools/datatracker/blob/fix-uidgid-devcontainer/docker/scripts/app-init.sh#L11

which I guess could technically be removed if we assume the host user id and container user id are the same...

@rjsparks rjsparks merged commit b6186f1 into main May 20, 2022
@NGPixel NGPixel deleted the fix-uidgid-devcontainer branch May 21, 2022 04:18
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2022
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.

3 participants