Copilot commented on code in PR #13597:
URL: https://github.com/apache/cloudstack/pull/13597#discussion_r3577599169
##########
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 -; \
apt-get install -y nodejs; \
cd ui && npm rebuild node-sass && npm install
Review Comment:
`sudo` is used in the NodeSource setup pipeline, but the Dockerfile snippet
doesn’t show `sudo` being installed; Ubuntu base images typically don’t include
it by default, so this can break the build. Since Docker builds usually run as
root, drop `sudo` and pipe directly to `bash`, or explicitly install `sudo`
earlier if it’s required.
##########
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:
Installing both `openjdk-17-jre-headless` and `openjdk-17-jdk` is typically
redundant because the JDK already pulls in the needed runtime; keeping both can
unnecessarily increase image size and build time. Consider removing
`openjdk-17-jre-headless` unless there’s a confirmed need for it independently.
##########
tools/docker/supervisord.conf:
##########
@@ -16,7 +17,8 @@ redirect_stderr=true
user=root
[program:cloudstack-ui]
-command=/bin/bash -c "npm run serve"
+environment=NODE_OPTIONS="--openssl-legacy-provider"
Review Comment:
Enabling the OpenSSL legacy provider can re-enable legacy/weak algorithms
and is generally a workaround for older dependency stacks (e.g., older
webpack/node-sass toolchains). If possible, prefer updating the UI build
dependencies/configuration to avoid requiring the legacy provider; otherwise,
document why this is needed and scope it as narrowly as possible (e.g., only
for the build step, not the long-running `npm run serve` process).
##########
.github/workflows/docker-cloudstack-simulator.yml:
##########
Review Comment:
Adding `docker-image-build` as a push trigger will run this workflow on
every push to that branch, increasing CI load/cost. If this branch trigger is
temporary for debugging, consider removing it before merge or gating it behind
`workflow_dispatch`/path filters to limit when Docker builds run.
##########
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:
`MARVIN_FILE` is unquoted and `find` can return multiple matches (or paths
with whitespace), which can cause `pip3 install` to receive unintended
arguments. Prefer selecting a single deterministic match (e.g., first result)
and quoting the variable when installing.
--
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]
