Skip to content

Add comments for speeding up installations in China #379

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add comments for speeding up installations in China #379

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ FROM node:20-alpine3.22 AS node_base
FROM node_base AS node_deps
WORKDIR /app
COPY package.json package-lock.json ./
# ** use this instead for speeding up, if in China
# RUN npm ci --legacy-peer-deps --registry=https://registry.npmmirror.com
RUN npm ci --legacy-peer-deps
Comment on lines +11 to 13
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

While adding a commented-out alternative is helpful, a more robust and cleaner approach for handling alternative package registries is to use build arguments. This avoids having commented-out code in the Dockerfile and makes it easier for users to switch registries without editing the file. The if statement ensures the default behavior is preserved if no mirror is specified.

# Optionally, specify an npm registry mirror, e.g., --build-arg NPM_REGISTRY=https://registry.npmmirror.com
ARG NPM_REGISTRY
RUN if [ -n "$NPM_REGISTRY" ]; then \
        npm ci --legacy-peer-deps --registry="$NPM_REGISTRY"; \
    else \
        npm ci --legacy-peer-deps; \
    fi


FROM node_base AS node_builder
Expand All @@ -27,6 +29,8 @@ WORKDIR /app
RUN python -m venv /opt/venv
ENV PATH="/opt/venv/bin:$PATH"
COPY api/requirements.txt ./api/
# ** use this instead for speeding up, if in China
RUN pip install -i https://mirrors.aliyun.com/pypi/simple/ --no-cache -r api/requirements.txt
RUN pip install --no-cache -r api/requirements.txt
Comment on lines +32 to 34
Copy link
Contributor

Choose a reason for hiding this comment

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

high

This change introduces a second RUN pip install ... command. This is redundant because the first pip install (using the Aliyun mirror) will have already installed the packages. This creates an unnecessary layer in the Docker image and can be confusing. The likely intention was to provide a commented-out alternative, similar to the npm change.

A better approach is to use a build argument for the PyPI mirror. This makes the Dockerfile cleaner, more maintainable, and avoids the redundant command. The if statement preserves the default behavior when no mirror is provided.

# Optionally, specify a PyPI mirror, e.g., --build-arg PYPI_INDEX_URL=https://mirrors.aliyun.com/pypi/simple/
ARG PYPI_INDEX_URL
RUN if [ -n "$PYPI_INDEX_URL" ]; then \
        pip install -i "$PYPI_INDEX_URL" --no-cache -r api/requirements.txt; \
    else \
        pip install --no-cache -r api/requirements.txt; \
    fi


# Use Python 3.11 as final image
Expand All @@ -36,6 +40,10 @@ FROM python:3.11-slim
WORKDIR /app

# Install Node.js and npm

# ** use this instead for speeding up, if in China
# RUN sed -i 's@deb.debian.org@mirrors.aliyun.com@g' /etc/apt/sources.list.d/debian.sources \
# && apt-get update && apt-get install -y \
RUN apt-get update && apt-get install -y \
Comment on lines +44 to 47
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Similar to the npm and pip installations, using a build argument for the Debian mirror is a cleaner and more maintainable approach than having commented-out code. This allows users to easily specify a mirror at build time without modifying the Dockerfile. The if statement ensures that the sed command only runs if a mirror is provided, making it more robust.

# Optionally, specify a Debian mirror, e.g., --build-arg DEBIAN_MIRROR=mirrors.aliyun.com
ARG DEBIAN_MIRROR
RUN if [ -n "$DEBIAN_MIRROR" ]; then \
        sed -i "s|deb.debian.org|$DEBIAN_MIRROR|g" /etc/apt/sources.list.d/debian.sources; \
    fi && \
    apt-get update && apt-get install -y \

curl \
gnupg \
Expand Down