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

Node with JDK17 #9

Draft
wants to merge 6 commits into
base: trunk
Choose a base branch
from
Draft

Node with JDK17 #9

wants to merge 6 commits into from

Conversation

wzieba
Copy link

@wzieba wzieba commented Feb 2, 2024

Description

This PR adds JDK 17, Gradle 8.2.1 and npm to the image. For more context about certain decisions, please see commit messages.

Used in: wordpress-mobile/react-native-libraries-publisher#36

This is probably not gonna to be merged, similarly to #7

oguzkocer and others added 6 commits February 2, 2024 13:42
Those packages seems to be broken via apt-get. Also, we install them via `sdkmanager` in next steps.
It has preinstalled JDK17 (which was problematic to install on Debian). Also, we don't have to cache Gradle because it's preinstalled.
I'm not sure how it was working previously, but now it was failing with `nvm command not found`
Copy link

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test the Docker image per se but the changes matches what is described in the individual commit messages and in the PR description, so as long as it works as expected when used in wordpress-mobile/react-native-libraries-publisher#36 that should be ok.

Up to you to publish the image and merge the PR, or not publish it (ref by sha1 only) and close that PR like was done with #7 — both works for me, as long has we have the history of the Dockerfile used for a given sha1 (here I'm assuming 9d1710c7d74d1349d84e94465fe7b3a90216e92a, since that's the one used in wordpress-mobile/react-native-libraries-publisher#36)


RUN rm /bin/sh && ln -s /bin/bash /bin/sh

# Increase the file watcher limit for node. This should not be necessary for CI builds since watchers should be disabled, but it can be useful when running this image in a local environment.
RUN echo fs.inotify.max_user_watches=524288 | tee -a /etc/sysctl.conf

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a specific number 😅

RUN curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.7/install.sh | bash \
&& source "$HOME/.nvm/nvm.sh" \
&& nvm install $NODE_VERSION
ENV PATH="//root/.nvm/versions/node/$NODE_VERSION/bin:${PATH}"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ENV PATH="//root/.nvm/versions/node/$NODE_VERSION/bin:${PATH}"
ENV PATH="/root/.nvm/versions/node/$NODE_VERSION/bin:${PATH}"

&& nvm install $NODE_VERSION
ENV PATH="//root/.nvm/versions/node/$NODE_VERSION/bin:${PATH}"

RUN which npm

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious: what made npm be installed in that Docker image? Is it installed as part of the nvm install script (curl -o- …/nvm/v0.39.7/install.sh)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants