Reformatted the Docker entrypoint.sh script #9510
Conversation
WalkthroughRefactors the Docker entrypoint script to replace POSIX Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-70: The shell invocation that builds external_config_db_exists
calls python3 -c with an outer double-quoted string and accidentally uses
unescaped double quotes around ${PGADMIN_CONFIG_CONFIG_DATABASE_URI}, breaking
the shell quoting; fix by switching the -c payload to single quotes and keep the
variable expansion in double quotes (e.g. /venv/bin/python3 -c 'from
check_external_config_db import check_external_config_db; val =
check_external_config_db("'"${PGADMIN_CONFIG_CONFIG_DATABASE_URI}"'");
print(val)') or escape the inner quotes, ensuring the Python code receives a
proper quoted string; update the command that sets external_config_db_exists
accordingly (references: PGADMIN_CONFIG_CONFIG_DATABASE_URI,
external_config_db_exists, check_external_config_db).
🧹 Nitpick comments (4)
pkg/docker/entrypoint.sh (4)
30-32: Consider quoting variable expansions consistently.While Bash
[[...]]prevents word splitting, it's safer and more consistent to quote variable expansions that may contain paths or special characters. The variables$PGADMIN_CONFIG_CONFIG_DATABASE_URI_FILE(line 30) and$CONFIG_DISTRO_FILE_PATH(line 41) should be quoted in the conditional tests.🔎 Proposed fix
-if [[ -n $PGADMIN_CONFIG_CONFIG_DATABASE_URI_FILE ]]; then +if [[ -n "$PGADMIN_CONFIG_CONFIG_DATABASE_URI_FILE" ]]; then-if [[ ! -e $CONFIG_DISTRO_FILE_PATH || $(wc -m "${CONFIG_DISTRO_FILE_PATH}" 2>/dev/null | awk '{ print $1 }') == 0 ]]; then +if [[ ! -e "$CONFIG_DISTRO_FILE_PATH" || $(wc -m "${CONFIG_DISTRO_FILE_PATH}" 2>/dev/null | awk '{ print $1 }') == 0 ]]; thenAlso applies to: 41-64
73-91: Consider quoting variables for consistency.While
[[...]]prevents word splitting, quoting variables like$PGADMIN_REPLACE_SERVERS_ON_STARTUP,$PGADMIN_SERVER_JSON_FILE, and$PGADMIN_CONFIG_SERVER_MODEimproves code consistency and defensive programming practices.
115-119: Consider more efficient pattern matching.The
echo | grep &>/dev/nullpipeline on line 115 can be replaced with Bash's native pattern matching for better performance and clarity.🔎 Proposed fix
- if echo "${is_valid_email}" | grep "False" &>/dev/null; then + if [[ $is_valid_email == *False* ]]; thenThis uses Bash's glob pattern matching within
[[...]], which is more efficient than spawning external processes.
57-58: Update outdated comment.The comment mentions "BusyBox/ash as it's shell" but this script explicitly uses Bash (line 1) and has been refactored to use Bash-specific features throughout. The comment should be updated to reflect the current implementation.
🔎 Proposed fix
- # This is a bit kludgy, but necessary as the container uses BusyBox/ash as - # it's shell and not bash which would allow a much cleaner implementation + # This iterates over PGADMIN_CONFIG_* environment variables and writes them + # to the config file
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/docker/entrypoint.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-20T20:14:11.407Z
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 0
File: :0-0
Timestamp: 2025-11-20T20:14:11.407Z
Learning: In the pgadmin4 Dockerfile, the sudoers entry for `pgadminr` (line containing `echo "pgadminr ALL = NOPASSWD: /usr/sbin/postfix start" >> /etc/sudoers.d/postfix`) is intentional and not a typo. The `pgadminr` user is dynamically created by the docker entrypoint script when the container runs with a non-default UID (not 5050) and the user can write to /etc/passwd. Both `pgadmin` and `pgadminr` sudoers entries are needed to support different container execution scenarios.
Applied to files:
pkg/docker/entrypoint.sh
🪛 Shellcheck (0.11.0)
pkg/docker/entrypoint.sh
[warning] 69-69: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (15)
🔇 Additional comments (5)
pkg/docker/entrypoint.sh (5)
4-6: LGTM!The OpenShift passwd fixup logic has been cleanly consolidated into a single compound condition using Bash
[[...]]syntax. The use of&>/dev/nullis idiomatic for Bash, and the combined condition correctly checks all three requirements before appending to/etc/passwd.
11-27: LGTM!The
file_envfunction has been properly modernized to use Bash idioms. The use of[[ ... ]], indirect parameter expansion with safe defaults (${!var:-}), and the$(< file)construct for reading files are all appropriate Bash patterns.
164-166: LGTM!The Postfix startup logic correctly checks if
PGADMIN_DISABLE_POSTFIXis unset or empty before starting the service.
175-181: LGTM! Critical binding logic preserved.The bind address configuration correctly prioritizes socket over TLS, with appropriate defaults (port 443 for TLS, port 80 otherwise). The explicit
elifchain is clearer than nested conditions.
183-187: LGTM!The final gunicorn execution correctly branches on TLS configuration, with appropriate certificate options when TLS is enabled and additional request-limiting options when it's not.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/docker/entrypoint.sh (1)
65-66: Fix quoting in Python command string.The variable
${PGADMIN_CONFIG_CONFIG_DATABASE_URI}on line 66 is unquoted within the Python code string, which will cause shell quoting issues. The inner quotes must be escaped or the string structure adjusted.🔎 Recommended fix
- external_config_db_exists=$(cd /pgadmin4/pgadmin/utils && /venv/bin/python3 -c "from check_external_config_db import check_external_config_db; val = check_external_config_db(${PGADMIN_CONFIG_CONFIG_DATABASE_URI}); print(val)") + external_config_db_exists=$(cd /pgadmin4/pgadmin/utils && /venv/bin/python3 -c 'from check_external_config_db import check_external_config_db; val = check_external_config_db("'"${PGADMIN_CONFIG_CONFIG_DATABASE_URI}"'"); print(val)')
🧹 Nitpick comments (1)
pkg/docker/entrypoint.sh (1)
75-178: Consider quoting variables in[[ ... ]]tests for consistency.While unquoted variables in
[[ ... ]]constructs are safe (word splitting doesn't occur), quoting them is considered best practice for consistency and clarity. Examples:
- Line 75:
[[ "$PGADMIN_REPLACE_SERVERS_ON_STARTUP" == 'True' ]]- Line 79:
[[ -f "$PGADMIN_SERVER_JSON_FILE" ]]- Line 82:
[[ "$PGADMIN_CONFIG_SERVER_MODE" == 'False' ]]And similarly throughout the rest of the file.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/docker/entrypoint.sh
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-20T20:14:11.407Z
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 0
File: :0-0
Timestamp: 2025-11-20T20:14:11.407Z
Learning: In the pgadmin4 Dockerfile, the sudoers entry for `pgadminr` (line containing `echo "pgadminr ALL = NOPASSWD: /usr/sbin/postfix start" >> /etc/sudoers.d/postfix`) is intentional and not a typo. The `pgadminr` user is dynamically created by the docker entrypoint script when the container runs with a non-default UID (not 5050) and the user can write to /etc/passwd. Both `pgadmin` and `pgadminr` sudoers entries are needed to support different container execution scenarios.
Applied to files:
pkg/docker/entrypoint.sh
📚 Learning: 2025-11-20T19:52:43.337Z
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 0
File: :0-0
Timestamp: 2025-11-20T19:52:43.337Z
Learning: In the pgadmin4 Dockerfile, the setcap command uses the glob pattern `python3.[0-9][0-9]` to match the Python binary. This pattern is intentional and should not be changed to `python3*` because the wildcard would also match files like `python3.14-config`, causing build failures. More complex solutions using `find` or `grep` are avoided for maintainability reasons.
Applied to files:
pkg/docker/entrypoint.sh
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: run-feature-tests-pg (15)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (14)
🔇 Additional comments (4)
pkg/docker/entrypoint.sh (4)
4-6: LGTM! Cleaner combined condition.The refactoring to combine the OpenShift passwd fixup checks into a single
[[ ... ]]test with&&is more concise and idiomatic Bash. The logic is preserved correctly.
11-27: LGTM! Excellent Bash refactoring.The
file_envfunction has been properly converted to use Bash[[ ... ]]tests throughout. The logic flow, error handling, and variable exports are all preserved correctly.
58-60: LGTM! Much cleaner iteration pattern.The refactoring to use
${!PGADMIN_CONFIG_@}for iterating over environment variables is excellent. This is the idiomatic Bash approach and is much cleaner than the previous implementation.
1-184: Excellent refactoring work overall!The conversion from POSIX shell constructs to Bash idioms is well-executed throughout the file. The changes improve readability and maintainability while preserving functional behavior. Key improvements include:
- Consistent use of
[[ ... ]]tests- Idiomatic
${!PREFIX@}expansion for iterating environment variables- Pattern matching instead of external commands
- Normalized indentation
Aside from the critical quoting issue on line 66, the refactoring looks solid.
That's out of the question. This will break all existing deployments that make use of the environmental variable, since they all currently add the missing quotes inside the value itself. Besides, it's completely out of scope of this PR.
It's a matter of preference. I did mention in the PR description that I'd be happy to change things if the maintainers have a different preference. |
|
Tip For best results, initiate chat on the files or code changes. @Guiorgy, thank you for the clarifications! You're absolutely right on both points:
The reformatting work you've done looks solid, and it's great that you've tested the happy path to ensure no behavioral changes. Thank you for taking the time to explain the context! |
1 participant
Since the script is annotated with
#!/usr/bin/env bashand uses some Bashism likelocal, we may as well go all the way. While at it, I also fixed some indentation inconsistencies, where most of the code used 4 spaces, but some small blocks used a tab character, some used 2 spaces, and some had an extra space. Normalized everything to 4 space characters.Admittedly, code formatting is a very subjective topic, thus, do let me know if there are any specific formats you disagree with and I'll revert them by rebasing.
Note: I built and am running this image, however, technically this only tests the happy path, though since this is only a reformat, I don't expect any change in behaviour.Not yet tested after pushing a few more commits.Edit: Coderabbitai found the source of #8529, there's an incorrect variable quoting in the script. Since the issue was marked as won't fix, and this is only a reformat PR, I fixed the code while retaining the old behaviour. I'll submit a separate PR just in case anyway.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.