Merged
Conversation
Owner
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe changes update configuration templating and substitution across Docker and config scripts. The Dockerfile now installs Changes
Sequence Diagram(s)sequenceDiagram
participant Admin
participant Docker
participant ConfigScript
participant EnvVars
participant ConfigFiles
Admin->>Docker: Build image (installs gettext-base, moreutils)
Docker->>ConfigScript: Run setup scripts
ConfigScript->>EnvVars: Fetch environment variables
ConfigScript->>ConfigFiles: Substitute ${VAR} placeholders using envsubst
ConfigFiles-->>Admin: Ready-to-use config with dynamic values
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
Dockerfile (1)
114-114: Fix trailing space after line-continuation backslashes (Dockerfile parse breakage)At Line 114 (
RANDOMIZER_SEED="" \) and Line 232 (HEALTHCHECK ... \), there’s a space after the backslash. In Dockerfiles, the backslash must be the last character on the line; trailing spaces break continuation and can fail the build.Apply this minimal fix:
- RANDOMIZER_SEED="" \ + RANDOMIZER_SEED="" \ ... -HEALTHCHECK --interval=10s --timeout=10s --start-period=30s --retries=3 \ +HEALTHCHECK --interval=10s --timeout=10s --start-period=30s --retries=3 \Also applies to: 232-233
includes/config.sh (1)
59-66: Harden RCON gating and validate ADMIN_PASSWORD across all scriptsTo avoid silent mis‐configurations and support case-insensitive flags, we should:
- Normalize every
RCON_ENABLEDcheck to[[ "${RCON_ENABLED,,}" == "true" ]]instead of a strict== "true".- Require both
RCON_PORTandADMIN_PASSWORDbefore any RCON behavior.- Update the following locations (at minimum):
•includes/config.sh(setup_rcon_yaml)
•includes/server.sh
•scripts/servermanager.sh
•includes/playerdetection.sh
•scripts/restart.sh
•scripts/rconcli.shExample diff for
includes/config.sh(lines 59–66):function setup_rcon_yaml () { - if [[ -n ${RCON_ENABLED+x} ]] && [ "$RCON_ENABLED" == "true" ] ; then + if [[ -n ${RCON_ENABLED+x} ]] && [[ "${RCON_ENABLED,,}" == "true" ]] ; then ei ">>> RCON is enabled - Setting up rcon.yaml ..." - if [[ -n ${RCON_PORT+x} ]]; then - envsubst < "$RCON_CONFIG_FILE" | sponge "$RCON_CONFIG_FILE" + if [[ -n ${RCON_PORT+x} ]] && [[ -n ${ADMIN_PASSWORD+x} ]]; then + envsubst < "$RCON_CONFIG_FILE" | sponge "$RCON_CONFIG_FILE" else - ee "> RCON_PORT is not set, please set it for RCON functionality to work!" + ee "> RCON_PORT and/or ADMIN_PASSWORD are not set; please set both for RCON to work!" fiConsider extracting a small helper (e.g.
is_enabled()) to DRY up these checks.
🧹 Nitpick comments (1)
configs/rcon.yaml (1)
2-3: LGTM: Standardized env substitutionSwitching to
${RCON_PORT}and${ADMIN_PASSWORD}simplifies setup via envsubst. Ensure ADMIN_PASSWORD isn’t empty in production; the script currently only checks RCON_PORT.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Dockerfile(1 hunks)configs/PalWorldSettings.ini.template(1 hunks)configs/rcon.yaml(1 hunks)includes/config.sh(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
includes/config.sh (1)
includes/colors.sh (2)
es(23-26)ei(18-21)
🔇 Additional comments (1)
Dockerfile (1)
211-211: Good addition: dependencies for envsubst/sponge are installedInstalling gettext-base (envsubst) and moreutils (sponge) aligns with includes/config.sh changes. No concerns.
| @@ -1,2 +1,2 @@ | |||
| [/Script/Pal.PalGameWorldSettings] | |||
| OptionSettings=(Difficulty=None,RandomizerType=None,RandomizerSeed="",bIsRandomizerPalLevelRandom=False,DayTimeSpeedRate=1.000000,NightTimeSpeedRate=1.000000,ExpRate=1.000000,PalCaptureRate=1.000000,PalSpawnNumRate=1.000000,PalDamageRateAttack=1.000000,PalDamageRateDefense=1.000000,PlayerDamageRateAttack=1.000000,PlayerDamageRateDefense=1.000000,PlayerStomachDecreaceRate=1.000000,PlayerStaminaDecreaceRate=1.000000,PlayerAutoHPRegeneRate=1.000000,PlayerAutoHpRegeneRateInSleep=1.000000,PalStomachDecreaceRate=1.000000,PalStaminaDecreaceRate=1.000000,PalAutoHPRegeneRate=1.000000,PalAutoHpRegeneRateInSleep=1.000000,BuildObjectHpRate=1.000000,BuildObjectDamageRate=1.000000,BuildObjectDeteriorationDamageRate=1.000000,CollectionDropRate=1.000000,CollectionObjectHpRate=1.000000,CollectionObjectRespawnSpeedRate=1.000000,EnemyDropItemRate=1.000000,DeathPenalty=All,bEnablePlayerToPlayerDamage=False,bEnableFriendlyFire=False,bEnableInvaderEnemy=True,bActiveUNKO=False,bEnableAimAssistPad=True,bEnableAimAssistKeyboard=False,DropItemMaxNum=3000,DropItemMaxNum_UNKO=100,BaseCampMaxNum=128,BaseCampWorkerMaxNum=15,DropItemAliveMaxHours=1.000000,bAutoResetGuildNoOnlinePlayers=False,AutoResetGuildTimeNoOnlinePlayers=72.000000,GuildPlayerMaxNum=20,BaseCampMaxNumInGuild=4,PalEggDefaultHatchingTime=72.000000,WorkSpeedRate=1.000000,AutoSaveSpan=30.000000,bIsMultiplay=False,bIsPvP=False,bHardcore=False,bPalLost=False,bCharacterRecreateInHardcore=False,bCanPickupOtherGuildDeathPenaltyDrop=False,bEnableNonLoginPenalty=True,bEnableFastTravel=True,bIsStartLocationSelectByMap=True,bExistPlayerAfterLogout=False,bEnableDefenseOtherGuildPlayer=False,bInvisibleOtherGuildBaseCampAreaFX=False,bBuildAreaLimit=False,ItemWeightRate=1.000000,CoopPlayerMaxNum=4,ServerPlayerMaxNum=32,ServerName="Default Palworld Server",ServerDescription="",AdminPassword="",ServerPassword="",PublicPort=8211,PublicIP="",RCONEnabled=False,RCONPort=25575,Region="",bUseAuth=True,BanListURL="https://api.palworldgame.com/api/banlist.txt",RESTAPIEnabled=False,RESTAPIPort=8212,bShowPlayerList=False,ChatPostLimitPerMinute=10,CrossplayPlatforms=(Steam,Xbox,PS5,Mac),bIsUseBackupSaveData=True,LogFormatType=Text,SupplyDropSpan=180,EnablePredatorBossPal=True,MaxBuildingLimitNum=0,ServerReplicatePawnCullDistance=15000.000000,bAllowGlobalPalboxExport=True,bAllowGlobalPalboxImport=False,EquipmentDurabilityDamageRate=1.000000,ItemContainerForceMarkDirtyInterval=1.000000) No newline at end of file | |||
| OptionSettings=(Difficulty=${DIFFICULTY},RandomizerType=${RANDOMIZER_TYPE},RandomizerSeed="${RANDOMIZER_SEED}",bIsRandomizerPalLevelRandom=${IS_RANDOMIZER_PAL_LEVEL_RANDOM},DayTimeSpeedRate=${DAYTIME_SPEEDRATE},NightTimeSpeedRate=${NIGHTTIME_SPEEDRATE},ExpRate=${EXP_RATE},PalCaptureRate=${PAL_CAPTURE_RATE},PalSpawnNumRate=${PAL_SPAWN_NUM_RATE},PalDamageRateAttack=${PAL_DAMAGE_RATE_ATTACK},PalDamageRateDefense=${PAL_DAMAGE_RATE_DEFENSE},PlayerDamageRateAttack=${PLAYER_DAMAGE_RATE_ATTACK},PlayerDamageRateDefense=${PLAYER_DAMAGE_RATE_DEFENSE},PlayerStomachDecreaceRate=${PLAYER_STOMACH_DECREASE_RATE},PlayerStaminaDecreaceRate=${PLAYER_STAMINA_DECREACE_RATE},PlayerAutoHPRegeneRate=${PLAYER_AUTO_HP_REGENE_RATE},PlayerAutoHpRegeneRateInSleep=${PLAYER_AUTO_HP_REGENE_RATE_IN_SLEEP},PalStomachDecreaceRate=${PAL_STOMACH_DECREACE_RATE},PalStaminaDecreaceRate=${PAL_STAMINA_DECREACE_RATE},PalAutoHPRegeneRate=${PAL_AUTO_HP_REGENE_RATE},PalAutoHpRegeneRateInSleep=${PAL_AUTO_HP_REGENE_RATE_IN_SLEEP},BuildObjectHpRate=${BUILD_OBJECT_HP_RATE},BuildObjectDamageRate=${BUILD_OBJECT_DAMAGE_RATE},BuildObjectDeteriorationDamageRate=${BUILD_OBJECT_DETERIORATION_DAMAGE_RATE},CollectionDropRate=${COLLECTION_DROP_RATE},CollectionObjectHpRate=${COLLECTION_OBJECT_HP_RATE},CollectionObjectRespawnSpeedRate=${COLLECTION_OBJECT_RESPAWN_SPEED_RATE},EnemyDropItemRate=${ENEMY_DROP_ITEM_RATE},DeathPenalty=${DEATH_PENALTY},bEnablePlayerToPlayerDamage=${ENABLE_PLAYER_TO_PLAYER_DAMAGE},bEnableFriendlyFire=${ENABLE_FRIENDLY_FIRE},bEnableInvaderEnemy=${ENABLE_INVADER_ENEMY},bActiveUNKO=${ACTIVE_UNKO},bEnableAimAssistPad=${ENABLE_AIM_ASSIST_PAD},bEnableAimAssistKeyboard=${ENABLE_AIM_ASSIST_KEYBOARD},DropItemMaxNum=${DROP_ITEM_MAX_NUM},DropItemMaxNum_UNKO=${DROP_ITEM_MAX_NUM_UNKO},BaseCampMaxNum=${BASE_CAMP_MAX_NUM},BaseCampWorkerMaxNum=${BASE_CAMP_WORKER_MAXNUM},DropItemAliveMaxHours=${DROP_ITEM_ALIVE_MAX_HOURS},bAutoResetGuildNoOnlinePlayers=${AUTO_RESET_GUILD_NO_ONLINE_PLAYERS},AutoResetGuildTimeNoOnlinePlayers=${AUTO_RESET_GUILD_TIME_NO_ONLINE_PLAYERS},GuildPlayerMaxNum=${GUILD_PLAYER_MAX_NUM},BaseCampMaxNumInGuild=${BASE_CAMP_MAX_NUM_IN_GUILD},PalEggDefaultHatchingTime=${PAL_EGG_DEFAULT_HATCHING_TIME},WorkSpeedRate=${WORK_SPEED_RATE},AutoSaveSpan=${AUTO_SAVE_SPAN},bIsMultiplay=${IS_MULTIPLAY},bIsPvP=${IS_PVP},bHardcore=${HARDCORE},bPalLost=${PAL_LOST},bCharacterRecreateInHardcore=${CHARACTER_RECREATE_IN_HARDCORE},bCanPickupOtherGuildDeathPenaltyDrop=${CAN_PICKUP_OTHER_GUILD_DEATH_PENALTY_DROP},bEnableNonLoginPenalty=${ENABLE_NON_LOGIN_PENALTY},bEnableFastTravel=${ENABLE_FAST_TRAVEL},bIsStartLocationSelectByMap=${IS_START_LOCATION_SELECT_BY_MAP},bExistPlayerAfterLogout=${EXIST_PLAYER_AFTER_LOGOUT},bEnableDefenseOtherGuildPlayer=${ENABLE_DEFENSE_OTHER_GUILD_PLAYER},bInvisibleOtherGuildBaseCampAreaFX=${INVISBIBLE_OTHER_GUILD_BASE_CAMP_AREA_FX},bBuildAreaLimit=${BUILD_AREA_LIMIT},ItemWeightRate=${ITEM_WEIGHT_RATE},CoopPlayerMaxNum=${COOP_PLAYER_MAX_NUM},ServerPlayerMaxNum=${MAX_PLAYERS},ServerName="${SERVER_NAME}",ServerDescription="${SERVER_DESCRIPTION}",AdminPassword="${ADMIN_PASSWORD}",ServerPassword="${SERVER_PASSWORD}",PublicPort=${PUBLIC_PORT},PublicIP="${PUBLIC_IP}",RCONEnabled=${RCON_ENABLED},RCONPort=${RCON_PORT},Region="${REGION}",bUseAuth=${USEAUTH},BanListURL="${BAN_LIST_URL}",RESTAPIEnabled=${RESTAPI_ENABLED},RESTAPIPort=${RESTAPI_PORT},bShowPlayerList=${SHOW_PLAYER_LIST},ChatPostLimitPerMinute=${CHAT_POST_LIMIT_PER_MINUTE},CrossplayPlatforms=(${CROSSPLAY_PLATFORMS}),bIsUseBackupSaveData=${ENABLE_WORLD_BACKUP},LogFormatType=${LOG_FORMAT_TYPE},SupplyDropSpan=${SUPPLY_DROP_SPAN},EnablePredatorBossPal=${ENABLE_PREDATOR_BOSS_PAL},MaxBuildingLimitNum=${MAX_BUILDING_LIMIT_NUM},ServerReplicatePawnCullDistance=${SERVER_REPLICATE_PAWN_CULL_DISTANCE},bAllowGlobalPalboxExport=${ALLOW_GLOBAL_PALBOX_EXPORT},bAllowGlobalPalboxImport=${ALLOW_GLOBAL_PALBOX_IMPORT},EquipmentDurabilityDamageRate=${EQUIPMENT_DURABILITY_DAMAGE_RATE},ItemContainerForceMarkDirtyInterval=${ITEM_CONTAINER_FORCE_MARK_DIRTY_INTERVAL}) | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid double parentheses for CrossplayPlatforms
Default in Dockerfile sets CROSSPLAY_PLATFORMS="(Steam,Xbox,PS5,Mac)" while the template wraps it again as CrossplayPlatforms=(${CROSSPLAY_PLATFORMS}), resulting in ((Steam,...)).
Pick one:
- Prefer to remove parentheses in template:
-...,CrossplayPlatforms=(${CROSSPLAY_PLATFORMS}),...
+...,CrossplayPlatforms=${CROSSPLAY_PLATFORMS},...- Or change Dockerfile default to omit parentheses (and keep template as-is).
Also, SERVER_NAME default contains ###RANDOM###. With envsubst, this stays literal, losing prior randomness. See suggested fix in includes/config.sh to reintroduce randomization before substitution.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate Dockerfile
DOCKERFILE=$(fd -g Dockerfile | head -n1)
echo "Dockerfile path: $DOCKERFILE"
# Inspect CROSSPLAY_PLATFORMS in Dockerfile
echo -e "\n[CROSSPLAY_PLATFORMS entries in Dockerfile]"
grep -Rn 'CROSSPLAY_PLATFORMS' -n "$DOCKERFILE" || true
grep -En 'ENV\s+CROSSPLAY_PLATFORMS' -n "$DOCKERFILE" || true
# Inspect CROSSPLAY_PLATFORMS usage across repo
echo -e "\n[CROSSPLAY_PLATFORMS usage across repo]"
rg 'CROSSPLAY_PLATFORMS' -n .
# Inspect SERVER_NAME defaults and usage
echo -e "\n[SERVER_NAME definitions and usage]"
rg 'SERVER_NAME' -n . || true
# Check for literal ###RANDOM### occurrences
echo -e "\n[Occurrences of literal ###RANDOM###]"
rg '###RANDOM###' -n . || trueLength of output: 10397
Resolve CrossplayPlatforms double parentheses and reintroduce SERVER_NAME randomization
- configs/PalWorldSettings.ini.template: remove the extra
()around${CROSSPLAY_PLATFORMS}. - Dockerfile (line 190) & default.env (line 132): if you’d rather keep the template untouched, change
CROSSPLAY_PLATFORMS="(Steam,Xbox,PS5,Mac)"→CROSSPLAY_PLATFORMS="Steam,Xbox,PS5,Mac". - Startup script (e.g., includes/config.sh): generate a random suffix for
SERVER_NAMEinstead of using the literal###RANDOM###before callingenvsubst.
Suggested diff for the template:
--- a/configs/PalWorldSettings.ini.template
+++ b/configs/PalWorldSettings.ini.template
@@ Line 2
-…,CrossplayPlatforms=(${CROSSPLAY_PLATFORMS}),…
+,…,+CrossplayPlatforms=${CROSSPLAY_PLATFORMS},…Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In configs/PalWorldSettings.ini.template at line 2, remove the extra parentheses
around ${CROSSPLAY_PLATFORMS} to fix the double parentheses issue. Additionally,
ensure that SERVER_NAME is randomized by updating the startup script to generate
a random suffix for SERVER_NAME instead of using the literal ###RANDOM### before
calling envsubst. If you prefer not to modify the template, update
CROSSPLAY_PLATFORMS in Dockerfile line 190 and default.env line 132 by removing
the parentheses from the value string.
jammsen
requested changes
Aug 9, 2025
Owner
jammsen
left a comment
jammsen
left a comment
There was a problem hiding this comment.
This version of the pr needs some work, for not breaking devevelop or master/latest
Contributor
Author
|
Removed the |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
includes/config.sh (2)
60-61: Limit envsubst scope and add error handling + atomic writeAvoid substituting unintended env vars and handle failures. Also write atomically without partially-written files.
- envsubst < "${PALWORLD_TEMPLATE_FILE}" > "${GAME_SETTINGS_FILE}" + # Limit substitution to only variables present in the template and write atomically + _vars="$(envsubst -v < "${PALWORLD_TEMPLATE_FILE}" | awk '{printf "${%s} ", $0}')" + _tmp="$(mktemp)" + if ! envsubst "${_vars}" < "${PALWORLD_TEMPLATE_FILE}" > "${_tmp}"; then + ee "Failed to render ${GAME_SETTINGS_FILE} from ${PALWORLD_TEMPLATE_FILE}" + rm -f "${_tmp}" + return 1 + fi + mv "${_tmp}" "${GAME_SETTINGS_FILE}" + # Optional: protect config perms if it can contain secrets + chmod 640 "${GAME_SETTINGS_FILE}" >/dev/null 2>&1 || trueNotes:
- envsubst -v is available in GNU gettext; if not, fall back to a maintained explicit list.
- chmod line is optional, but recommended if secrets can land in the INI.
68-68: Drop moreutils dependency; use mktemp + mv and restrict variablessponge is unnecessary; use a temp file for a safe in-place update and limit envsubst to only the needed vars.
- envsubst < "$RCON_CONFIG_FILE" | sponge "$RCON_CONFIG_FILE" + _vars='${RCON_PORT} ${ADMIN_PASSWORD}' + _tmp="$(mktemp)" + if ! envsubst "${_vars}" < "${RCON_CONFIG_FILE}" > "${_tmp}"; then + ee "Failed to render ${RCON_CONFIG_FILE}" + rm -f "${_tmp}" + return 1 + fi + mv "${_tmp}" "${RCON_CONFIG_FILE}" + chmod 600 "${RCON_CONFIG_FILE}" >/dev/null 2>&1 || trueFollow-up:
- Remove moreutils from the Dockerfile (gettext-base is still required).
🧹 Nitpick comments (1)
includes/config.sh (1)
56-58: Nit: make randomness generation locale-agnosticEnsure predictable behavior across locales and minimal deps.
- rand="$(tr -dc 'A-Za-z0-9' </dev/urandom | head -c 6)" + rand="$(LC_ALL=C tr -dc 'A-Za-z0-9' </dev/urandom | head -c 6)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Dockerfile(1 hunks)configs/PalWorldSettings.ini.template(1 hunks)configs/rcon.yaml(1 hunks)includes/config.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- configs/rcon.yaml
- Dockerfile
- configs/PalWorldSettings.ini.template
🧰 Additional context used
🧬 Code Graph Analysis (1)
includes/config.sh (1)
includes/colors.sh (2)
es(23-26)ei(18-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docker - Test Linux-amd64 image
🔇 Additional comments (1)
includes/config.sh (1)
54-59: Good reintroduction of SERVER_NAME random suffixThe pre-envsubst replacement for the ###RANDOM### token restores the legacy behavior correctly. Using global replacement (//) is appropriate here.
jammsen
added a commit
that referenced
this pull request
Sep 6, 2025
…r checks, removed obsolete moreutils package from PR, removed trailing spaces, added more error-fallbacks in config.sh around the envsubst integration
Owner
|
Hey @codebam - I cleaned up this PR, this needed a lot of thinking and work ontop of the idea you presented. Thanks for the contribution. Its now merged to develop. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I haven't completely gone through the template, it was generated with AI, but aside from that this entire commit was written by me and worked on @ripps818's wine version of this container
Summary by CodeRabbit
New Features
Chores