Copilot commented on code in PR #13597:
URL: https://github.com/apache/cloudstack/pull/13597#discussion_r3569743365
##########
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 populated via backticks + unquoted `find` output. If `find`
returns multiple matches (or any whitespace), this will be word-split and
passed incorrectly to `pip3`. Prefer `$(...)`, limit to a single match, and
quote the variable; also fail fast when no archive is found.
##########
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:
`npm rebuild node-sass` looks stale here: the UI
`package.json`/`package-lock.json` do not include `node-sass`, so this step is
at best redundant and at worst can fail with newer npm versions. Consider
switching to `npm ci` (lockfile is present) and dropping the rebuild.
##########
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:
`curl` should use `-f` (fail on HTTP errors) so the build doesn't execute an
error page through `bash`. Also `sudo` is unnecessary in a Docker build step
(the default user is root here).
--
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]
