Skip to content

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

Merged
rjsparks merged 11 commits into
mainfrom
fix-uidgid-devcontainer
May 20, 2022
Merged

chore(dev): fix uid/gid mapping for non-vscode devs#3981
rjsparks merged 11 commits into
mainfrom
fix-uidgid-devcontainer

Conversation

@NGPixel

@NGPixel NGPixel commented May 17, 2022

Copy link
Copy Markdown
Member

No description provided.

@larseggert

larseggert commented May 17, 2022

Copy link
Copy Markdown
Collaborator

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

NGPixel commented May 17, 2022

Copy link
Copy Markdown
Member Author

@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

NGPixel commented May 18, 2022

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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
Copy Markdown
Collaborator

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

@NGPixel

NGPixel commented May 19, 2022

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
Collaborator

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

@NGPixel

NGPixel commented May 19, 2022

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Collaborator

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

@rjsparks

Copy link
Copy Markdown
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
Copy Markdown
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

NGPixel commented May 19, 2022

Copy link
Copy Markdown
Member Author

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