potiuk commented on a change in pull request #19737:
URL: https://github.com/apache/airflow/pull/19737#discussion_r754454633
##########
File path: scripts/ci/images/ci_run_docker_tests.sh
##########
@@ -0,0 +1,121 @@
+#!/usr/bin/env bash
Review comment:
Not really:
1) BUILD_CACHE_DIR is always `AIRFLOW_SOURCES/.build` - the ENV var is only
defined once to use it in many scripts and do not make a typo, but this could
be easily hardcoded in the python script and we can generalize it later when we
change the approach in similar way to "Chart unit tests" and "Helm tests'
(whcih we will be able to next)
2) pip install --upgrade pytest pytest-xdist - does not neeed constraints
(that leaves most of the variables out). There is no reason why "environment"
for "docker_tests" should be the same as Airifow one - we do not even install
(and have no intentions to install) airflow here
3) similarly there is no reason to use the same pip and wheel versions -
those are really needed to install airflow (which actually happens when we run
Helm tests because we are using imports from airflow there). Just to install
pytest and pytest-xdist we can use "current PIP and no wheel" - we do not even
need to reinstall them
So what I am saying - this case and environment is far simpler, so it could
be initialized in way simpler way - that's also one reason we should not carry
all the "Breeze" baggage here - it's simply not needed here.
##########
File path: scripts/ci/images/ci_run_docker_tests.sh
##########
@@ -0,0 +1,121 @@
+#!/usr/bin/env bash
Review comment:
And BTW.
I really like the idea of small, dedicated venvs for different tasks -
separate venv for "dev", separate for "docker_tests", separate for
"chart/tests" - especially if we add some scripting that will create them when
needed automtically. So we should reapply this to other cases in the future.
--
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]
