[GitHub] [airflow] potiuk commented on a change in pull request #9105: Cope with multiple processes get_remote_image_info in parallel

[GitBox <gi...@apache.org>]

potiuk commented on a change in pull request #9105:
URL: https://github.com/apache/airflow/pull/9105#discussion_r433903219



##########
File path: scripts/ci/_utils.sh
##########
@@ -712,25 +717,29 @@ function get_local_image_info() {
 function get_remote_image_info() {
     set +e
     # Pull remote manifest image
-    if ! verbose_docker pull "${AIRFLOW_CI_REMOTE_MANIFEST_IMAGE}" >/dev/null 2>&1 ; then
+    if ! verbose_docker pull "${AIRFLOW_CI_REMOTE_MANIFEST_IMAGE}" >/dev/null; then
         echo
         echo "Remote docker registry unreachable"
         echo
         REMOTE_DOCKER_REGISTRY_UNREACHABLE="true"
         return
     fi
+    set -e
+
+    # Docker needs the file passed to --cidfile to not exist, so we can't use mktemp
+    TMP_CONTAINER_ID="remote-airflow-manifest-$$.container_id"
+    trap 'rm -f "$TMP_CONTAINER_ID"' EXIT
+
     TMP_MANIFEST_REMOTE_JSON=$(mktemp)
     TMP_MANIFEST_REMOTE_SHA=$(mktemp)
-    # delete container just in case
-    verbose_docker rm --force "remote-airflow-manifest" >/dev/null 2>&1
-    set -e
+
     # Create container out of the manifest image without runnning it
-    verbose_docker create --name "remote-airflow-manifest" "${AIRFLOW_CI_REMOTE_MANIFEST_IMAGE}" >/dev/null 2>&1
+    verbose_docker create --cidfile "${TMP_CONTAINER_ID}" "${AIRFLOW_CI_REMOTE_MANIFEST_IMAGE}"

Review comment:
       Nice. Indeed it works better in parallel this way.

##########
File path: scripts/ci/_utils.sh
##########
@@ -39,10 +39,15 @@ function check_verbose_setup {
 # In case "VERBOSE" is set to "true" (--verbose flag in Breeze) all docker commands run will be
 # printed before execution
 function verbose_docker {
-    if [[ ${VERBOSE:="false"} == "true" ]]; then
+    if [[ ${VERBOSE_COMMANDS:=} == "true" ]]; then

Review comment:
       ```suggestion
       if [[ ${VERBOSE} == "true" && ${VERBOSE_COMMAND} != "true" ]]; then
            # do not print echo if VERSBOSE_COMMAND is set (set -x does it already)
            echo "docker" "${@}"
        fi
        docker "${@}"
   ```

##########
File path: scripts/ci/_utils.sh
##########
@@ -712,25 +717,29 @@ function get_local_image_info() {
 function get_remote_image_info() {
     set +e
     # Pull remote manifest image
-    if ! verbose_docker pull "${AIRFLOW_CI_REMOTE_MANIFEST_IMAGE}" >/dev/null 2>&1 ; then
+    if ! verbose_docker pull "${AIRFLOW_CI_REMOTE_MANIFEST_IMAGE}" >/dev/null; then
         echo
         echo "Remote docker registry unreachable"
         echo
         REMOTE_DOCKER_REGISTRY_UNREACHABLE="true"
         return
     fi
+    set -e
+
+    # Docker needs the file passed to --cidfile to not exist, so we can't use mktemp
+    TMP_CONTAINER_ID="remote-airflow-manifest-$$.container_id"
+    trap 'rm -f "$TMP_CONTAINER_ID"' EXIT

Review comment:
       this will override previous trap if you had it set (which is likely), The best solution I found is to wrap the trap code into a function and do it as follows:
   
   ```bash
   # adding trap to exiting trap
   HANDLERS="$( trap -p EXIT | cut -f2 -d \' )"
   # shellcheck disable=SC2064
   trap "${HANDLERS}${HANDLERS:+;}the_new_function" EXIT
   ``




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org