Copilot commented on code in PR #52233:
URL: https://github.com/apache/airflow/pull/52233#discussion_r2166266881
##########
scripts/docker/entrypoint_ci.sh:
##########
@@ -271,6 +271,22 @@ function check_boto_upgrade() {
set +x
}
+# Upgrade sqlalchemy to the latest version to run tests with it
+function check_upgrade_sqlalchemy() {
+ if [[ ${UPGRADE_SQLALCHEMY=} != "true" ]]; then
+ return
+ fi
+ echo
+ echo "${COLOR_BLUE}Upgrading sqlalchemy to the latest version to run tests
with it${COLOR_RESET}"
+ echo
+ set -x
+ # shellcheck disable=SC2086
+ ${PACKAGING_TOOL_CMD} uninstall ${EXTRA_UNINSTALL_FLAGS} flask-appbuilder
|| true
+ # shellcheck disable=SC2086
+ ${PACKAGING_TOOL_CMD} install ${EXTRA_INSTALL_FLAGS} --upgrade
"sqlalchemy[asyncio]<2.1" databricks-sqlalchemy
Review Comment:
[nitpick] After installing or upgrading packages, it’s a good practice to
run `pip check` to catch any dependency conflicts early. Consider adding `pip
check` after this installation step.
```suggestion
${PACKAGING_TOOL_CMD} install ${EXTRA_INSTALL_FLAGS} --upgrade
"sqlalchemy[asyncio]<2.1" databricks-sqlalchemy
pip check
```
##########
scripts/docker/entrypoint_ci.sh:
##########
@@ -271,6 +271,22 @@ function check_boto_upgrade() {
set +x
}
+# Upgrade sqlalchemy to the latest version to run tests with it
+function check_upgrade_sqlalchemy() {
Review Comment:
The `check_upgrade_sqlalchemy` implementation is duplicated in both
`entrypoint_ci.sh` and `Dockerfile.ci`. Consider extracting it into a shared
helper script or sourcing a single definition to avoid code duplication.
##########
scripts/docker/entrypoint_ci.sh:
##########
@@ -197,14 +197,14 @@ function handle_mount_sources() {
echo
echo "${COLOR_BLUE}Mounted sources are removed, cleaning up mounted
dist-info files${COLOR_RESET}"
echo
- rm -rf
/usr/local/lib/python${PYTHON_MAJOR_MINOR_VERSION}/site-packages/apache_airflow*.dist-info/
+ rm -rf
/usr/local/lib/python"${PYTHON_MAJOR_MINOR_VERSION}"/site-packages/apache_airflow*.dist-info/
fi
}
# Determine which airflow version to use
function determine_airflow_to_use() {
USE_AIRFLOW_VERSION="${USE_AIRFLOW_VERSION:=""}"
- if [[ ${USE_AIRFLOW_VERSION} == "" && ${USE_DISTRIBUTIONS_FROM_DIST=} !=
"true" ]]; then
+ if [[ "${USE_AIRFLOW_VERSION}" == "" && ${USE_DISTRIBUTIONS_FROM_DIST=} !=
"true" ]]; then
Review Comment:
In the conditional, `USE_DISTRIBUTIONS_FROM_DIST` is unquoted and could be
split if empty or contain spaces. For consistency and safety, wrap it in
quotes: `"${USE_DISTRIBUTIONS_FROM_DIST}"`.
```suggestion
if [[ "${USE_AIRFLOW_VERSION}" == "" && "${USE_DISTRIBUTIONS_FROM_DIST}"
!= "true" ]]; then
```
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
