[GitHub] [airflow] potiuk commented on a change in pull request #13585: Add installRequirements for helm chart

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

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



##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -116,6 +116,11 @@ if ! whoami &> /dev/null; then
   export HOME="${AIRFLOW_USER_HOME_DIR}"
 fi
 
+# Install extra python packages if requirements.txt is present.

Review comment:
       Using Airflow AIRFLOW_HOME for extra requirements is not a good idea. It's better to mount the whole folder where you will mount various files, and in this case overriding AIRFLOW_HOME is wrong. Besides AIRFLOW_HOME is read-write where the requirements and related files should be "read-only"
   
   It will be better if you follow the convention that we already have in our "CI" image.
   
   * "/files" directory should be there for any files mounted to to the image
   * "/files/requirements" should be the folder where requirements are placed
   * "/files/requirements/requirements-3.7.txt" should be the file that you load (depending on the python version you might have different requirements"
   
   Additionally you must print some diagnostics information so that when you see the log files you will see exactly what happening.
   
   This way you will not only be able to use it in production but also test it seamlessly for different python versions in our 'breeze` development envionment.
   
   Relevant snippet from the `run_init_script.sh`:
   
   ```
   if [ -z "${FILES_DIR+x}" ]; then
       export FILES_DIR="/files"
   fi
   if [ -z "${AIRFLOW_BREEZE_CONFIG_DIR+x}" ]; then
       export AIRFLOW_BREEZE_CONFIG_DIR="${FILES_DIR}/airflow-breeze-config"
   fi
   
   if [ -z "${INIT_SCRIPT_FILE=}" ]; then
       export INIT_SCRIPT_FILE="init.sh"
   fi
   
   if [[ -d "${AIRFLOW_BREEZE_CONFIG_DIR}" && \
       -f "${AIRFLOW_BREEZE_CONFIG_DIR}/${INIT_SCRIPT_FILE}" ]]; then
   
           pushd "${AIRFLOW_BREEZE_CONFIG_DIR}" >/dev/null 2>&1 || exit 1
           echo
           echo "Sourcing the initialization script from ${INIT_SCRIPT_FILE} in ${AIRFLOW_BREEZE_CONFIG_DIR}"
           echo
            # shellcheck disable=1090
           source "${INIT_SCRIPT_FILE}"
           popd >/dev/null 2>&1 || exit 1
   else
       echo
       echo "You can add ${AIRFLOW_BREEZE_CONFIG_DIR} directory and place ${INIT_SCRIPT_FILE}"
       echo "In it to make breeze source an initialization script automatically for you"
       echo
   fi
   ```




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