-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
base: main
Are you sure you want to change the base?
Conversation
Added comments for alternative commands to speed up installations in China.
Summary of ChangesHello @hass-demacia, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces helpful commented-out alternative commands within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request aims to help users in China by providing alternative mirrors for package installations. While this is a good initiative, the implementation can be improved for better maintainability and correctness.
I've made the following suggestions:
- For
npm,pip, andapt-getinstallations, I've recommended using Docker build arguments (ARG) instead of commented-out code. This is a cleaner, more robust, and idiomatic way to handle environment-specific configurations like package mirrors. - The change for
pipinstallation was incorrect as it introduced a redundant command, creating an unnecessary layer in the Docker image. I've provided a fix for this that also uses a build argument.
By adopting these suggestions, the Dockerfile will be more flexible and easier to use for developers in all regions without needing to modify the file itself.
| # ** 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 |
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 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 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 |
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.
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
| # ** 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 \ |
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.
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 \
Added comments for alternative commands to speed up installations in China.