-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[readme] add docker tips #2318
[readme] add docker tips #2318
Conversation
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.
https://github.com/nvm-sh/nvm/blob/master/Dockerfile / https://hub.docker.com/r/peterdavehello/nvm exists, perhaps it's better to point people to that?
@ljharb , I'm open to your suggestion, but those docker files fundamentally are for a different intent. Those are for running a first-class NVM image, where the above instructions teach the user how to make any image a NVM image. Implicitly, those docker files do the required work, but they do so with noise interspersed, and without a clear focus on the requirements to make NVM work in any image. Perhaps simply sprinkling on some extra verbage with reference to those would be useful? I don't think references to those alone are the correct solve. |
README.md
Outdated
# patch PATH | ||
ENV PATH $NVM_DIR/versions/node/v$NODE_VERSION/bin:$PATH |
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.
this does not belong in docs; nobody should be manually editing the PATH, only sourcing nvm.sh
and letting nvm do it.
# patch PATH | |
ENV PATH $NVM_DIR/versions/node/v$NODE_VERSION/bin:$PATH |
it's also not necessary here since nvm install
handles it in the next RUN statement.
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.
doesn't each run in docker happen in a new shell? iirc, mutating your env in a RUN doesn't persist in downstream runs
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.
Yes - but instead of hardcoding the PATH, is there a way the RUN that sources nvm.sh
could export the resulting PATH for further RUNs to use?
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.
@ljharb @cdargine ENV PATH="$NVM_DIR/versions/node/v$NODE_VERSION/bin:$PATH"
sholdnt this export the path to resulting run commands ?
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.
he's looking for something like ENV=$(/path/to/nvm env)
I think. Feel free to push to or close this or, btw.
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.
@ljharb, i have been thinking about it, and I don't see a way forward otherwise, which is disappointing. ENV stanzas do not accept child commands, so we are left with only build args, the file system, and RUN blocks as sources of dynamic input, however only build arg inputs can be pulled into the image ENV definition from what i can tell. or, better said, i cannot find a way to dynamically promote content from within a build step to the image ENV. if ENV PATH=$PATH;$(nvm path)
worked, we would have a clean solve. it does not.
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.
In that case, a paragraph in the docs that explains these things clearly (including to me, who has minimal docker experience) might be a good way to resolve this PR?
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 think I found a pretty neat way of installing nvm in a docker container by leveraging the bash environment variable $BASH_ENV
, this way we don't even need to make the bash in the container into an interactive/login shell.
FROM ubuntu:22.10
SHELL ["/bin/bash", "-o", "pipefail", "-c"]
ENV BASH_ENV /root/.env
RUN touch $BASH_ENV
RUN echo '. "'"$BASH_ENV"'"' >> ~/.bashrc # only needed if executing interactively into the container
RUN apt-get update && apt-get install -y curl
RUN curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.2/install.sh | PROFILE="$BASH_ENV" bash
COPY .nvmrc .nvmrc
RUN nvm install
RUN node -v
This way nvm+node is easily available both later in the Dockerfile, and when executing into the container
> echo 19.0.1 > .nvmrc
> docker build -t foo .
...
Step 9/10 : RUN nvm install
Found '//.nvmrc' with version <19.0.1>
...
Now using node v19.0.1 (npm v8.19.2)
Creating default alias: default -> 19.0.1 (-> v19.0.1 *)
...
Step 10/10 : RUN node -v
v19.0.1
> docker run --rm -it --entrypoint bash foo
root@4e5088bdde4d:/# tail -1 ~/.bashrc
. "/root/.env"
root@4e5088bdde4d:/# node -v
v19.0.1
Maybe it's neat enough to fit in the readme?
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.
That does seem pretty compelling.
Do you think that would be a replacement for this PR? If so, please post a link to a branch (NOT a new PR) and i'll pull in the changes. If not, please file a new PR :-)
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.
Yeah I think it could be.
Sure!
https://github.com/blole/nvm/tree/docs/install-in-docker
I tried my hand at updating the Dockerfile in the repo as well, it seems to work like I'd expect, but I'm not sure what people use it for, so I can't really verify that.
- covers installation using BASH_ENV Co-authored-by: Christopher Dieringer <cdaringe@users.noreply.github.com> Co-authored-by: Björn Holm <blollle@gmail.com>
c6cfc3a
to
c20db2a
Compare
8b14f36
to
b77fcec
Compare
Problem
It's common to install node in docker, and using NVM is quite helpful in this regard. Installing and using NVM isn't always. straightforward.
Solution
Make it easier for users to setup nvm in docker by droppin' some docs.
Ref: https://stackoverflow.com/questions/25899912/how-to-install-nvm-in-docker