potiuk commented on a change in pull request #17070:
URL: https://github.com/apache/airflow/pull/17070#discussion_r672139602
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -34,14 +34,14 @@ function run_check_with_retries {
last_check_result=$(eval "${cmd} 2>&1")
res=$?
set -e
- if [[ ${res} == 0 ]]; then
+ if [[ "${res}" == 0 ]]; then
Review comment:
That's not good. left side of if does not need ""
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -34,14 +34,14 @@ function run_check_with_retries {
last_check_result=$(eval "${cmd} 2>&1")
res=$?
set -e
- if [[ ${res} == 0 ]]; then
+ if [[ "${res}" == 0 ]]; then
echo
break
else
echo -n "."
countdown=$((countdown-1))
fi
- if [[ ${countdown} == 0 ]]; then
+ if [[ "${countdown}" == 0 ]]; then
Review comment:
same here
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -106,13 +111,13 @@ function wait_for_connection {
readonly BACKEND
if [[ -z "${detected_port=}" ]]; then
- if [[ ${BACKEND} == "postgres"* ]]; then
+ if [[ "${BACKEND}" == "postgres"* ]]; then
Review comment:
no need for those either.
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -83,9 +85,12 @@ function wait_for_connection {
# It tries `CONNECTION_CHECK_MAX_COUNT` times and sleeps
`CONNECTION_CHECK_SLEEP_TIME` between checks
local connection_url
connection_url="${1}"
- local detected_backend=""
- local detected_host=""
- local detected_port=""
+ local detected_backend
Review comment:
same here. It was good as was before.
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -68,8 +68,10 @@ function run_nc() {
# Even if this message might be harmless, it might hide the real reason
for the problem
# Which is the long time needed to start some services, seeing this
message might be totally misleading
# when you try to analyse the problem, that's why it's best to avoid it,
- local host="${1}"
- local port="${2}"
+ local host
Review comment:
it was good as it was before I think. Shorter and safe. The split to
declaration/assignment should only be done if there is a "command" executed on
the right side because it then can produce unexpected results.
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -16,7 +16,7 @@
# specific language governing permissions and limitations
# under the License.
# Might be empty
-AIRFLOW_COMMAND="${1:-}"
+readonly AIRFLOW_COMMAND="${1:-}"
Review comment:
This is wrong too. In the "if" below the AIRFLOW_COMMAND might be
overwritten so it cannot be readonly
--
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]
