You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/07/19 00:14:02 UTC
[GitHub] [airflow] BShraman opened a new pull request #17070: Updated entrypoint_prod.sh
BShraman opened a new pull request #17070:
URL: https://github.com/apache/airflow/pull/17070
<!--
Below are the changes :
- Declared readonly variables .
- Separated declaration and assignment in two different line for local variables inside the function , to make it consistences in scripts.
- Double Quoted strings containing variables.
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #17070: Updated entrypoint_prod.sh
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17070:
URL: https://github.com/apache/airflow/pull/17070#discussion_r672141888
##########
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #17070: Updated entrypoint_prod.sh
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17070:
URL: https://github.com/apache/airflow/pull/17070#issuecomment-882400495
Closing it, as none of the changes are good in it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #17070: Updated entrypoint_prod.sh
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #17070:
URL: https://github.com/apache/airflow/pull/17070
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #17070: Updated entrypoint_prod.sh
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17070:
URL: https://github.com/apache/airflow/pull/17070#issuecomment-882400495
Closing it, as none of the changes are good in it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #17070: Updated entrypoint_prod.sh
Posted by GitBox <gi...@apache.org>.
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 ""
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #17070: Updated entrypoint_prod.sh
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17070:
URL: https://github.com/apache/airflow/pull/17070#discussion_r672139686
##########
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.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #17070: Updated entrypoint_prod.sh
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #17070:
URL: https://github.com/apache/airflow/pull/17070
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #17070: Updated entrypoint_prod.sh
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org