potiuk commented on issue #4938: [AIRFLOW-4117] Multi-staging Image - Travis CI tests [Step 3/3] URL: https://github.com/apache/airflow/pull/4938#issuecomment-507757213 Re: changes in tests/* I think some of them were because of the pylint started to detect them (but it is likely it was because of the pre-commit hook). I will try to revert all of those and see if they fail in the new environment (luckily it is super easy to exact-reproduce it with the pylint docker environment using ci-slim docker). Previously some of the local pylint errors were not showing up in CI but they were showing up locally due to different pylint version and local configuration. I am not sure where this came from https://github.com/apache/airflow/pull/4938/files/b32a836fac5eeea85f09ad5460287c122911be64#diff-9753408ee5d93e5869f8ad8a047776e0R64 :). Will revert. Some other changes are actually related to the new environment changes: - Change of the run_unit_tests.sh script name: https://github.com/apache/airflow/pull/4938/files/b32a836fac5eeea85f09ad5460287c122911be64#diff-63f689d4fbfcc88aeebc63c85611ef41R83 - I changed the "run_unit_tests.sh" name "run_tests.sh" which is far more appropriate (those are not only unit tests). So I figured updating source code comment that refers to it together with name change is a good practice :). - AIRFLOW_SOURCES_DIR was a wrong name in this comment. I changed it (but wrongly) it should be AIRFLOW_SOURCES instead (I cleaned up and standardised use of AIRFLOW_SOURCE/AIRFLOW_ROOT variables as it was quite messy). It is a comment only but I will fix it - https://github.com/apache/airflow/pull/4938/files/b32a836fac5eeea85f09ad5460287c122911be64#diff-e5927ea2fff7d7b3c1de8d69fffd1f35R43 - the change with impersonation : https://github.com/apache/airflow/pull/4938/files/b32a836fac5eeea85f09ad5460287c122911be64#diff-86b0f172baeb73219800925605a34b07R42 This was is the problem was that in the new environment this test fails because the whole test environment was modified to support this case. It was one of the first failing test in the new CI environment which I fixed. I could not easily do exactly the same approach in the exact same way as it was done in the old environment. For one in the new environment everything runs as root. This is another fix (along the way) because of the way local sources are mounted on Linux when you try to run tests locally. Current way when sources are mounted and airflow user is used, works on Mac but when you try to run it on Linux with docker it breaks in cases of generated npm code etc. In the new environment starting docker with mounted local sources works also on Linux . Originally it was done in test_run script and it was very difficult to find out that it is related to these impersonation tests. And it was actually quite bad approach as it was modifying the dag folder for o+ access - ALWAYS - just to handle this one case. I did not want to propagate the problem to the new environment - I instead decided to fix it in the tests - it seems much more appropriate. I can try to make separate PR out of that but then it would mean I will have to do quite some work to re-introduce the bad environment configuration back in the new one, apply the fix and remove the workaround. I think it is not worth it - really. WDYT?
---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services
