Esnure the container deployment suuports boolean values in yaml format.#9522#9575
Conversation
WalkthroughUpdated the entrypoint script loop that processes Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/docker/entrypoint.sh`:
- Around line 68-69: The case branch that maps numeric 1/0 to boolean True/False
(the patterns "true|1) val=\"True\" ;; " and "false|0) val=\"False\" ;;") causes
integer PGADMIN_CONFIG_* options to become booleans; remove the "1" and "0"
alternatives from those case patterns so only literal boolean strings map to
True/False, or implement a type-aware conversion: detect keys like
PGADMIN_CONFIG_DEFAULT_SERVER_PORT, LOG_ROTATION_SIZE, LOG_ROTATION_AGE,
LOG_ROTATION_MAX_LOG_FILES, and MAX_SESSION_IDLE_TIME and preserve numeric
values (emit them unchanged) while still converting "true"/"false" to Python
True/False for non-integer keys.
🧹 Nitpick comments (2)
pkg/docker/entrypoint.sh (2)
64-71: Consider using${!var}instead ofevalfor safer indirect expansion.The script already uses bash indirect expansion (
${!var}) in thefile_envfunction (lines 19, 24, 26). Usingevalhere is less safe and inconsistent with the rest of the script. Additionally, if a value contains shell metacharacters, theevalapproach could produce unexpected results.♻️ Proposed refactor using indirect expansion
for var in $(env | grep "^PGADMIN_CONFIG_" | cut -d "=" -f 1); do - # Get the raw value - val=$(eval "echo \"\$$var\"") + # Get the raw value using indirect expansion + val="${!var}" # This normalization step is what makes 'true', 'True', and 1 all work case "$(echo "$val" | tr '[:upper:]' '[:lower:]')" in true|1) val="True" ;; false|0) val="False" ;; esac echo "${var#PGADMIN_CONFIG_} = $val" >> "${CONFIG_DISTRO_FILE_PATH}" done
61-62: Comment is inconsistent with the script's actual shell.The comment states the container uses BusyBox/ash, but the shebang (line 1) specifies
#!/usr/bin/env bash, and the script already uses bash-specific features like${!var}indirect expansion in thefile_envfunction. Consider updating the comment to reflect reality, or removing it since the suggested refactor above makes the code cleaner.
d4247d0 to
5d8f605
Compare
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.