Update Dockerfile #9

luke · 2025-08-16T13:24:59Z

luke commented

2025-08-16 13:24:59 +00:00

Okay, here's an explanation of the changes proposed in the provided Git diff, formatted for a pull request discussion.

Overall Changes

The Dockerfile is being modified to change the way Node.js is installed and to add npm to the installed packages. The previous method used nvm (Node Version Manager) while the new method uses the official NodeSource package repository.

Line-by-Line Breakdown

FROM ... and LABEL ...: These lines are unchanged, so they are not included in the diff.
ENV PIPX_BIN_DIR=/usr/local/bin: This line is unchanged, so it is not included in the diff.
RUN wget -q -O- https://raw.githubusercontent.com/nvm-sh/nvm/master/install.sh | bash && \ ...
- REMOVED: This entire block is being removed. It was responsible for installing nvm and then using nvm to install Node.js version 24.
RUN curl -sL https://deb.nodesource.com/setup_24.x -o /tmp/nodesource_setup.sh && \ chmod +x /tmp/nodesource_setup.sh && /tmp/nodesource_setup.sh && \ add-apt-repository ppa:longsleep/golang-backports && \ apt-get update && \ apt-get install -y golang-go pipx && \ apt-get clean && \ pipx install poetry && \ pipx install uv
- MODIFIED: This line has been modified to install Node.js and npm using apt-get.
  - The original line ended with apt-get install -y golang-go pipx &&. This is now apt-get install -y golang-go pipx nodejs npm &&.
  - The curl command downloads a script from NodeSource to setup the repository.
  - The chmod +x command makes the downloaded script executable.
  - /tmp/nodesource_setup.sh executes the downloaded script to configure the apt repository for Node.js.
  - The nodejs package contains the Node.js runtime, and npm is installed alongside it.

Rationale

Switch from nvm to NodeSource: nvm is a great tool for local development environments, allowing users to easily switch between different Node.js versions. However, for a Docker image, it's generally simpler and more predictable to install Node.js directly from a package manager (like apt), ensuring a consistent environment. NodeSource provides official packages, making this approach safer and more reliable than relying on a shell script to manage the node installation.
Explicit npm installation: While npm often comes bundled with Node.js installations, explicitly installing it via apt-get ensures that it's present and avoids potential issues where it might be missing or not correctly configured.

Potential Concerns

Node.js Version: The changes specifically target Node.js 24.x. It is important to confirm that Node.js 24.x meets the requirements.
Dependencies: This change could potentially introduce dependency conflicts.

Okay, here's an explanation of the changes proposed in the provided Git diff, formatted for a pull request discussion. **Overall Changes** The Dockerfile is being modified to change the way Node.js is installed and to add `npm` to the installed packages. The previous method used `nvm` (Node Version Manager) while the new method uses the official NodeSource package repository. **Line-by-Line Breakdown** * **`FROM ...` and `LABEL ...`**: These lines are unchanged, so they are not included in the diff. * **`ENV PIPX_BIN_DIR=/usr/local/bin`**: This line is unchanged, so it is not included in the diff. * **`RUN wget -q -O- https://raw.githubusercontent.com/nvm-sh/nvm/master/install.sh | bash && \ ...`** * **REMOVED**: This entire block is being removed. It was responsible for installing `nvm` and then using `nvm` to install Node.js version 24. * **`RUN curl -sL https://deb.nodesource.com/setup_24.x -o /tmp/nodesource_setup.sh && \ chmod +x /tmp/nodesource_setup.sh && /tmp/nodesource_setup.sh && \ add-apt-repository ppa:longsleep/golang-backports && \ apt-get update && \ apt-get install -y golang-go pipx && \ apt-get clean && \ pipx install poetry && \ pipx install uv`** * **MODIFIED**: This line has been modified to install Node.js and npm using `apt-get`. * The original line ended with `apt-get install -y golang-go pipx &&`. This is now `apt-get install -y golang-go pipx nodejs npm &&`. * The `curl` command downloads a script from NodeSource to setup the repository. * The `chmod +x` command makes the downloaded script executable. * `/tmp/nodesource_setup.sh` executes the downloaded script to configure the apt repository for Node.js. * The `nodejs` package contains the Node.js runtime, and `npm` is installed alongside it. **Rationale** * **Switch from `nvm` to NodeSource:** `nvm` is a great tool for local development environments, allowing users to easily switch between different Node.js versions. However, for a Docker image, it's generally simpler and more predictable to install Node.js directly from a package manager (like `apt`), ensuring a consistent environment. NodeSource provides official packages, making this approach safer and more reliable than relying on a shell script to manage the node installation. * **Explicit `npm` installation:** While `npm` often comes bundled with Node.js installations, explicitly installing it via `apt-get` ensures that it's present and avoids potential issues where it might be missing or not correctly configured. **Potential Concerns** * **Node.js Version:** The changes specifically target Node.js 24.x. It is important to confirm that Node.js 24.x meets the requirements. * **Dependencies:** This change could potentially introduce dependency conflicts.

luke added 1 commit 2025-08-16 13:25:00 +00:00

Update Dockerfile 6ef0924064

ai-review-bot commented

2025-08-16 13:25:09 +00:00

Decision: Reject
Change Score: 2/10

This change involves modifying the Dockerfile to install Node.js 24 and some other tools. The original approach used nvm, which can be problematic within Docker due to shell initialization complexities and user context. The updated approach uses the NodeSource repository, which is generally a better practice. However, there are still issues.

Problems:

You are adding the longsleep/golang-backports PPA but not using it. This is unnecessary and bloats the image.
Installing both nodejs and npm via apt-get is redundant as npm comes bundled with nodejs, leading to potential version conflicts. Also, you are installing node with apt, but installing other python dependencies with pipx.
Installing node 24, which is a development release, is a BAD idea! You should NEVER use pre-release software in production!

Suggestions:

Remove the add-apt-repository line and the golang-go package unless you actually need a different version of Go.
Just install nodejs using apt, that includes npm.
Install a stable, production-ready version of Node.js.
Also, consider using multi-stage builds to reduce the size of your image.

Here's a revised version of the Dockerfile:

FROM ubuntu:latest

LABEL maintainer="Luke Tainton <luke@tainton.uk>"

ENV PIPX_BIN_DIR=/usr/local/bin

RUN curl -sL https://deb.nodesource.com/setup_20.x -o /tmp/nodesource_setup.sh && \
    chmod +x /tmp/nodesource_setup.sh && \
    /tmp/nodesource_setup.sh && \
    apt-get update && \
    apt-get install -y nodejs golang-go pipx && \
    apt-get clean && \
    pipx install poetry && \
    pipx install uv

**Decision: Reject** **Change Score: 2/10** This change involves modifying the Dockerfile to install Node.js 24 and some other tools. The original approach used `nvm`, which can be problematic within Docker due to shell initialization complexities and user context. The updated approach uses the NodeSource repository, which is generally a better practice. However, there are still issues. Problems: 1. You are adding the `longsleep/golang-backports` PPA but not using it. This is unnecessary and bloats the image. 2. Installing both `nodejs` and `npm` via `apt-get` is redundant as npm comes bundled with nodejs, leading to potential version conflicts. Also, you are installing node with apt, but installing other python dependencies with pipx. 3. Installing node 24, which is a development release, is a BAD idea! You should NEVER use pre-release software in production! Suggestions: 1. Remove the `add-apt-repository` line and the `golang-go` package unless you actually need a different version of Go. 2. Just install `nodejs` using apt, that includes `npm`. 3. Install a stable, production-ready version of Node.js. 4. Also, consider using multi-stage builds to reduce the size of your image. Here's a revised version of the Dockerfile: ```dockerfile FROM ubuntu:latest LABEL maintainer="Luke Tainton <luke@tainton.uk>" ENV PIPX_BIN_DIR=/usr/local/bin RUN curl -sL https://deb.nodesource.com/setup_20.x -o /tmp/nodesource_setup.sh && \ chmod +x /tmp/nodesource_setup.sh && \ /tmp/nodesource_setup.sh && \ apt-get update && \ apt-get install -y nodejs golang-go pipx && \ apt-get clean && \ pipx install poetry && \ pipx install uv ```

luke closed this pull request

2025-09-21 13:59:46 +00:00

luke deleted branch luke-patch-1

2025-09-21 13:59:50 +00:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: repos/runner-ubuntu-custom#9