Copilot commented on code in PR #13814:
URL: https://github.com/apache/cloudstack/pull/13814#discussion_r3749081042
##########
tools/docker/Dockerfile:
##########
@@ -61,12 +62,10 @@ RUN find /var/lib/mysql -type f -exec touch {} \; && \
mysql -e "ALTER USER 'root'@'localhost' IDENTIFIED WITH
mysql_native_password by ''" --connect-expired-password; \
mvn -Pdeveloper -pl developer -Ddeploydb; \
mvn -Pdeveloper -pl developer -Ddeploydb-simulator; \
- MARVIN_FILE=`find /root/tools/marvin/dist/ -name "Marvin*.tar.gz"`; \
- rm -rf /usr/bin/x86_64-linux-gnu-gcc && \
- ln -s /usr/bin/gcc-10 /usr/bin/x86_64-linux-gnu-gcc; \
- pip3 install $MARVIN_FILE
+ MARVIN_FILE=`find /root/tools/marvin/dist/ -name "[mM]arvin*.tar.gz"`; \
+ pip3 install $MARVIN_FILE --break-system-packages
-RUN curl -sL https://deb.nodesource.com/setup_14.x | sudo -E bash -; \
+RUN curl -sL https://deb.nodesource.com/setup_24.x | sudo -E bash -; \
Review Comment:
`sudo` is used inside the Docker build step. Docker builds typically run as
`root` and `sudo` is often not installed in minimal images; this can cause the
image build to fail. Drop `sudo -E` and run the script directly (or ensure
`sudo` is installed earlier if you truly need it).
##########
tools/docker/Dockerfile:
##########
@@ -61,12 +62,10 @@ RUN find /var/lib/mysql -type f -exec touch {} \; && \
mysql -e "ALTER USER 'root'@'localhost' IDENTIFIED WITH
mysql_native_password by ''" --connect-expired-password; \
mvn -Pdeveloper -pl developer -Ddeploydb; \
mvn -Pdeveloper -pl developer -Ddeploydb-simulator; \
- MARVIN_FILE=`find /root/tools/marvin/dist/ -name "Marvin*.tar.gz"`; \
- rm -rf /usr/bin/x86_64-linux-gnu-gcc && \
- ln -s /usr/bin/gcc-10 /usr/bin/x86_64-linux-gnu-gcc; \
- pip3 install $MARVIN_FILE
+ MARVIN_FILE=`find /root/tools/marvin/dist/ -name "[mM]arvin*.tar.gz"`; \
+ pip3 install $MARVIN_FILE --break-system-packages
-RUN curl -sL https://deb.nodesource.com/setup_14.x | sudo -E bash -; \
+RUN curl -sL https://deb.nodesource.com/setup_24.x | sudo -E bash -; \
apt-get install -y nodejs; \
Review Comment:
Piping a remote script directly into `bash` during image build is a
supply-chain risk (arbitrary code execution if the endpoint is compromised).
Prefer adding the NodeSource apt repository via signed keyring +
`sources.list.d` entry (or use Ubuntu-packaged Node where feasible) so installs
are verifiable and reproducible.
##########
tools/docker/Dockerfile:
##########
@@ -34,7 +34,8 @@ RUN apt-get -y update && apt-get install -y \
ipmitool \
iproute2 \
maven \
- openjdk-11-jdk \
+ openjdk-17-jre-headless \
+ openjdk-17-jdk \
Review Comment:
`openjdk-17-jdk` already pulls in a JRE; installing both typically adds
unnecessary packages/layers. Consider removing `openjdk-17-jre-headless` unless
you have a specific reason to list it separately.
##########
tools/marvin/setup.py:
##########
@@ -57,7 +57,7 @@
"ipmisim >= 0.7",
"pytz",
"retries",
- "PyCrypt",
+ "pycryptodome",
Review Comment:
Consider adding a lower-bound version constraint for `pycryptodome` (and
documenting why it’s required) to improve reproducibility across environments.
Without pinning, older distro-provided versions could behave differently and
make failures harder to triage.
##########
tools/docker/Dockerfile:
##########
@@ -61,12 +62,10 @@ RUN find /var/lib/mysql -type f -exec touch {} \; && \
mysql -e "ALTER USER 'root'@'localhost' IDENTIFIED WITH
mysql_native_password by ''" --connect-expired-password; \
mvn -Pdeveloper -pl developer -Ddeploydb; \
mvn -Pdeveloper -pl developer -Ddeploydb-simulator; \
- MARVIN_FILE=`find /root/tools/marvin/dist/ -name "Marvin*.tar.gz"`; \
- rm -rf /usr/bin/x86_64-linux-gnu-gcc && \
- ln -s /usr/bin/gcc-10 /usr/bin/x86_64-linux-gnu-gcc; \
- pip3 install $MARVIN_FILE
+ MARVIN_FILE=`find /root/tools/marvin/dist/ -name "[mM]arvin*.tar.gz"`; \
+ pip3 install $MARVIN_FILE --break-system-packages
Review Comment:
Quote the variable expansion when installing the artifact (e.g.,
`\"$MARVIN_FILE\"`) to avoid word-splitting issues if the path ever contains
whitespace. Also, `--break-system-packages` is a last-resort workaround; using
a dedicated virtualenv for Marvin inside the image is more robust and avoids
clobbering system-managed Python packages.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
