Copilot commented on code in PR #13597:
URL: https://github.com/apache/cloudstack/pull/13597#discussion_r3577744827
##########
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 includes a JRE; installing
`openjdk-17-jre-headless` as well is redundant and increases image size and
build time. Prefer keeping only `openjdk-17-jdk` (or only the JRE if you don't
need toolchain components like `javac`).
##########
tools/docker/Dockerfile:
##########
@@ -61,12 +62,10 @@ RUN find /var/lib/mysql -type f -exec touch {} \; && \
mysql -e "ALTER USER 'root'@'localhost' IDENTIFIED WITH
caching_sha2_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:
Using `sudo` in Docker builds is unnecessary (build steps typically run as
root) and will fail if `sudo` isn't installed. Replace this with a root-safe
approach (e.g., pipe to `bash` directly), or switch to the recommended
NodeSource keyring/apt repo configuration without `curl | bash` to reduce
supply-chain risk.
##########
tools/docker/supervisord.conf:
##########
@@ -16,6 +17,7 @@ redirect_stderr=true
user=root
[program:cloudstack-ui]
+environment=NODE_OPTIONS="--openssl-legacy-provider"
Review Comment:
`--openssl-legacy-provider` is intended for older Node/OpenSSL compatibility
and may be unsupported or a hard error on newer Node major versions. With
Node.js 24, this can prevent the UI process from starting. Prefer removing it
and fixing the underlying dependency incompatibility, or make it conditional
(e.g., only set for older Node versions / only when required).
##########
.github/workflows/docker-cloudstack-simulator.yml:
##########
@@ -21,6 +21,7 @@ on:
push:
branches:
- main
+ - docker-image-build
Review Comment:
Triggering this workflow on a dedicated branch can create ongoing CI noise
if the branch is long-lived or frequently rebased. If this is intended only for
ad-hoc verification, consider using `workflow_dispatch` or narrowing triggers
with `paths:` filters to Docker-related files (or removing the branch trigger
once the build fixes are merged).
##########
tools/docker/Dockerfile:
##########
@@ -61,12 +62,10 @@ RUN find /var/lib/mysql -type f -exec touch {} \; && \
mysql -e "ALTER USER 'root'@'localhost' IDENTIFIED WITH
caching_sha2_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:
`pip3 install --break-system-packages` can make the image harder to
maintain/upgrade because it mutates system Python site-packages and can
conflict with OS-managed modules. Prefer installing Marvin into an isolated
environment (venv/pipx) or a dedicated prefix. Also, `find` can return multiple
matches; consider making the selection deterministic (e.g., pick the newest or
`-print -quit`) to avoid non-reproducible builds.
--
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]
