Copilot commented on code in PR #723:
URL: https://github.com/apache/dubbo-go-pixiu/pull/723#discussion_r2251029737
##########
docker-entrypoint.sh:
##########
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/bin/sh
Review Comment:
Changing from bash to sh may introduce compatibility issues. The script uses
features like variable substitution that should work in sh, but consider
verifying all functionality works correctly with the POSIX shell, especially if
the original script relied on bash-specific features.
##########
docker-entrypoint.sh:
##########
@@ -18,29 +18,35 @@
# under the License.
#
-CMD_PARAMS=""
-API_CONF="api_config.yaml"
-LOG="log.yml"
-CONF="conf.yaml"
+set -e
-cd /etc/pixiu/ && ls -al /etc/pixiu/
-echo "current path: $(pwd)"
+CONF_DIR="/etc/pixiu"
+APP_BIN="/app/dubbo-go-pixiu"
-if [ ! -f "$CONF" ]; then
- echo "error: $CONF not exists!"
- exit
+API_CONF_FILE="$CONF_DIR/api_config.yaml"
+LOG_FILE="$CONF_DIR/log.yml"
+CONF_FILE="$CONF_DIR/conf.yaml"
+
+echo "Checking configurations in $CONF_DIR..."
+ls -al "$CONF_DIR"
+
+if [ ! -f "$CONF_FILE" ]; then
+ echo "Error: Main configuration file not found at $CONF_FILE"
+ exit 1
fi
-CMD_PARAMS="-c /etc/pixiu/${CONF}"
-if [ -f "$API_CONF" ]; then
- CMD_PARAMS="$CMD_PARAMS -a /etc/pixiu/${API_CONF}"
+CMD_PARAMS="-c ${CONF_FILE}"
+
+if [ -f "$API_CONF_FILE" ]; then
+ CMD_PARAMS="$CMD_PARAMS -a ${API_CONF_FILE}"
fi
-if [ -f "$LOG" ]; then
- CMD_PARAMS="$CMD_PARAMS -g /etc/pixiu/${LOG}"
+if [ -f "$LOG_FILE" ]; then
+ CMD_PARAMS="$CMD_PARAMS -g ${LOG_FILE}"
fi
-cd / ls -al .
-echo "current path: $(pwd)"
-echo "CMD_PARAMS: $CMD_PARAMS"
-./dubbo-go-pixiu gateway start $CMD_PARAMS
+echo "Starting Pixiu Gateway..."
+echo "Binary: $APP_BIN"
+echo "Parameters: $CMD_PARAMS"
+
+exec "$APP_BIN" gateway start $CMD_PARAMS
Review Comment:
The CMD_PARAMS variable should be quoted to handle arguments with spaces
correctly. Consider changing to: exec "$APP_BIN" gateway start "$CMD_PARAMS" or
use an array-based approach for safer parameter handling.
##########
Dockerfile:
##########
@@ -18,7 +18,7 @@
#
### builder
Review Comment:
The change from bookworm to bullseye moves to an older Debian version.
Consider documenting the reason for this downgrade, as bookworm (Debian 12) is
newer than bullseye (Debian 11) and typically preferred for security updates.
```suggestion
### builder
# NOTE: Using Debian 11 ("bullseye") instead of newer "bookworm" (Debian 12)
due to compatibility requirements with certain dependencies/tools.
# Please revisit this choice periodically to check if upgrade to "bookworm"
is possible and safe.
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
