potiuk commented on a change in pull request #4938: [AIRFLOW-4117] Multi-staging Image - Travis CI tests [Step 3/3] URL: https://github.com/apache/airflow/pull/4938#discussion_r299567705
########## File path: tests/contrib/hooks/test_sftp_hook.py ########## @@ -25,14 +25,29 @@ from airflow.contrib.hooks.sftp_hook import SFTPHook from airflow.models import Connection +from airflow.utils.db import provide_session TMP_PATH = '/tmp' TMP_DIR_FOR_TESTS = 'tests_sftp_hook_dir' TMP_FILE_FOR_TESTS = 'test_file.txt' +SFTP_CONNECTION_USER = "root" + class SFTPHookTest(unittest.TestCase): + + @provide_session + def update_connection(self, login, session=None): + connection = (session.query(Connection). + filter(Connection.conn_id == "sftp_default") + .first()) + old_login = connection.login + connection.login = login + session.commit() + return old_login Review comment: Not really. I argue this 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
